search

johnstonskj / rust-atelier

Rust native core model for the AWS Smithy IDL
MIT License
76 stars 10 forks source link

Fix json loading when struct members have ShapeID instead of just Identifier #30

Closed stevelr closed 3 years ago

stevelr commented 3 years ago

This might be a fix for issue #28

When converting smithy models to json, structure members are written with the full ShapeID: namespace + shape (the structure name) + member (field name), but the json reader expected an Identifier (field name only). To avoid breaking the reader for models that might have loaded previously, I modified the reader to accept the member id in either format - ShapeID or Identifier. This seems to fix issue #28 and it works for the example given in the bug report and a couple other models I tried.

stevelr commented 3 years ago

Per the JSON-AST spec,

All shape IDs in the JSON AST MUST be absolute shape IDs that contain a namespace

so I removed the conditional logic that accepted Identifier for member_id, and now it generates an error if the member_id is not a ShapeID

johnstonskj commented 3 years ago

Thanks Steve, unfortunately I have sporty access this week (vacation), I'll be able to dig in more when I return. Can you also take a look at adding regression tests too?

Thanks

On Tue, Jun 15, 2021, 5:27 PM stevelr @.***> wrote:

Per the JSON-AST spec,

All shape IDs in the JSON AST MUST be absolute shape IDs that contain a namespace

so I removed the conditional logic that accepted Identifier for member_id, and now it generates an error if the member_id is not a ShapeID

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/johnstonskj/rust-atelier/pull/30#issuecomment-861898661, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIGRRYFNFENH7HF2GK4SVDTS7OXTANCNFSM46YE2R5Q .

stevelr commented 3 years ago

@johnstonskj The files in atelier-json/tests/good/aws.api.json serve as a regression test for the the requirement that structure members have absolute shapeId. I fixed the test json files so they are all passing.

in this PR I included a fix for #31 also, and added a test case. There had been several test cases built with saved "*lines" output that captured the incorrect output due to bug #31. So fixing the bug meant fixing all the sample output files as well. Those are fixed now and all tests pass.

johnstonskj commented 3 years ago

Not required.