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

Fail to set Content-Type in LRO polling url in modular but we have in HLC #2394

Closed
Tracked by #2347
kazrael2119 opened this issue Apr 1, 2024 · 6 comments
Closed
Tracked by #2347
Assignees
Labels
HRLC p0 priority 0

Comments

@kazrael2119
Copy link
Contributor

kazrael2119 commented Apr 1, 2024

Use playback mode to run test case with the sdk generated by modular, the polling url would get failure as

<Content-Type> is absent in request, value <application/json>

this is a issue found in comparing modular and hlc.
In hlc, the test recording is https://github.com/Azure/azure-sdk-assets/blob/js/servicenetworking/arm-servicenetworking_bb0c85674f/js/sdk/servicenetworking/arm-servicenetworking/recordings/node/servicenetworking_test/recording_trafficcontrollerinterface_create_test.json#L67
we can see Content-Type is valid in polling request header

but in modular, only remove content type in polling request header, it will pass
Azure/azure-sdk-for-js@9fb9ecb#diff-b5d30781a8e42df28af130c1d4e1b9a9cdb20ff8030bf1cd76ecb71734ab94a7L67

@joheredi
Copy link
Member

I'm having a bit of a hard time to understand what the issue is here. I think what the issue is describing is that modular is not setting a default content-type in this case.

@joheredi
Copy link
Member

joheredi commented May 22, 2024

Following the categorization checklist I think we should add the default content-type

Breaking Change Categorization
Bug (Unwanted Breaking Changes):

  • Missing data from the spec in the new implementation.
  • Incorrect results produced by the new implementation.
  • Slight differences in the new implementation that do not improve user experience. If uncertain, categorize as unwanted.

Improvements (Acceptable Breaking Changes):

  • Fixes broken behavior in the new implementation.
  • Offers clear enhancements to user experience.
  • Eliminates or discourages bad patterns.
  • Provides additional functionality.
  • Aligns more closely with TypeSpec standards.

@MaryGao MaryGao changed the title The content type of polling is absent in request in the recordings Fail to set Content-Type in LRO polling url in modular but we have in HLC May 23, 2024
@MaryGao
Copy link
Member

MaryGao commented May 23, 2024

@joheredi your understanding and categorization is correct. In modular we didn't set the content type in polling url but we do this in HLC. I believe we should do the same thing in modular also.

@MaryGao
Copy link
Member

MaryGao commented May 27, 2024

@qiaozha @joheredi Could you share your insights here?

TL;DR This is a question for should we set content-type when there is no body?. In HLC we set as application/json but in modular we would not set. Personally I would prefer modular way if no body there just not setting the content-type.

Whole story
This is diff between HLC and modular when the body has no content. For HLC we would prepare the content-type because of sererlization parameter spec. But in modular we would not prepare this parameter if there is no definition in typespec side.

In RFC, it doesn't say explicitly what to do if you're generating a message that doesn't have a payload body.

A sender that generates a message containing content SHOULD generate a Content-Type header field in that message unless the intended media type of the enclosed representation is unknown to the sender.

So both seems workable for me, and personally I would prefer modular way if no body there is no need for this clolumn and we just not set it.

Please note this would only happen when the body is empty, if the body is not empty. Both HLC and modular core would have fallback logic to set the default content type.

@qiaozha
Copy link
Member

qiaozha commented May 28, 2024

I think we should leave it as is. @joheredi what do you think?

@lirenhe
Copy link
Member

lirenhe commented May 31, 2024

Close it as no further action needed.

@lirenhe lirenhe closed this as completed May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HRLC p0 priority 0
Projects
None yet
Development

No branches or pull requests

5 participants