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.
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
chore: OrphanedShapes TestModel #744
base: main-1.x
Are you sure you want to change the base?
chore: OrphanedShapes TestModel #744
Changes from all commits
92440a6
60160ab
2f49bfc
080a0d7
ea69572
e01c79d
56a1be6
39e8c5e
a73fee0
8df6dd4
a30f9d9
b506b65
96c568f
398e899
5ff1750
fd8164c
20a00f7
6816df5
8aae719
b03fc10
4df19f1
d5cafd5
daae9d9
20cc166
c77ffab
a1af584
f8eaf5e
b4bb57a
4e39e8c
a049cd7
6cf63f0
1f91128
54ef386
f2488ce
5dc6296
2366eab
b92e7b7
25a49fc
c93de72
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Most of this content should exist outside of a specific TestModel IMO. I'd suggest moving a good chunk into a separate file under
designs
.I'd also suggest naming the complete list of use cases that leads us to deviate from Smithy-Core's approach, just to be clear about which are fundamental (like config shapes) and which are closer to bugs in our implementations.
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 can do some content moving.
I don't like writing a complete list anywhere. It's a long list (I'd estimate 30+) and it will get stale quickly. This list has recently gotten longer due to Tony's mutations changes.
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.
30+ individual reasons we need orphaned shapes? Yikes!
I guess what you're saying is: if I wanted to get to a place where local services didn't need generation for orphaned shapes, I'd be better off building that and testing it against all the existing libraries to identify gaps?
(Thanks for satisfying my curiousity on these question btw, I have no intention of blocking this PR as we clearly should be explicitly testing a feature we depend on, I just want to take the opportunity to understand the scope of the feature better, since you would have had to dive into it a certain depth anyway)
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.
Yes, I think so.
I think we're landing on the idea of the "ideal state", which would be using Smithy-Core's shape discovery logic plus a bounded, limited set of orphaned shapes (like local service config shapes).
And the path to get there may just be a breaking change to Smithy-Dafny (made by testing against complex libraries) combined with Smithy model changes to downstream libraries
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.
This is the main "legit" reason to deviate at least, and I'm hoping in the future we can model local service client creation as an operational as well somehow, since then it wouldn't be a special case.
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.
Why are these orphaned? Is it because errors from dependency services are used but not directly connected?
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.
Errors will be orphaned if they are not attached to any operations
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.
Right, but I assume we still need conversions for them for some reason, just trying to understand that better
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.
In my mind, it's because Java generates conversions for orphaned errors, so other languages should also generate conversions for parity.
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.
These should still be listed as
resources
on the local service though, right?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.
Maybe, but in many cases they aren't
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 also believe that Smithy-Core doesn't discover resources attached to the service -- it's only mixins, operations, and errors)
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.
That wouldn't surprise me given no other code generators produce anything for resources yet. I would still argue Smithy-Core probably should traverse them even if no one else does anything for them yet, so I might try to submit that change to Smithy-Core down the road...
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.
NVM! Resources attached to services are discovered
https://github.com/smithy-lang/smithy/blob/main/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborVisitor.java#L89-L91
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.
Which structures other than config structures?
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.
The only examples are unused structures in the MPL.
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.
Which ones specifically? I don't yet appreciate the use case
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.
There's this one https://github.com/aws/aws-cryptographic-material-providers-library/blob/23650a9c6e1c897455e2d59d15b7d7a2c5a07cdd/AwsCryptographyPrimitives/Model/aes.smithy#L62-L69
There's some unions too https://github.com/aws/aws-cryptographic-material-providers-library/blob/23650a9c6e1c897455e2d59d15b7d7a2c5a07cdd/AwsCryptographicMaterialProviders/dafny/AwsCryptographicMaterialProviders/Model/key-agreement-scheme.smithy#L5-L28
Then there may also be descendents of Config shapes, if Config shapes have orphaned members, like in DBESDK https://github.com/aws/aws-database-encryption-sdk-dynamodb/blob/main/DynamoDbEncryption/dafny/DynamoDbEncryption/Model/DynamoDbEncryption.smithy#L145-L210