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.

Sign up for GitHub

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

Jump to bottom

[core-http] use # as the charkey in xml parsers #9599

Closed

jeremymeng wants to merge 3 commits into Azure:master from jeremymeng:corehttp-xml2js-charkey

Contributor

jeremymeng commented Jun 18, 2020 •

edited

Loading

We were using '_' as charkey in xml2js parser settings. It is the key used to
access the value content of xml elements. This caused issues like
#9171 where '_' cannot be used as a blob metadata key. The fix is
switching to use '#' as the charkey. '#' is an invalid character for
blob metadata key names and for identifer names in most programming
languages so it should be safe to use.

Consumers of our libraries are not impacted as this key is only used internally in core-http/Service Bus.

Contributor Author

jeremymeng commented Jun 18, 2020

/azp run js - storage-blob - tests

azure-pipelines bot commented Jun 18, 2020

Azure Pipelines successfully started running 1 pipeline(s).

Contributor Author

jeremymeng commented Jun 18, 2020

/azp run js - storage-file-share - tests

Contributor Author

jeremymeng commented Jun 18, 2020

/azp run js - storage-queue - tests

azure-pipelines bot commented Jun 18, 2020

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

azure-pipelines bot commented Jun 18, 2020

Azure Pipelines successfully started running 1 pipeline(s).

Contributor Author

jeremymeng commented Jun 18, 2020

/azp run js - servicebus - tests

azure-pipelines bot commented Jun 18, 2020

Azure Pipelines successfully started running 1 pipeline(s).

Contributor Author

jeremymeng commented Jun 18, 2020

/azp run js - storage-blob - tests

azure-pipelines bot commented Jun 18, 2020

Azure Pipelines successfully started running 1 pipeline(s).

Contributor Author

jeremymeng commented Jun 22, 2020

/azp run js - servicebus - tests

azure-pipelines bot commented Jun 22, 2020

Azure Pipelines successfully started running 1 pipeline(s).

ramya-rao-a added the Azure.Core label

Contributor Author

jeremymeng commented Jun 22, 2020

Technically this is a breaking change in core-http, although I don't think there are any libraries except storage and service bus depending on xml parser.

jeremymeng marked this pull request as ready for review

June 22, 2020 07:20

jeremymeng requested review from bterlson, chradek, daviwil, HarshaNalluru, ramya-rao-a, richardpark-msft and xirzec as code owners

June 22, 2020 07:20

xirzec reviewed

View reviewed changes

sdk/core/core-http/src/util/xml.common.ts

+              /**
+               * Key that is used to access the XML value content.
+               */
+              export const XML_CHARKEY = "#";

Member

xirzec Jun 22, 2020

nit: should have newline at EOF

xirzec reviewed

View reviewed changes

Member

xirzec left a comment

Provisionally, I'm fine with this change, but I'm curious if/how it will affect consumer code.

Does it mean someone will have to change the property name they're accessing in their code?

Contributor Author

jeremymeng commented Jun 22, 2020

but I'm curious if/how it will affect consumer code.

consumer of our libraries or consumer of core-http? It shouldn't affect the former. For latter if there are any other direct consumers (I doubt) than storage/service bus, they would need changes similar to those storage/SB in this PR.

Member

xirzec commented Jun 22, 2020

but I'm curious if/how it will affect consumer code.

consumer of our libraries or consumer of core-http? It shouldn't affect the former. For latter if there are any other direct consumers (I doubt) than storage/service bus, they would need changes similar to those storage/SB in this PR.

But if Storage/SB need updates to work with this version of core, would it mean a major version bump of core-http so that folks on older storage/SB don't get broken?

Contributor Author

jeremymeng commented Jun 22, 2020

But if Storage/SB need updates to work with this version of core, would it mean a major version bump of core-http so that folks on older storage/SB don't get broken?

My understanding is it's internal in the core-http SB related to how xml is parsed to JSON. '$' would be the JSON property key referring to xml element attribute and '_' the key of element value initially then they were deserialized to object that don't have these keys.

Contributor Author

jeremymeng commented Jun 23, 2020

I will do some testing. There might be scenario in storage where previously '#' was passing in via some parameters.

jeremymeng mentioned this pull request

Cannot list blobs if any of the blobs have a metadata whose key is "_" #9171

Closed

6 tasks

jeremymeng mentioned this pull request

