[core-http] Support overriding xml parser char key via options#12065
Conversation
jeremymeng
requested review from
bterlson,
chradek,
daviwil and
xirzec
as code owners
ghost
added
the
There was a problem hiding this comment.
I feel it is useful to export the default value, since we now support overriding it.
|
What storage need to do after this change: |
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 add 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).
jeremymeng
force-pushed
the
passing-xmlcharkey
branch
from
0be1515 to
efc62ae
Compare
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
I really like the approach here; my comments are all code style/cleanup ones.
Can we also apply this change to core-xml and friends?
| (deserializationContentTypes && deserializationContentTypes.json) || defaultJsonContentTypes; | ||
| this.xmlContentTypes = | ||
| (deserializationContentTypes && deserializationContentTypes.xml) || defaultXmlContentTypes; | ||
| this.xmlCharKey = parsingOptions?.xmlCharKey || XML_CHARKEY; |
There was a problem hiding this comment.
might as well be consistently fancy, right? ;)
| this.xmlCharKey = parsingOptions?.xmlCharKey || XML_CHARKEY; | |
| this.xmlCharKey = parsingOptions?.xmlCharKey ?? XML_CHARKEY; |
| constructor( | ||
| nextPolicy: RequestPolicy, | ||
| deserializationContentTypes: DeserializationContentTypes | undefined, | ||
| parsingOptions: { xmlCharKey?: string } | undefined, |
There was a problem hiding this comment.
rather than putting a literal { xmlCharKey?: string } everywhere, can we write this down as a type? It'll make it much easier to add additional parsingOptions later if needed.
|
|
||
| constructor( | ||
| nextPolicy: RequestPolicy, | ||
| deserializationContentTypes: DeserializationContentTypes | undefined, |
There was a problem hiding this comment.
is there a reason these are all | undefined instead of making the parameters themselves optional with ??
There was a problem hiding this comment.
The last parameter is non-optional. I could move both parameters to be after request policy options
| xmlContentTypes: string[], | ||
| operationResponse: HttpOperationResponse | ||
| operationResponse: HttpOperationResponse, | ||
| opts?: { includeRoot?: boolean; xmlCharKey?: string } |
There was a problem hiding this comment.
again I wonder if this should be an interface
| responseBody = responseBody["_"]; | ||
| if ( | ||
| responseBody[XML_ATTRKEY] != undefined && | ||
| responseBody[options?.xmlCharKey ?? XML_CHARKEY] != undefined |
There was a problem hiding this comment.
feels like we should assign the result of options?.xmlCharKey ?? XML_CHARKEY instead of calculating it twice. Why not just assign the final char key at the top of the method?
| objectName: string, | ||
| isXml: boolean | ||
| isXml: boolean, | ||
| options?: { xmlCharKey?: string } |
There was a problem hiding this comment.
feels like this private method shouldn't have xmlCharKey be optional, since the caller already knows the final value
There was a problem hiding this comment.
RIGHT! Making it required for private methods reveals some places where I missed passing the options
There was a problem hiding this comment.
oh, you meant requiring xmlCharKey. I made options required.
After I replaced { xmlCharKey?: string } with an interface, I would need some type mapping to make this property required, and assert it when calling I think.
There was a problem hiding this comment.
Made internal options Required<> as @xirzec suggested. I also rename the interface name to more generic SerializerOptions.
| propertyMapper: Mapper, | ||
| serializedValue: any, | ||
| isXml: boolean, | ||
| options?: { xmlCharKey?: string } |
There was a problem hiding this comment.
same thought, why make this optional everywhere? We should make it optional where it comes in from a public interface, but required everywhere else so we don't have default logic sprinkled everywhere.
| urlParameterValue, | ||
| getPathStringFromParameter(urlParameter) | ||
| getPathStringFromParameter(urlParameter), | ||
| { xmlCharKey: operationArguments.options?.parsingOptions?.xmlCharKey } |
There was a problem hiding this comment.
can we create this object once earlier in the method and pass it by reference in each place?
| parameterMapper | ||
| ); | ||
| serializer.serialize(parameterMapper, value, parameterPathString); | ||
| serializer.serialize(parameterMapper, value, parameterPathString, { |
There was a problem hiding this comment.
same thing, let's shorthand the last argument
- introduce XmlOptions interface - Make xml options required for private methods - Remove duplicate by introducing locals
jeremymeng
force-pushed
the
passing-xmlcharkey
branch
from
f6e9d69 to
f710448
Compare
Sure, I may do it in another PR as I am trying to ship core-http tomorrow. |
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
LGTM.
I'm fine shipping this for this release, but if you wanted it to bake longer for the next release I wouldn't be opposed.
|
|
||
| - Explicitly set `manual` redirect handling for node fetch. And fixing redirectPipeline [PR 11863](https://github.com/Azure/azure-sdk-for-js/pull/11863) | ||
| - Add support for multiple error response codes.[PR 11841](https://github.com/Azure/azure-sdk-for-js/) | ||
| - Add support for multiple error response codes. [PR 11841](https://github.com/Azure/azure-sdk-for-js/) |
There was a problem hiding this comment.
these notes should be in a newer section right or we need to fix the release date for 1.2.0 above, right?
| mergeAttrs: false, | ||
| explicitRoot: true, | ||
| validator: null, | ||
| validator: undefined, |
There was a problem hiding this comment.
if we're setting these to undefined, should we instead omit the keys entirely? Or would that do something different?
There was a problem hiding this comment.
This is unrelated to the fix, but I added the type xml2js.OptionsV2 and undefined are correct according to the type definitions. I believe this is a copy of all the supported xml2js settings so I will keep them listed here even we don't use any of them directly.
|
/azp run js - storage-blob - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - storage-file-share - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - storage-queue - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - storage-file-datalake - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Service Bus and Storage live tests are passing. I will merge this so we will have nightly test results of other libraries tomorrow for more confidence. |
FYI: I believe service-bus is unaffected by these changes since it isn't using the serializers from core-http. This would change once service-bus starts using the swagger and codegen instead of handwritten ATOM API. |
|
atomXmlHelper uses parseXML and stringifyXML, which are touched in this PR |
|
Oh, thanks! |
…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
…ey (#12418) 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. - 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
4 participants
Currently our xml parser uses hard-coded
_as key to access parsedxml 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
xmlCharKeyoption is supported, with room forother options (e.g., attr key for accessing parsed xml attributes).