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 Support for XSD Extensions #36

Merged

Conversation

alexandre-normand
Copy link
Contributor

@alexandre-normand alexandre-normand commented Nov 19, 2021

PR Details

This builds on the work from @BatmanAoD in #18 (thanks again for doing all that work!) but also considers the case where an extension is used to extend another type. I was motivated by having this very case in one of the XSD I'm working with but I also wanted to support the case described in issue #7.

Description

I can't say this PR is complete because I'm not sure how to implement the feature in the C version. Are there any references/links on typical libs that C programmers would use to generate XML from their structs? I've implemented the feature in the other languages (Typescript, Java, Rust and Go) with my main interest/expertise being in the Go one but I could use guidance to complete the work on the C variant.

Some details regarding the case of extending a complex type:

  • Go: The generated code uses struct embedding. the base64 example has been modified to include both supported extension cases and the tests were modified to show the new expected output.
  • Rust: Children types extending a parent type use flattening. There's no concept of embedding as far as I could tell (I don't know much about Rust) but the flattening should work fine to serialize/deserialize correctly even if there's an extra level in the code to access "inherited" fields.
  • Java: Uses classic inheritance.
  • TypeScript: Similar as the Java version, it uses inheritance.

To make development easier, I updated the parser tests to assert on the content of the expected output rather than just the length. This made it easier to view the differences and I actually started with what the desired expected output should be before the implementation.

Related Issue

This fixes issue #7 although it covers the other common case for extensions (extending other complex types).

Motivation and Context

Currently, XSD extensions aren't supported. I'm not sure how generally commonly used they are but I have one use case with the Microsoft ShowPlan XSD that uses extensions heavily to define elements and attributes in base types that are then extended in children types.

How Has This Been Tested

Added to the base64.xsd example to showcase the simple cases and ran through a more complex real-world example where I used the generated go structs and then parsed some real XML for that matching xsd.

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.

@xuri xuri added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 19, 2021
@alexandre-normand
Copy link
Contributor Author

@xuri : I did a few more things here that I've kept as separate commits because I could see the argument for spinning those off into their own PRs. Let me go through those things:

  • I refactored the parser_test so that there wouldn't be as much code duplication between the different languages.
  • The refactoring enabled running both an internal set of parser tests (the ones in test) as well as external (the ones found in data and copied by a developer there).
  • I introduced subtests so that each XSD going through the parser test would show up nicely with its relative file path. This makes it easier to identify which XSD is failing and then look at the expected vs actual generated code to either fix the code or update the fixture.
  • I introduced xml_test.go which uses the expected go output from test along with xml fixture files in xmlFixtures to validate that the generated code is correctly marshaling and unmarshaling. It does this by running unmarshal on the input fixture file and the remarshaling to confirm that the input fixture and the remarshaled content are the same.
  • Updated the testing section of the contribution guidelines according to ☝️ .

Let me know if you'd like me to create a separate PR for those testing improvements (at least, they seem like improvements to me?) before I move on to the rest of this PR.

Otherwise, back to the main goal of this PR, I'd like some guidance on the missing changes to the C generator. I've done C a long time ago but I don't think I've ever done XML marshaling/unmarshaling in C so I'm a bit lost as to how those generated C structs can be used. Is there a common XML library that can marshal structs into XML? I'm just wondering where to look to find out how the generated C code would end up being used by developers using xgen to generate C. I looked at libxml2 as it came up as a popular one but didn't seem to find how it would use structs for parsing/encoding.

Thanks!

@xuri xuri added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2021
@xuri
Copy link
Owner

xuri commented Nov 21, 2021

Thanks for your PR, I was motivated by generated C code which can be used for some custom parser based on libxml2, or others ASN.1 module. I'll accept this change, could you squash the commits and refine the commit message, you can copy the description of the PR to the squashed commit in this case.

Adds support for two cases of xsd extensions:
- Extending a built-in type
- Extending a complex type

The supported languages for the extension support are Java, Typescript, Rust and Go.
C is not yet supported.

Details on each implentation:

* Go: The generated code uses struct embedding. the base64 example has been modified to include both supported extension cases and the tests were modified to show the new expected output.
* Rust: Children types extending a parent type declare a field of that parent type with serde(flatten).
* Java: Uses classic inheritance.
* TypeScript: Similar as the Java version, it uses inheritance.

To make development easier, parser tests were updated to assert on the content of the expected output rather than just the length. This made it easier to view the differences and I actually started with what the desired expected output should be before the implementation. In order to make the tests stable, the generation was updated to sort members and make that unstable part of the generation now be stable.

Fixes issue xuri#7.

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

Thanks for your PR, I was motivated by generated C code which can be used for some custom parser based on libxml2, or others ASN.1 module. I'll accept this change, could you squash the commits and refine the commit message, you can copy the description of the PR to the squashed commit in this case.

Thanks for the review, @xuri ! I squashed all commits and included the summary of each language's implementation in that commit. I also submitted a PR on the xsd repo to update the tests. The generated code should be stable now. The members were previously unsorted but I applied a sort so we could have stable tests asserting on the content.

alexandre-normand added a commit to alexandre-normand/xsd that referenced this pull request Nov 22, 2021
@xuri xuri merged commit a23283c into xuri:master Nov 22, 2021
@alexandre-normand alexandre-normand deleted the 7-add-support-for-xsd-extensions branch November 22, 2021 17:27
xuri pushed a commit to xuri/xsd that referenced this pull request Nov 23, 2021
@BatmanAoD
Copy link
Contributor

Cool, thank you for carrying this across the finish line!

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

Successfully merging this pull request may close these issues.

3 participants