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

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

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

alexandre-normand
Copy link
Contributor

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

#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:

  • maven.apache.org/fml-1.0.1.xsd
  • fml-1.0.xsd
  • xdoc-2.0.xsd

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

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

…n ComplexType

Fixes issue xuri#28

Signed-off-by: Alex Normand <alexandre.normand@gmail.com>
@alexandre-normand
Copy link
Contributor Author

@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 #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 xuri added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 18, 2021
@xuri xuri merged commit d282178 into xuri:master Nov 18, 2021
@xuri
Copy link
Owner

xuri commented Nov 18, 2021

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 alexandre-normand deleted the 28-fix-enum-attribute-parsing branch November 18, 2021 18:35
@alexandre-normand
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants