Extract builderInstantiator interface to prepare for nullability changes #2988

rcoh · 2023-09-15T20:28:41Z

Motivation and Context

#1725 exposed the need for easily configuring builder behavior between client & server

Description

extract builderGenerator interface and attach it to codegenContext to avoid loads of threading it up and down

Testing

codegen unit tests
codegen diff audit

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2023-09-18T13:44:48Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
Server Test Python (ignoring whitespace)
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

rcoh · 2023-09-18T14:05:35Z

audited codegen, no diff

rcoh · 2023-09-18T14:05:51Z

no changelog, internal only change

github-actions · 2023-09-18T14:38:10Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

david-perez

The only blocking comment is the one in JsonParserGenerator.kt, which should be artifact agnostic, but is currently passing in a value to map_err that only applies to client generation.

david-perez · 2023-09-18T14:34:59Z

...ain/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderInstantiator.kt

+import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
+import software.amazon.smithy.rust.codegen.core.rustlang.writable
+
+/** Abstraction for instantiating a builders.


Suggested change

/** Abstraction for instantiating a builders.

/**

* Abstraction for instantiating builders.

david-perez · 2023-09-18T14:39:01Z

...e/amazon/smithy/rust/codegen/core/smithy/protocols/parse/EventStreamUnmarshallerGenerator.kt

@@ -339,6 +340,7 @@ class EventStreamUnmarshallerGenerator(
                    // TODO(EventStream): Errors on the operation can be disjoint with errors in the union,
                    //  so we need to generate a new top-level Error type for each event stream union.
                    when (codegenTarget) {
+                        // TODO(https://github.com/awslabs/smithy-rs/issues/1970) It should be possible to unify these branches now


Why should it be possible?

because we have the builderInstantiator, I think. But it's possible this is more divergence that can't be handled like that.

david-perez · 2023-09-18T14:42:03Z

...e/amazon/smithy/rust/codegen/core/smithy/protocols/parse/EventStreamUnmarshallerGenerator.kt

@@ -352,9 +354,19 @@ class EventStreamUnmarshallerGenerator(
                                        })?;
                                    builder.set_meta(Some(generic));


Aside: not sure why we bind the error metadata to a variable named generic.

david-perez · 2023-09-18T14:43:08Z

...ain/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderInstantiator.kt

+    /** Set a field on a builder. */
+    fun setField(builder: String, value: Writable, field: MemberShape): Writable
+
+    /** Finalize a builder, turning into a built object (or in the case of builders-of-builders, return the builder directly).*/


Suggested change

/** Finalize a builder, turning into a built object (or in the case of builders-of-builders, return the builder directly).*/

/** Finalize a builder, turning it into a built object (or in the case of builders-of-builders, return the builder directly).*/

david-perez · 2023-09-18T14:44:08Z

...ain/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderInstantiator.kt

+    fun setField(builder: String, value: Writable, field: MemberShape): Writable
+
+    /** Finalize a builder, turning into a built object (or in the case of builders-of-builders, return the builder directly).*/
+    fun finalizeBuilder(builder: String, shape: StructureShape, mapErr: Writable? = null): Writable


Worth documenting mapErr.

david-perez · 2023-09-18T14:49:15Z

...otlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt

