Break codegen-server's dependency on codegen-client
#2105
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
jdisanti
added
needs-sdk-review
needs-server-review
needs-review
There was a problem hiding this comment.
The builder used in the server tests is still the client one. In the server we need to exercise two builder generators, ServerBuilderGenerator and ServerBuilderGeneratorWithoutPublicConstrainedTypes, depending on the value of codegenContext.settings.codegenConfig.publicConstrainedTypes.
It would also be nice to move ServerCombinedErrorGenerator from codegen-core to codegen-server.
I think it should be possible to do the above by passing in more closures in the test functions. As an aside, what do you think of us setting up a codegen-core-test subproject? It seems odd to have the subproject-specific unit tests depend on the entirety of the codegen-core module, when what we really want is to depend on shared test code only (e.g. EventStreamTestTools, the test helpers, a central Smithy model bank we can reuse so that we don't duplicate models etc.) I briefly looked into setting up a shared test subproject with Gradle but couldn't figure it out (see the two StackOverflow links at the end of #1442 (comment)).
| class UnmarshallTestCasesProvider : ArgumentsProvider { | ||
| override fun provideArguments(context: ExtensionContext?): Stream<out Arguments> = | ||
| EventStreamTestModels.TEST_CASES.map { Arguments.of(it) }.stream() | ||
| } |
There was a problem hiding this comment.
Isn't this identical to UnmarshallTestCasesProvider from software.amazon.smithy.rust.codegen.server.smithy.protocols.parse? Should we put this in codegen-core to centralize?
There was a problem hiding this comment.
Perhaps in EventStreamTestTools?
There was a problem hiding this comment.
Same goes for software.amazon.smithy.rust.codegen.server.smithy.protocols.serialize.MarshallTestCasesProvider and software.amazon.smithy.rust.codegen.client.smithy.protocols.serialize.MarshallTestCasesProvider.
There was a problem hiding this comment.
Although in the server we don't support ec2Query or awsQuery, and we very badly support restXml, so maybe we want to filter those out in the server too.
Although then again, if these specific event stream tests pass in the server I guess there's no harm in leaving them in so as to not regress in case we want to support those protocols in the future, so I'd just make the test case providers identical for both subprojects.
There was a problem hiding this comment.
I kept these argument providers in codegen-client and codegen-server to avoid needing to depend on junit in codegen-core. However, if we set up a codegen-core-test module, then that won't be a problem anymore.
There was a problem hiding this comment.
I played around with creating codegen-core-test, and it seems like it might be the right path forward long-term. However, it's a large scale refactor that has rippling effects that will require making codegen-client-test a Kotlin module so that sdk-codegen can depend on test-only code from codegen-client.
To do it nicely, we're going to have to move all of codegen-core's unit tests into codegen-core-test/src/test, while test code that is intended to be shared will need to reside in codegen-core-test/src/main. I was able to create and populate codegen-core-test easily enough, but then code in codegen-client/src/main no longer compiles due to requiring test-only code in src/main so that sdk-codegen can reuse that client-specific test-only code.
One consequence of going through with this is that there will be no way to unit test code that is internal, and if we decide to reuse the existing codegen-client-test and codegen-server-test modules, they will run both unit tests and codegen tests (but maybe that's desirable?).
I think I'm going to punt on this for this PR.
| @@ -25,7 +25,6 @@ val kotestVersion: String by project | |||
|
|
|||
| dependencies { | |||
| implementation(project(":codegen-core")) | |||
| implementation(project(":codegen-client")) | |||
...on/smithy/rust/codegen/server/smithy/protocols/parse/EventStreamUnmarshallerGeneratorTest.kt
Outdated
Show resolved
Hide resolved
...re/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/EventStreamTestTools.kt
Outdated
Show resolved
Hide resolved
...lin/software/amazon/smithy/rust/codegen/core/smithy/customizations/SmithyTypesPubUseExtra.kt
Outdated
Show resolved
Hide resolved
...lin/software/amazon/smithy/rust/codegen/core/smithy/customizations/SmithyTypesPubUseExtra.kt
Outdated
Show resolved
Hide resolved
| .toList() | ||
| when (codegenTarget) { | ||
| CodegenTarget.CLIENT -> CombinedErrorGenerator(model, symbolProvider, operationSymbol, errors).render(this) | ||
| CodegenTarget.SERVER -> ServerCombinedErrorGenerator(model, symbolProvider, operationSymbol, errors).render(this) |
There was a problem hiding this comment.
We're still not solving the core issue of subproject independence. ServerCombinedErrorGenerator should not be in codegen-core.
There was a problem hiding this comment.
Could we pass the subproject-specific combined error generator's render function in to EventStreamTestTools.runTestCase (just like we're doing with EventStreamUnmarshallerGenerator.render)?
There was a problem hiding this comment.
I actually did move ServerCombinedErrorGenerator into codegen-server in #2107. I didn't want to make this PR too large.
...re/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/EventStreamTestTools.kt
Outdated
Show resolved
Hide resolved
|
@david-perez - Thanks for the thorough feedback! I think I incorporated everything, or left a reply explaining why not. Let me know if I missed something or if you disagree on a point. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
…-server-dependency
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
| codegenContext: ServerCodegenContext, | ||
| shape: StructureShape, | ||
| ) { | ||
| // TODO(https://github.com/awslabs/smithy-rs/issues/1442): Use the correct builder: |
There was a problem hiding this comment.
If you're punting on this, mention in the TODO that we need to test with two builders: ServerBuilderGeneratorWithoutPublicConstrainedTypes and ServerBuilderGenerator.
Same goes for the software.amazon.smithy.rust.codegen.server.smithy.protocols.eventstream.EventStreamMarshallerGeneratorTest class, which is only testing with ServerBuilderGenerator.
There was a problem hiding this comment.
Fixed in a47597f. Edit: Actually, I missed part... working on it.
There was a problem hiding this comment.
Fixed the marshaller side of things in 0b77d8f.
| import software.amazon.smithy.rust.codegen.core.smithy.generators.implBlock | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestRequirements | ||
|
|
||
| abstract class EventStreamBaseRequirements : EventStreamTestRequirements<ClientCodegenContext> { |
There was a problem hiding this comment.
Should we agree on a naming strategy once and for all on classes that have equivalents across codegen-client and codegen-server? Most (but not all) of the classes in codegen-server are prefixed with Server, while few of the classes in codegen-client are prefixed with Client. Likewise, some classes from codegen-core that we inherit from in the other modules are prefixed with Core.
My preference would be to not prefix things with Core, but have everything prefixed with Client and Server that comes from codegen-client and codegen-server, respectively. While redundant (the information is encoded in the classpath), I like that one can tell at a glance the purpose of things. This would only apply to classes that have equivalents across the subprojects (e.g. we have ServerRestJson in codegen-server and RestJson in codegen-client, so the latter should be renamed to ClientRestJson, but there's no need to prefix ConstraintViolationSymbolProvider with Server, since constraint traits are ignored in the client).
We could even attempt writing a lint that enforced the naming strategy in CI. What do you think?
There was a problem hiding this comment.
I'm fine with your proposed naming strategy. It definitely makes it easier to pull up the correct class in IntelliJ. I'll rename these ones.
There was a problem hiding this comment.
Fixed in 5c73370.
| CodegenTarget.CLIENT -> CombinedErrorGenerator(model, symbolProvider, operationSymbol, errors).render(this) | ||
| CodegenTarget.SERVER -> ServerCombinedErrorGenerator(model, symbolProvider, operationSymbol, errors).render(this) | ||
| } | ||
| for (shape in model.shapes().filter { shape -> shape.isStructureShape && shape.hasTrait<ErrorTrait>() }) { |
There was a problem hiding this comment.
| for (shape in model.shapes().filter { shape -> shape.isStructureShape && shape.hasTrait<ErrorTrait>() }) { | |
| for (shape in model.shapes().filter { shape -> shape is StructureShape && shape.hasTrait<ErrorTrait>() }) { |
nit: I favor is checks instead of using the Smithy library's provided methods because Kotlin is able to do type narrowing.
There was a problem hiding this comment.
Fixed in c608a77.
| ) { | ||
| for (member in shape.members()) { | ||
| val target = model.expectShape(member.target) | ||
| if (target is StructureShape || target is UnionShape) { |
There was a problem hiding this comment.
Is the reason why we only render these shape types that we're not testing with more complex models? If yes, I would crash here upon encountering an unrendered shape type, lest a future dev be puzzled when adding more shapes to the event stream tests.
There was a problem hiding this comment.
Good call. Fixed in c608a77.
There was a problem hiding this comment.
Actually, I forgot to remove the if (target is StructureShape || target is UnionShape) while testing 🤦♂️
Correctly fixed in 38598f2.
| codegenTarget: CodegenTarget, | ||
| ): ServerCodegenContext { | ||
| val settings = serverTestRustSettings() | ||
| val serverSymbolProviders = ServerSymbolProviders.from( |
There was a problem hiding this comment.
Use the software.amazon.smithy.rust.codegen.server.smithy.testutil.serverTestSymbolProviders function.
There was a problem hiding this comment.
Or directly serverTestCodegenContext, since this test is only testing with publicConstrainedTypes enabled.
There was a problem hiding this comment.
Thanks for the pointers! I'm not so familiar with the server testing helpers.
…-server-dependency
...smithy/rust/codegen/client/smithy/protocols/eventstream/ClientEventStreamBaseRequirements.kt
Show resolved
Hide resolved
...rust/codegen/client/smithy/protocols/eventstream/ClientEventStreamMarshallerGeneratorTest.kt
Outdated
Show resolved
Hide resolved
.../software/amazon/smithy/rust/codegen/core/smithy/customizations/CrateVersionCustomization.kt
Show resolved
Hide resolved
...re/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/EventStreamTestTools.kt
Show resolved
Hide resolved
...re/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/EventStreamTestTools.kt
Show resolved
Hide resolved
...n/kotlin/software/amazon/smithy/rust/codegen/core/testutil/EventStreamUnmarshallTestCases.kt
Show resolved
Hide resolved
...n/kotlin/software/amazon/smithy/rust/codegen/core/testutil/EventStreamUnmarshallTestCases.kt
Show resolved
Hide resolved
| @@ -29,7 +29,7 @@ class ServerRequiredCustomizations : ServerCodegenDecorator { | |||
| codegenContext: ServerCodegenContext, | |||
| baseCustomizations: List<LibRsCustomization>, | |||
| ): List<LibRsCustomization> = | |||
| baseCustomizations + CrateVersionGenerator() + AllowLintsGenerator() | |||
| baseCustomizations + CrateVersionCustomization() + AllowLintsCustomization() | |||
.../main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/testutil/ServerTestHelpers.kt
Show resolved
Hide resolved
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
jdisanti
removed
needs-review
Motivation and Context
This PR and fixes #1864.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.