xuri / xgen

XSD (XML Schema Definition) parser and Go/C/Java/Rust/TypeScript code generator
BSD 3-Clause "New" or "Revised" License
339 stars 80 forks source link

Fix for #28: Attribute Enumeration Parsing Aborted Prematurely When Enclosed in ComplexType #34

Closed alexandre-normand closed 3 years ago

alexandre-normand commented 3 years ago

PR Details

This fixes the parsing of attributes defined with enumerations and enclosed in complex types. Before the change proposed here, attributes of a complex type that were defined with enumerations would render without a type (in go, that meant interface{}) and other attributes in that enclosing complex types would be missing. Issue #28 has an example of this and this PR includes a new type that is an example of this.

Description

This is my first contribution so I'm just getting familiar with the code base but it looks like the problem was that the attribute was prematurely added to the enclosing complex type in the attribute start instead of in the end. This had the side-effect of clearing the attribute from the stack and improperly including the restriction and enumeration data enclosed in the attribute when present.

To fix this, I moved that logic to add the attribute to the parent type in the EndAttribute. For the example added to base64.xsd, this looks correct.

Related Issue

https://github.com/xuri/xgen/issues/28

Motivation and Context

This fixes one issue I was having with generating go structs from a somewhat complex (but valid) XSD. The main problem was that a lot of fields were missing in types that included enumerations but also, as stated earlier, the type for the attribute was incorrectly erased and generated as interface{}.

How Has This Been Tested

I ran the test on the modified base64.xsd and validated that it looks correct. I then updated the expected generated code to ensure the tests would pass.

Note that some expected results for tests in https://github.com/xuri/xsd need to be updated because they were affected by the bug and had incorrect expectations that need to be changed:

Question: I didn't see anything in the contribution guidelines regarding tests in https://github.com/xuri/xsd for which the expected output needed to be changed as part of a bugfix. Should I open a parallel PR in https://github.com/xuri/xsd to update those?

Types of changes

Checklist

alexandre-normand commented 3 years ago

@xuri : I might have some time in the coming days to invest in this PR and maybe start to review what I had been thinking for https://github.com/xuri/xgen/pull/18. Any chance you can look at this PR soon and let me know if I'm on the right track (including the general contribution process)?

I'm also wondering if you have thoughts regarding the question above regarding the tests in https://github.com/xuri/xsd that will need to have their expected output updated.

Thanks!

xuri commented 3 years ago

LGTM, thanks for your PR. Yep, the tests in https://github.com/xuri/xsd should be updated, I'll certainly accept that if you create PR for that.

alexandre-normand commented 3 years ago

LGTM, thanks for your PR. Yep, the tests in https://github.com/xuri/xsd should be updated, I'll certainly accept that if you create PR for that.

Awesome. Thanks for the review. I'll create the PR in https://github.com/xuri/xsd shortly.

alexandre-normand commented 3 years ago

Here's the xsd PR to fix the expected outputs.