-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[core-lro] Support path rewriting #33363
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
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for URL rewriting for long-running operations by introducing a baseUrl option. Key changes include:
- Introducing the rewriteUrl utility to handle URL rewriting.
- Updating the testing suite to validate rewritten polling and resource URLs.
- Adding a baseUrl option to the poller and LRO API configuration.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
sdk/core/core-lro/src/http/utils.ts | Implements and exports the core URL rewriting functionality. |
sdk/core/core-lro/test/lro.spec.ts | Adds tests to verify that polling URLs are rewritten when a baseUrl is provided. |
sdk/core/core-lro/CHANGELOG.md | Updates changelog to reflect the new feature. |
sdk/core/core-lro/test/utils/router.ts | Updates type definitions and refactors test poller utility functions. |
sdk/core/core-lro/src/http/models.ts | Adds the baseUrl field to the CreateHttpPollerOptions interface. |
sdk/core/core-lro/review/core-lro.api.md | Updates the API definition to include the baseUrl option. |
sdk/core/core-lro/src/http/poller.ts | Integrates URL rewriting into the poller creation logic. |
sdk/core/core-lro/test/utils/utils.ts | Updates type annotations for operation state in tests. |
sdk/core/core-lro/src/http/operation.ts | Adds explicit type annotation to the nested getDefault function. |
Comments suppressed due to low confidence (1)
sdk/core/core-lro/test/lro.spec.ts:2956
- Consider adding additional test cases to cover scenarios where the input URL is relative to ensure that rewriteUrl correctly handles both absolute and relative URL inputs.
it("polling URL is rewritten if the baseUrl option is provided ", async function () {
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! If really needed in the future, we could still expand it to take a string or a callback returning string, without breaking customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small remarks, thanks for enhancement!
Packages impacted by this PR
@azure/core-lro
Issues associated with this PR
Addresses #33321
Describe the problem that is addressed by this PR
LRO operations return specific URLs in the initial response that should be used to polling the operation and to also retrieve the resource being created, if any. These URLs are currently being treated opaquely, however, if the LRO operation lives behind some API gateway, the polling and resource location URLs won't be updated to point to the gateway instead.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
I am adding a
baseUrl
option where the caller can specify if they want to rewrite the these URLs to use this base URL. I think this approach strikes a good balance between complexity and keeping the user in control. Alternatively, there are a few ideas I considered:RunningOperation.sendPollRequest
to handle such customization.sendPollRequest
handle the path rewriting, however, if we want to support this scenario across our libraries, the rewriting logic will need to be centralized somewhere else and the libraries will have to import from two different places.Are there test cases added in this PR? (If not, why?)
Yes!
Provide a list of related PRs (if any)
N/A
Checklists