Skip to content

[Core xml/client] Port PR #12065 Support overriding xml parser char key#12418

Merged
jeremymeng merged 6 commits into
Azure:masterfrom
jeremymeng:port-xmlcharkey-pr
Nov 20, 2020
Merged

[Core xml/client] Port PR #12065 Support overriding xml parser char key#12418
jeremymeng merged 6 commits into
Azure:masterfrom
jeremymeng:port-xmlcharkey-pr

Conversation

@jeremymeng
Copy link
Copy Markdown
Member

Currently our xml parser uses hard-coded _ as key to access parsed
xml element content. This causes issue like storage customer getting
errors listing blobs when they use _ as Blob metadata key.

This PR adds support to allow customizing the xml parser char key via
options. Currently xmlCharKey option is supported, with room for
other xml parser options (e.g., attr key for accessing parsed xml
attributes).

  • Introduced XmlOptions which is the same as SerializerOptions
    currently but gives better separation of concerns.

  • Fix two missing cases of replacement of "$" with XML_ATTRKEY in
    core-http. We don't support customizing this yet so functionality
    remains unchanged.

  • Fixed typos

Resolves #12313

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes themselves all look fantastic, great job on this!

The only issue I have is that one of the requirements for corev2 is that we don't drag in xml2js for clients that don't need xml support. This means we can't have a runtime dependency from core-https to core-xml.

So we may need to duplicate XmlOptions, XML_ATTRKEY, and XML_CHARKEY or find some other way to share the source file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm this is tricky, because one of the design requirements is that core-client doesn't have a runtime dependency on core-xml but this isn't a type-only import because of XML_CHARKEY

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right! Maybe duplicating would work.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to duplicate since this is small amount of code, and core-xml only have two dependencies: tslib and xml2js. It doesn't feel worth it to add a common dependency or shared source.

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for doing this.

I left one last suggestion that is up to you if you want to take or not. 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth eagerly creating this interface if it's empty for now? Should we just reuse XmlOptions directly until the Serializer gets a custom option?

Alternatively, should we nest this like:

interface SerializerOptions {
   xml: XmlOptions;
}

So that options specific to a format are split apart? This might help if we ever want to separate XML and JSON serialization.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it worth eagerly creating this interface if it's empty for now? Should we just reuse XmlOptions directly until the Serializer gets a custom option?

I was trying to avoid future breaking changes and it feels right to have xml options for core-xml, and serializer options in serailzer.

Alternatively, should we nest this like:

I debated between nesting and extending too. Yes nesting sounds better. I will do that.

@jeremymeng jeremymeng added this to the MQ-2020 milestone Nov 17, 2020
…har key

Currently our xml parser uses hard-coded `_` as key to access parsed
xml element content. This causes issue like storage customer getting
errors listing blobs when they use `_` as Blob metadata key.

This PR adds support to allow customizing the xml parser char key via
options. Currently `xmlCharKey` option is supported, with room for
other xml parser options (e.g., attr key for accessing parsed xml
attributes).

- Introduced `XmlOptions` which is the same as `SerializerOptions`
currently but gives better separation of concerns.

- Fix two missing cases of replacement of "$" with XML_ATTRKEY in
core-http. We don't support customizing this yet so functionality
remains unchanged.

- Fixed typos

- Remove run time dependency on core-xml by duplicating XML_ATTRKEY, XML_CHARKEY, and XmlOptions in core-client.

- Address CR feedback: nest XmlOptions in SerializerOptions
@jeremymeng jeremymeng merged commit 54f8879 into Azure:master Nov 20, 2020
@jeremymeng jeremymeng deleted the port-xmlcharkey-pr branch November 20, 2020 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[core-xml,core-client] Port PR 12065 from core-http

2 participants