-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Break codegen-server's dependency on codegen-client
#2105
Merged
Merged
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
53e25af
Move the allow lints customization into `codegen-core`
jdisanti ed84f25
Move the crate version customization into `codegen-core`
jdisanti bc804ce
Move "pub use" extra into `codegen-core`
jdisanti 46494d4
Move `EventStreamSymbolProvider` into `codegen-core`
jdisanti 40f876a
Move the streaming shape providers into `codegen-core`
jdisanti 608f1c4
Refactor event stream marshall/unmarshall tests
jdisanti 69cdfc8
Break `codegen-server` dependency on `codegen-client`
jdisanti b8b6ed5
Split up `EventStreamTestTools`
jdisanti d93346e
Move codegen context creation in event stream tests
jdisanti 52899f9
Restructure tests so that #1442 is easier to resolve in the future
jdisanti 5f7bc7e
Fix comments in `SmithyTypesPubUseExtra`
jdisanti 2dd9881
Use pair instead of list
jdisanti 2659182
Merge remote-tracking branch 'origin/main' into jdisanti-break-client…
jdisanti be77fa0
Simplify base event stream test requirements
jdisanti c608a77
Explode on unrecognized shape types in event stream test tools
jdisanti 5c73370
Add client/server prefixes to test classes
jdisanti a47597f
Improve TODO comments in server event stream tests
jdisanti 0b77d8f
Use correct builders for `ServerEventStreamMarshallerGeneratorTest`
jdisanti c898100
Merge remote-tracking branch 'origin/main' into jdisanti-break-client…
jdisanti 38598f2
Fix shape check in event stream test tools
jdisanti 80ed7a9
Remove test cases for protocols that don't support event streams
jdisanti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
53 changes: 53 additions & 0 deletions
53
...on/smithy/rust/codegen/client/smithy/protocols/eventstream/EventStreamBaseRequirements.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package software.amazon.smithy.rust.codegen.client.smithy.protocols.eventstream | ||
|
|
||
| import software.amazon.smithy.model.Model | ||
| import software.amazon.smithy.model.shapes.ServiceShape | ||
| import software.amazon.smithy.model.shapes.ShapeId | ||
| import software.amazon.smithy.model.shapes.StructureShape | ||
| import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext | ||
| import software.amazon.smithy.rust.codegen.client.smithy.customize.CombinedClientCodegenDecorator | ||
| import software.amazon.smithy.rust.codegen.client.testutil.clientTestRustSettings | ||
| import software.amazon.smithy.rust.codegen.client.testutil.testSymbolProvider | ||
| import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter | ||
| import software.amazon.smithy.rust.codegen.core.smithy.CodegenTarget | ||
| import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider | ||
| import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator | ||
| import software.amazon.smithy.rust.codegen.core.smithy.generators.implBlock | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestRequirements | ||
|
|
||
| abstract class EventStreamBaseRequirements : EventStreamTestRequirements<ClientCodegenContext> { | ||
| override fun createCodegenContext( | ||
| model: Model, | ||
| symbolProvider: RustSymbolProvider, | ||
| serviceShape: ServiceShape, | ||
| protocolShapeId: ShapeId, | ||
| codegenTarget: CodegenTarget, | ||
| ): ClientCodegenContext = ClientCodegenContext( | ||
| model, | ||
| symbolProvider, | ||
| serviceShape, | ||
| protocolShapeId, | ||
| clientTestRustSettings(), | ||
| CombinedClientCodegenDecorator(emptyList()), | ||
| ) | ||
|
|
||
| override fun createSymbolProvider(model: Model): RustSymbolProvider = testSymbolProvider(model) | ||
|
|
||
| override fun renderBuilderForShape( | ||
| writer: RustWriter, | ||
| codegenContext: ClientCodegenContext, | ||
| shape: StructureShape, | ||
| ) { | ||
| BuilderGenerator(codegenContext.model, codegenContext.symbolProvider, shape).apply { | ||
| render(writer) | ||
| writer.implBlock(shape, codegenContext.symbolProvider) { | ||
| renderConvenienceMethod(writer) | ||
| } | ||
| } | ||
| } | ||
| } | ||
58 changes: 58 additions & 0 deletions
58
...hy/rust/codegen/client/smithy/protocols/eventstream/EventStreamMarshallerGeneratorTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package software.amazon.smithy.rust.codegen.client.smithy.protocols.eventstream | ||
|
|
||
| import org.junit.jupiter.api.extension.ExtensionContext | ||
| import org.junit.jupiter.params.ParameterizedTest | ||
| import org.junit.jupiter.params.provider.Arguments | ||
| import org.junit.jupiter.params.provider.ArgumentsProvider | ||
| import org.junit.jupiter.params.provider.ArgumentsSource | ||
| import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext | ||
| import software.amazon.smithy.rust.codegen.core.smithy.CodegenTarget | ||
| import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType | ||
| import software.amazon.smithy.rust.codegen.core.smithy.protocols.Protocol | ||
| import software.amazon.smithy.rust.codegen.core.smithy.protocols.serialize.EventStreamMarshallerGenerator | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestModels | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestTools | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestVariety | ||
| import software.amazon.smithy.rust.codegen.core.testutil.TestEventStreamProject | ||
| import software.amazon.smithy.rust.codegen.core.testutil.TestRuntimeConfig | ||
| import java.util.stream.Stream | ||
|
|
||
| class MarshallTestCasesProvider : ArgumentsProvider { | ||
| override fun provideArguments(context: ExtensionContext?): Stream<out Arguments> = | ||
| // Don't include awsQuery or ec2Query for now since marshall support for them is unimplemented | ||
| EventStreamTestModels.TEST_CASES | ||
| .filter { testCase -> !testCase.protocolShapeId.contains("Query") } | ||
| .map { Arguments.of(it) }.stream() | ||
| } | ||
|
|
||
| class EventStreamMarshallerGeneratorTest { | ||
| @ParameterizedTest | ||
| @ArgumentsSource(MarshallTestCasesProvider::class) | ||
| fun test(testCase: EventStreamTestModels.TestCase) { | ||
| EventStreamTestTools.runTestCase( | ||
| testCase, | ||
| object : EventStreamBaseRequirements() { | ||
| override fun renderGenerator( | ||
| codegenContext: ClientCodegenContext, | ||
| project: TestEventStreamProject, | ||
| protocol: Protocol, | ||
| ): RuntimeType = EventStreamMarshallerGenerator( | ||
| project.model, | ||
| CodegenTarget.CLIENT, | ||
| TestRuntimeConfig, | ||
| project.symbolProvider, | ||
| project.streamShape, | ||
| protocol.structuredDataSerializer(project.operationShape), | ||
| testCase.requestContentType, | ||
| ).render() | ||
| }, | ||
| CodegenTarget.CLIENT, | ||
| EventStreamTestVariety.Marshall, | ||
| ) | ||
| } | ||
| } |
58 changes: 58 additions & 0 deletions
58
.../rust/codegen/client/smithy/protocols/eventstream/EventStreamUnmarshallerGeneratorTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package software.amazon.smithy.rust.codegen.client.smithy.protocols.eventstream | ||
|
|
||
| import org.junit.jupiter.api.extension.ExtensionContext | ||
| import org.junit.jupiter.params.ParameterizedTest | ||
| import org.junit.jupiter.params.provider.Arguments | ||
| import org.junit.jupiter.params.provider.ArgumentsProvider | ||
| import org.junit.jupiter.params.provider.ArgumentsSource | ||
| import software.amazon.smithy.codegen.core.Symbol | ||
| import software.amazon.smithy.model.shapes.StructureShape | ||
| import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext | ||
| import software.amazon.smithy.rust.codegen.core.smithy.CodegenTarget | ||
| import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType | ||
| import software.amazon.smithy.rust.codegen.core.smithy.generators.builderSymbol | ||
| import software.amazon.smithy.rust.codegen.core.smithy.protocols.Protocol | ||
| import software.amazon.smithy.rust.codegen.core.smithy.protocols.parse.EventStreamUnmarshallerGenerator | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestModels | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestTools | ||
| import software.amazon.smithy.rust.codegen.core.testutil.EventStreamTestVariety | ||
| import software.amazon.smithy.rust.codegen.core.testutil.TestEventStreamProject | ||
| import java.util.stream.Stream | ||
|
|
||
| class UnmarshallTestCasesProvider : ArgumentsProvider { | ||
| override fun provideArguments(context: ExtensionContext?): Stream<out Arguments> = | ||
| EventStreamTestModels.TEST_CASES.map { Arguments.of(it) }.stream() | ||
| } | ||
|
|
||
| class EventStreamUnmarshallerGeneratorTest { | ||
| @ParameterizedTest | ||
| @ArgumentsSource(UnmarshallTestCasesProvider::class) | ||
| fun test(testCase: EventStreamTestModels.TestCase) { | ||
| EventStreamTestTools.runTestCase( | ||
| testCase, | ||
| object : EventStreamBaseRequirements() { | ||
| override fun renderGenerator( | ||
| codegenContext: ClientCodegenContext, | ||
| project: TestEventStreamProject, | ||
| protocol: Protocol, | ||
| ): RuntimeType { | ||
| fun builderSymbol(shape: StructureShape): Symbol = shape.builderSymbol(codegenContext.symbolProvider) | ||
| return EventStreamUnmarshallerGenerator( | ||
| protocol, | ||
| codegenContext, | ||
| project.operationShape, | ||
| project.streamShape, | ||
| ::builderSymbol, | ||
| ).render() | ||
| } | ||
| }, | ||
| CodegenTarget.CLIENT, | ||
| EventStreamTestVariety.Unmarshall, | ||
| ) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we agree on a naming strategy once and for all on classes that have equivalents across
codegen-clientandcodegen-server? Most (but not all) of the classes incodegen-serverare prefixed withServer, while few of the classes incodegen-clientare prefixed withClient. Likewise, some classes fromcodegen-corethat we inherit from in the other modules are prefixed withCore.My preference would be to not prefix things with
Core, but have everything prefixed withClientandServerthat comes fromcodegen-clientandcodegen-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 haveServerRestJsonincodegen-serverandRestJsonincodegen-client, so the latter should be renamed toClientRestJson, but there's no need to prefixConstraintViolationSymbolProviderwithServer, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5c73370.