[Core-http] Support xml namespaces #11201

Merged

jeremymeng modified the milestones: [2020] December, Backlog

jeremymeng and others added 3 commits

October 16, 2020 11:07


          [core-http] use # as the charkey in xml parsers

3980a89

We were using '_' as charkey, which is the prrefix that is used to
access the character content of xml elements.  It caused issues like
switch to use '#' as the charkey.  '#' is an invalid character for
blob metadata key names so it is safe to use.


          Update Service Bus to use the new XML_CHARKEY

c10c172


          Update code after rebasing onto latest master

c271876

jeremymeng force-pushed the corehttp-xml2js-charkey branch from afb9d66 to c271876 Compare

October 16, 2020 20:52

Contributor Author

jeremymeng commented Oct 16, 2020

Yes it's a breaking change. Older versions of Service Bus won't work after this. Storage should be fine as they don't use the CHARKEY/ATTRKEY directly. Are we ready for major bump?

Member

xirzec commented Oct 16, 2020

If it's a breaking change, should we wait on this and make it part of core-xml instead? What's the urgency of this fix?

Contributor Author

jeremymeng commented Oct 16, 2020

If it's a breaking change, should we wait on this and make it part of core-xml instead? What's the urgency of this fix?

Yes I think it's better to do this in core-xml. The issue is that storage explorer/storage cannot list blobs whose metadata key-value pair has _ as the key. One workaround is not using _ as the metadata key. I asked the customer earlier whether the workaround is acceptable for now.

Contributor Author

jeremymeng commented Oct 19, 2020

@xirzec I've been thinking about allowing Storage to customize CHARKEY in the options to stringifyXML() and parseXML(). So SB don't need to change; older version of storage still use _ and has the bug, newer versions of storage customize CHARKEY to #. What do you think?

Member

xirzec commented Oct 19, 2020

@xirzec I've been thinking about allowing Storage to customize CHARKEY in the options to stringifyXML() and parseXML(). So SB don't need to change; older version of storage still use _ and has the bug, newer versions of storage customize CHARKEY to #. What do you think?

That sounds like a great way to avoid breaking backwards compatibility. I do a similar thing in core-client where stringifyXML() and parseXML() is passed in rather than being hardcoded.

Contributor Author

jeremymeng commented Oct 20, 2020 •

edited

Loading

That sounds like a great way to avoid breaking backwards compatibility. I do a similar thing in core-client where stringifyXML() and parseXML() is passed in rather than being hardcoded.

I tried two approaches

passing option to set xml char key all the way. It seems very involved, and it's not done yet: WebResource.prepare() and ServiceClient.sendOperationRequest() also call Serializer.serialize() and I don't see an obvious way to pass the xml char key to them from Storage code.
add getXmlCharKey() and setXmlCharKey() similar to get and set log level. However, this won't work if we have older version of SB library and newer version of Storage library which sets xml char key to "#" in a same application.

Member

xirzec commented Oct 20, 2020

I'm not sure if WebResource.prepare matters, since as far as I can tell, that's a legacy usage of sendRequest that track2 doesn't leverage. SendOperationRequest always builds up a WebResource before calling sendRequest.

Contributor Author

jeremymeng commented Oct 22, 2020

@xirzec sendOperationRequest() is called from generated code, it uses serializer to build request parameters and body.

Member

xirzec commented Oct 23, 2020

Yes, my point was that sendOperationRequest doesn't rely on WebResource.prepare.

For sendOperationRequest itself we can pass the XML options, since the options are passed to ServiceClient which is where sendOperationRequest lives.

Am I missing something here?

Contributor Author

jeremymeng commented Oct 23, 2020

You are right. I can use RequestOptionsBase. I was thinking about adding an extra optional parameter to sendOperationRequest().

Contributor Author

jeremymeng commented Oct 26, 2020

Closing this. Will create another PR of passing xmlCharKey option.

jeremymeng closed this

xirzec removed this from the Backlog milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

xirzec xirzec left review comments

bterlson Awaiting requested review from bterlson

chradek Awaiting requested review from chradek

daviwil Awaiting requested review from daviwil

HarshaNalluru Awaiting requested review from HarshaNalluru

ramya-rao-a Awaiting requested review from ramya-rao-a

richardpark-msft Awaiting requested review from richardpark-msft

Labels