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

add restXml tests for flattened list of structures #781

Merged
merged 1 commit into from
May 6, 2021

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Apr 23, 2021

Description of changes:

  • Adds a test for ignoring the root element name since some services like S3 do not send a root element that matches the structure shape id from the model
  • expanded the xml list test to include a flattened list of structures (we had an issue in Kotlin where the deserializer state got messed up by this case, maybe it helps someone else)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aajtodd
Copy link
Contributor Author

aajtodd commented Apr 26, 2021

It's not clear if the IgnoresRootElementName belongs with the general restXml protocol tests suite or should be made specific to S3 customization test suite (or if the model should just be fixed but that's a longer resolution)

@srchase
Copy link
Contributor

srchase commented May 3, 2021

#789 was added which addresses the root element name, namely that it should not be ignored. Instead, models should properly set the root element with an xmlName trait if needed.

Could you remove the root element part of this in PR?

@aajtodd
Copy link
Contributor Author

aajtodd commented May 4, 2021

#789 was added which addresses the root element name, namely that it should not be ignored.

The test there is a nice addition but that doesn't address the root issue which was that the model is wrong. I'm assuming though that what you're saying is that the model(s) will be updated appropriately to include missing xmlName trait?

I've removed the test for ignoring the name, only the flattened list of structures remains.

@aajtodd aajtodd changed the title add restXml tests for ignoring root element and flattened structure lists add restXml tests for flattened list of structures May 4, 2021
@JordonPhillips
Copy link
Contributor

I'm assuming though that what you're saying is that the model(s) will be updated appropriately to include missing xmlName trait?

Yes

@srchase srchase merged commit 0e128fe into smithy-lang:main May 6, 2021
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