Skip to content
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

Create a server-specific bottom-most symbol provider #1724

Open
david-perez opened this issue Sep 9, 2022 · 2 comments
Open

Create a server-specific bottom-most symbol provider #1724

david-perez opened this issue Sep 9, 2022 · 2 comments
Labels
high-priority High priority issue refactoring Changes that do not affect our users, mostly focused on maintainability server Rust server SDK

Comments

@david-perez
Copy link
Contributor

Currently, both client and server projects have been using the same bottom-most (i.e. the core symbol provider that sits at the bottom of the hierarchy of "wrappers") symbol provider, defined in SymbolVisitor.kt:

https://github.com/awslabs/smithy-rs/blob/e3239e1a17ea0165849ce5379ae0fe40bdb16577/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RustCodegenPlugin.kt#L63

The mapping between a Smithy shape and a Symbol, the role of the symbol provider, has become ever more different between the client and the server projects. The first deviation was introduced when the server first decided to generate required member shapes as non-Optional, in #1148. This was tackled introducing a handleRequired configuration flag in the SymbolVisitorConfig:

https://github.com/awslabs/smithy-rs/blob/e3239e1a17ea0165849ce5379ae0fe40bdb16577/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt#L67

We now have more need for deviations:

  • Since Add new service builder codegen #1693, we now generate a Rust struct for each operation shape. The current SymbolVisitor generates other Rust structs for use by the client.
  • Since Add new service builder codegen #1693, we now generate a "service builder" Rust struct for the service (link). The current SymbolVisitor cannot be invoked on a service shape.
  • As a result of "constrained types" in Builders of builders #1342, we will introduce several other symbol providers to provide symbols for constrained and unconstrained shapes. I think these could be slightly unified if the server project had its own bottom-most symbol provider.
  • The advent of IDL v2 further complicates when a member shape should be generated as Option or not for both projects.

Introducing more configuration flags and increasing the branching in the current SymbolVisitor to accomodate all these different requirements will make the code increasingly complex, hampering maintainability.

In this task we will split the current SymbolVisitor into at least two classes across three files:

  1. CoreSymbolProviderUtilities will live in the recently introduced codegen-core Gradle module (Establish the codegen-core module #1697) and will provide utilities to construct Symbols, espeically those for core simple Smithy shapes whose associated Rust types are common among all projects.
  2. ClientSymbolProvider will live in the client Gradle module and be specifically tailored and unit-tested for the client project's needs.
  3. ServerSymbolProvider will live in the codegen-server Gradle module and be specifically tailored and unit-tested for the server project's needs.

Note we get rid of the "visitor" part of the name.


Note the Python server project is using its own bottom-most symbol provider:

https://github.com/awslabs/smithy-rs/blob/e3239e1a17ea0165849ce5379ae0fe40bdb16577/codegen-server/python/src/main/kotlin/software/amazon/smithy/rust/codegen/server/python/smithy/PythonCodegenServerPlugin.kt#L72

@jdisanti
Copy link
Collaborator

jdisanti commented Sep 9, 2022

I like it! Only thing I would consider changing is the name of CoreSymbolProviderUtilities. In my opinion, "utilities" or "util" don't really describe what a thing is, and make that thing a dumping ground for all sorts of loosely related code. Over time, utilities become spaghetti. Perhaps we can do this:

  1. Move the Symbol extension functions to a SymbolExt.kt file in core
  2. Move reusable shape to symbol conversions into a new RustSymbols.kt in core

What do you think about this?

@david-perez
Copy link
Contributor Author

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority High priority issue refactoring Changes that do not affect our users, mostly focused on maintainability server Rust server SDK
Projects
None yet
Development

No branches or pull requests

2 participants