+                        "builder", shape,
+                    ) {
+                        rustTemplate(
+                            """|err|#{Error}::custom_source("Response was invalid", err)""", *codegenScope,


This doesn't look correct to me. This only applies to clients, not servers. It only works because ServerBuilderInstantiator does not rely on the passed in mapErr writable.

mapErr defines how to convert the error back into the caller error—since the server never returns an error, it doesn't need to use it.

david-perez · 2023-09-18T14:50:54Z

...kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/TestBuilderInstantiator.kt

@@ -0,0 +1,6 @@
+/*


I don't understand what this file's purpose is.

whoops, thanks. My intellij git broken good catch

david-perez · 2023-09-18T14:52:23Z

...in/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerInstantiator.kt

+) :
+    BuilderInstantiator {


Suggested change

) :

BuilderInstantiator {

) : BuilderInstantiator {

The current formatting looks odd.

david-perez · 2023-09-18T14:56:32Z

...ain/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderInstantiator.kt

+    fun finalizeBuilder(builder: String, shape: StructureShape, mapErr: Writable? = null): Writable
+
+    /** Set a field on a builder using the `$setterName` method. $value will be passed directly. */
+    fun setFieldBase(builder: String, value: Writable, field: MemberShape) = writable {


nit:

Suggested change

fun setFieldBase(builder: String, value: Writable, field: MemberShape) = writable {

fun setFieldWithSetter(builder: String, value: Writable, field: MemberShape) = writable {

Base denotes we're doing inheritance. Which we kind of are doing manually in the implementations of this interface.

github-actions · 2023-09-19T14:18:44Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

rcoh requested review from a team as code owners September 15, 2023 20:28

rcoh force-pushed the builder-instantiator branch from 432642d to 9a6bd21 Compare September 18, 2023 13:11

rcoh force-pushed the builder-instantiator branch from 9a6bd21 to a81109b Compare September 18, 2023 14:03

Extract builderInstantiator interface to prepare for nullability changes

eacf466

rcoh force-pushed the builder-instantiator branch from a81109b to eacf466 Compare September 18, 2023 14:10

rcoh added needs-sdk-review needs-server-review labels Sep 18, 2023

david-perez approved these changes Sep 18, 2023

View reviewed changes

david-perez removed the needs-server-review label Sep 18, 2023

Velfi approved these changes Sep 18, 2023

View reviewed changes

CR feedback

217e019

rcoh enabled auto-merge September 19, 2023 13:56

rcoh added this pull request to the merge queue Sep 19, 2023

Merged via the queue into main with commit 5239e9c Sep 19, 2023

rcoh deleted the builder-instantiator branch September 19, 2023 14:28

Velfi removed the needs-sdk-review label Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract builderInstantiator interface to prepare for nullability changes #2988

Extract builderInstantiator interface to prepare for nullability changes #2988

rcoh commented Sep 15, 2023 •

edited

Loading

github-actions bot commented Sep 18, 2023

rcoh commented Sep 18, 2023

rcoh commented Sep 18, 2023

github-actions bot commented Sep 18, 2023

david-perez left a comment

david-perez Sep 18, 2023

david-perez Sep 18, 2023

rcoh Sep 18, 2023

david-perez Sep 18, 2023

david-perez Sep 18, 2023

david-perez Sep 18, 2023

david-perez Sep 18, 2023

rcoh Sep 18, 2023

david-perez Sep 18, 2023

rcoh Sep 18, 2023

david-perez Sep 18, 2023

david-perez Sep 18, 2023

github-actions bot commented Sep 19, 2023

	/** Abstraction for instantiating a builders.
	/**
	* Abstraction for instantiating builders.

		@@ -352,9 +354,19 @@ class EventStreamUnmarshallerGenerator(
		})?;
		builder.set_meta(Some(generic));

	/** Finalize a builder, turning into a built object (or in the case of builders-of-builders, return the builder directly).*/
	/** Finalize a builder, turning it into a built object (or in the case of builders-of-builders, return the builder directly).*/

	fun setFieldBase(builder: String, value: Writable, field: MemberShape) = writable {
	fun setFieldWithSetter(builder: String, value: Writable, field: MemberShape) = writable {

Extract builderInstantiator interface to prepare for nullability changes #2988

Extract builderInstantiator interface to prepare for nullability changes #2988

Conversation

rcoh commented Sep 15, 2023 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Sep 18, 2023

rcoh commented Sep 18, 2023

rcoh commented Sep 18, 2023

github-actions bot commented Sep 18, 2023

david-perez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2023

rcoh commented Sep 15, 2023 •

edited

Loading