Skip to content

Conversation

@JordonPhillips
Copy link
Contributor

This adds an alternative generation mode that only generates "data shapes". It's a widely requested feature for both this code generator (I can't tell you how many DMs about this I've received) and for other Smithy generators.

These generated types could still be used in a rich way thanks to the schemas backing them - they could still be plugged into any ShapeSerializer or ShapeDeserializer

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

nateprewitt
nateprewitt previously approved these changes Feb 12, 2025
@nateprewitt
Copy link
Contributor

Changes look good, looks like we have a test failing with the changes though.

E ImportError: cannot import name 'ALL_QUERY_STRING_TYPES_INPUT' from 'restjson._private.schemas' (/home/runner/work/smithy-python/smithy-python/codegen/smithy-python-protocol-test/build/smithyprojections/smithy-python-protocol-test/rest-json-1/python-client-codegen/restjson/_private/schemas.py)

@JordonPhillips
Copy link
Contributor Author

I refactored this after all the repo structure changes. I made it so that the new plugin is a separate package and abstracted out some of the functionality that was being done by the client's directed codegen so that it could be shared.

@JordonPhillips
Copy link
Contributor Author

Note that that number of added lines is scarier than reality as I had to copy over a bunch of smithy IDL files for testing.

README.md Outdated
Comment on lines 253 to 257
Input and output shapes (shapes with the `@input` or `@output` traits and
operation inputs / outputs created as part of an operation definition) are not
generated by default. To generate these shapes anyway, remove the traits with
the
[`excludeTraits` transform](https://smithy.io/2.0/guides/smithy-build-json.html#excludetraits):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.. is this something where there could be a footgun later if they are using a (hypothetical) server plugin or the client plugin? Using type codegen in conjunction with client/server codegen will be common.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just calling it out, because, while it would always be possible, I'm not sure we want to point this capability out if it can lead to problems later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Smithy Java generates inputs and outputs without needing the exclusion. We should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather they don't generate them tbh. I can make some improvments in this area though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it a simple toggle in the settings.

Copy link
Contributor

@haydenbaker haydenbaker Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run the generation for the weather service example, pyright throws some errors:

  examples/weather/build/smithy/types/python-type-codegen/weather/_private/schemas.py:569:1 - error: "STRING_LIST" is constant (because it is uppercase) and cannot be redefined (reportConstantRedefinition)
  examples/weather/build/smithy/types/python-type-codegen/weather/_private/schemas.py:1139:1 - error: "STRING_LIST" is constant (because it is uppercase) and cannot be redefined (reportConstantRedefinition)
  examples/weather/build/smithy/types/python-type-codegen/weather/_private/schemas.py:1493:1 - error: "NON_EMPTY_STRING" is constant (because it is uppercase) and cannot be redefined (reportConstantRedefinition)
  examples/weather/build/smithy/types/python-type-codegen/weather/_private/schemas.py:1507:1 - error: "NON_EMPTY_STRING_LIST" is constant (because it is uppercase) and cannot be redefined (reportConstantRedefinition)

This is because the same constant is being produced in the same file:

STRING_LIST = Schema.collection(
    id=ShapeID("aws.auth#StringList"),
    shape_type=ShapeType.LIST,
    ...
)
...
STRING_LIST = Schema.collection(
    id=ShapeID("aws.protocols#StringList"),
    shape_type=ShapeType.LIST,
    ...
)

Smithy Java has the same problem, but it's not checked by anything:

    static final Schema STRING_LIST = Schema.listBuilder(ShapeId.from("aws.auth#StringList"))
        .putMember("member", PreludeSchemas.STRING)
        .build();
    ...
    static final Schema STRING_LIST = Schema.listBuilder(ShapeId.from("aws.protocols#StringList"))
        .putMember("member", PreludeSchemas.STRING)
        .build();

Whether or not this is actually problematic... if the underlying schema is always the same for a given constant, then it's not a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same root issue, the serde methods are duplicated in models.py, leading to reportRedeclaration:

    models.py:892:5 - error: Function declaration "_serialize_string_list" is obscured by a declaration of the same name (reportRedeclaration)
    models.py:901:5 - error: Function declaration "_deserialize_string_list" is obscured by a declaration of the same name (reportRedeclaration)
    models.py:1123:5 - error: Function declaration "_serialize_string_list" is obscured by a declaration of the same name (reportRedeclaration)
    models.py:1132:5 - error: Function declaration "_deserialize_string_list" is obscured by a declaration of the same name (reportRedeclaration)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's because tons of dependencies have those types. This is why the integ test only generates local shapes. There's not really a great workaround other than raising a conflict exception that's a bit more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it turns out, we weren't running validation! I forgot that ModelTransformer doesn't validate by itself. All those string lists were private, so they shouldn't have been pulled in in the first place.

Comment on lines 52 to 56
.withMember("selector", """
:test([id|namespace = 'example.weather'],
[id|namespace = 'example.weather.nested'],
[id|namespace = 'example.weather.nested.more'])
""")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realistically, users probably won't add a selector, so I'd add a test (or modify this one) without the selector. You'll get the build error from pyright throwing reportConstantRedefinition and reportRedeclaration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh I disagree. I think most people aren't gonna want to generate every shape in smithy.aws or whatever random trait package they have. The only ways around that are selectors or creating your own service shape.

This adds an alternative generation mode that only generates "data
shapes".
This adds a validation step to generating a synthetic service to make
sure we aren't generating anything too crazy. It also resolves some
validation issues that cropped up in the integ test.
haydenbaker
haydenbaker previously approved these changes Mar 7, 2025
Co-authored-by: Hayden Baker <haydebak@amazon.com>
@JordonPhillips JordonPhillips merged commit e649340 into develop Mar 7, 2025
2 checks passed
@JordonPhillips JordonPhillips deleted the shapes-only branch March 7, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants