Skip to content

Commit

Permalink
fix-url-encoding-in-api-version-policy (Azure#24875)
Browse files Browse the repository at this point in the history
### Packages impacted by this PR
@azure-rest/core-client

### Issues associated with this PR


### Describe the problem that is addressed by this PR
In the apiVersionPolicy, we use url.toString() to get the final url,
which will encode everything and ruin our effort in buildRequestUrl even
if we have passed skipUrlEncoding.

### 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?


### Are there test cases added in this PR? _(If not, why?)_


### Provide a list of related PRs _(if any)_


### Command used to generate this PR:**_(Applicable only to SDK release
request PRs)_

### Checklists
- [x] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [x] Added a changelog (if necessary)
  • Loading branch information
qiaozha authored Apr 6, 2023
1 parent 09988fe commit 2938135
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 8 deletions.
6 changes: 1 addition & 5 deletions sdk/core/core-client-rest/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,9 @@

## 1.1.2 (Unreleased)

### Features Added

### Breaking Changes

### Bugs Fixed

### Other Changes
- fix unexpected url encoding when apiVersionPolicy applies and even if we have passed the skipUrlEncoding as true in the request.

## 1.1.1 (2023-03-02)

Expand Down
6 changes: 3 additions & 3 deletions sdk/core/core-client-rest/src/apiVersionPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ export function apiVersionPolicy(options: ClientOptions): PipelinePolicy {
// Append one if there is no apiVesion and we have one at client options
const url = new URL(req.url);
if (!url.searchParams.get("api-version") && options.apiVersion) {
// append the apiVersion with client one
url.searchParams.append("api-version", options.apiVersion);
req.url = url.toString();
req.url = `${req.url}${
Array.from(url.searchParams.keys()).length > 0 ? "&" : "?"
}api-version=${options.apiVersion}`;
}

return next(req);
Expand Down
81 changes: 81 additions & 0 deletions sdk/core/core-client-rest/test/getClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,87 @@ describe("getClient", () => {
// Define the apiVersion in url
await client.pathUnchecked(`/foo?api-version=${operationApiVersion}`).get();
});

it("should not encode url when skip query parameter encoding and api version parameter exists", async () => {
const defaultHttpClient = getCachedDefaultHttpsClient();
sinon.stub(defaultHttpClient, "sendRequest").callsFake(async (req) => {
return {
headers: createHttpHeaders(),
status: 200,
request: req,
} as PipelineResponse;
});

const apiVersion = "2021-11-18";
const client = getClient("https://example.org", { apiVersion });
const validationPolicy: PipelinePolicy = {
name: "validationPolicy",
sendRequest: (req, next) => {
assert.include(req.url, `colors=blue,red,green&api-version=${apiVersion}`);
return next(req);
},
};

client.pipeline.addPolicy(validationPolicy, { afterPhase: "Serialize" });
await client.pathUnchecked("/foo").get({
queryParameters: {
colors: ["blue", "red", "green"],
},
skipUrlEncoding: true,
});
});

it("should encode url when not skip query parameter encoding and api version parameter exists", async () => {
const defaultHttpClient = getCachedDefaultHttpsClient();
sinon.stub(defaultHttpClient, "sendRequest").callsFake(async (req) => {
return {
headers: createHttpHeaders(),
status: 200,
request: req,
} as PipelineResponse;
});

const apiVersion = "2021-11-18";
const client = getClient("https://example.org", { apiVersion });
const validationPolicy: PipelinePolicy = {
name: "validationPolicy",
sendRequest: (req, next) => {
assert.include(req.url, `colors=blue%2Cred%2Cgreen&api-version=${apiVersion}`);
return next(req);
},
};

client.pipeline.addPolicy(validationPolicy, { afterPhase: "Serialize" });
await client.pathUnchecked("/foo").get({
queryParameters: {
colors: ["blue", "red", "green"],
},
});
});
});

it("should append api version correctly", async () => {
const defaultHttpClient = getCachedDefaultHttpsClient();
sinon.stub(defaultHttpClient, "sendRequest").callsFake(async (req) => {
return {
headers: createHttpHeaders(),
status: 200,
request: req,
} as PipelineResponse;
});

const apiVersion = "2021-11-18";
const client = getClient("https://example.org", { apiVersion });
const validationPolicy: PipelinePolicy = {
name: "validationPolicy",
sendRequest: (req, next) => {
assert.equal(req.url, `https://example.org/foo?api-version=${apiVersion}`);
return next(req);
},
};

client.pipeline.addPolicy(validationPolicy, { afterPhase: "Serialize" });
await client.pathUnchecked("/foo").get();
});

it("should insert policies in the correct pipeline position", async function () {
Expand Down

0 comments on commit 2938135

Please sign in to comment.