chore: OrphanedShapes TestModel #744
Conversation
lucasmcdonald3
changed the title
| @@ -0,0 +1,57 @@ | |||
| # OrphanedShapes | |||
|
|
|||
| ## Background | |||
There was a problem hiding this comment.
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.
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.
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.
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
|
|
||
| This TestModel tests some instances of orphaned shapes | ||
|
|
||
| - LocalService Config shapes. (Config shapes are "orphaned", but are likely already handled as one-offs by any codegen that's this TestModels' prerequisites) |
There was a problem hiding this comment.
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.
| This TestModel tests some instances of orphaned shapes | ||
|
|
||
| - LocalService Config shapes. (Config shapes are "orphaned", but are likely already handled as one-offs by any codegen that's this TestModels' prerequisites) | ||
| - Errors |
There was a problem hiding this comment.
Why are these orphaned? Is it because errors from dependency services are used but not directly connected?
There was a problem hiding this comment.
Errors will be orphaned if they are not attached to any operations
There was a problem hiding this comment.
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.
In my mind, it's because Java generates conversions for orphaned errors, so other languages should also generate conversions for parity.
|
|
||
| - LocalService Config shapes. (Config shapes are "orphaned", but are likely already handled as one-offs by any codegen that's this TestModels' prerequisites) | ||
| - Errors | ||
| - Resources (with @aws.polymorph#reference trait) and their operations |
There was a problem hiding this comment.
These should still be listed as resources on the local service though, right?
There was a problem hiding this comment.
Maybe, but in many cases they aren't
There was a problem hiding this comment.
(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.
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.
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
| - LocalService Config shapes. (Config shapes are "orphaned", but are likely already handled as one-offs by any codegen that's this TestModels' prerequisites) | ||
| - Errors | ||
| - Resources (with @aws.polymorph#reference trait) and their operations | ||
| - Structures (and structures' members) |
There was a problem hiding this comment.
Which structures other than config structures?
There was a problem hiding this comment.
The only examples are unused structures in the MPL.
There was a problem hiding this comment.
Which ones specifically? I don't yet appreciate the use case
There was a problem hiding this comment.
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
| * Returns true if a conversion function should be written for the shape, false otherwise. | ||
| * Conversion functions are only written for "complex" shapes: | ||
| * - StructureShapes ("complex" because StructureShapes can be recursive) | ||
| * - except for non-AWS SDK StructureShapes with ErrorTrait; these aren't "complex" |
There was a problem hiding this comment.
This won't be true forever as we should lift the smithy-dafny restriction that errors can only have a single string "message" member
There was a problem hiding this comment.
Sure -- I think that's a "cross that bridge when we come to it" situation
Issue #, if available:
Description of changes:
OrphanedShapes TestModel + some missing Python conversions.
The bar I'm aiming for is "orphaned shapes are supported if an extern can convert an orphaned shape to/from Dafny".
This can't be tested via local services because passing a shape in a service's operations makes that shape no longer orphaned.
Externs are a reasonable way to test between native/Dafny code, and shapes passed via extern operations can be orphaned.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.