[Core-client] BUG: getRequestUrl method adds the path twice #21595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JonathanCrd
requested review from
xirzec,
jeremymeng and
deyaaeldeen
as code owners
ghost
added
the
|
API change check for API changes are not detected in this pull request for |
|
This PR is no longer needed, after having a discussion with @xirzec and the rest of the team we concluded that Core shouldn't handle of these particular scenarios. Regarding the error faced in the KeyVault package, that is fixed by using one of the generated methods with the keyword "Next" in them. The error was caused because the pagination logic is on the custom layer, and it's not being generated, so the methods that passes a continuation token should be modified manually (this was very opaque when debugging). So, for the record, next time facing an issue similar to this, make sure to modify the method in which the continuation token is being passed, something like this: |
azure-sdk
pushed a commit
to azure-sdk/azure-sdk-for-js
that referenced
this pull request
Feb 14, 2023
Release apimangement 2022-08 01 (Azure#22313) * Release api management 2022 08 01 (Azure#21169) * add base for 2022-08-01 * updates readme.md * update api-version and examples * resolve Go SDK breaking change * update examples to fix model definition Co-authored-by: Chenjie Shi <tadelesh.shi@live.cn> * Adding Resolver Entities to documentation (Azure#21352) * merging in resolver entities * adding resolver updates * fixed policy examples that were missing a policyId param * fixed typo that added a nested properties prop * fixed description for API Resolvers List, filter query * fixed error in definitions for resolvers * fixed example that had an incorrect response definition * added missing json file references * fix for linting errors * ref fixes and undoing bad merge overwrites * fix typo * wiki for apis and products (Azure#21595) * wiki for apis and products * prettier; fixed spellchecks * Fixed spelling * Updated path Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com> * code review changes; gihub checks fixes * fixed conact name and paths * added properties back * added contract properties objects * fixed lint * changed example to match the definition * prettier * code review changes * added paths and examples for wiki collections * Added x-ms-pageable * removed count Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com> * Add ConfirmConsentCode to APIM RP (Azure#22418) * Update apimauthorizationproviders.json Add ConfirmConsentCode endpoint * Update definitions.json * Create ApiManagementPostAuthorizationConfirmConsentCodeRequest.json * Update definitions.json Remove count property * Update apimauthorizationproviders.json * [2022-08-01] Fix Linter Errors (Azure#22537) * linter errors * prettier * v2 * fix error response duplicate schema * type object * prettier * sync data from current ga version * remove duplicate parameter * type object --------- Co-authored-by: Chenjie Shi <tadelesh.shi@live.cn> Co-authored-by: hoonality <92482069+hoonality@users.noreply.github.com> Co-authored-by: malincrist <92857141+malincrist@users.noreply.github.com> Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com> Co-authored-by: Logan Zipkes <44794089+LFZ96@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Packages impacted by this PR
Issues associated with this PR
None
Describe the problem that is addressed by this PR
After migrating Keyvault Secrets to Core V2 (Issue #15896, PR is not out yet), two tests were no longer passing:
In a debug session with @sadasant, by looking at the logs and the URLs of the requests, we discovered that an extra "/deletedSecrets" path in the request URL, so the server was throwing us an error. We dug in the KeyVault code to see where the URL was being modify and we found out that Core was the one modifying the URL.
For reference, the test did 2 consecutive requests, the first one looked like:
and the second one looked like:
I have made a change in this PR which solves this issue for the two tests, and also added a test case for it.
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?
This solution check if the path is already part of the URL and prevent adding it in a
elseblock.I'm open to discuss other ideas on how to fix this, or if someone else knows a better approach for this bug.
Are there test cases added in this PR? (If not, why?)
Yes, a test case was added for
getRequestUrl, providing an URL with the specifiedpathoption already in it.Checklists
CC: @sadasant @sarangan12