Skip to content

Commit

Permalink
[core-client-rest] Fix serialization of non-string path parameters (#…
Browse files Browse the repository at this point in the history
…31352)

### Packages impacted by this PR

- `@azure-rest/core-client`

### Issues associated with this PR

- Fix #31349

### Describe the problem that is addressed by this PR

We generate RLCs that allow for path parameters to be `number`. The
change for `allowReserved` assumed that path parameters could only be
`string`, causing number parameters to not be serialized properly. This
PR fixes that assumption.

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

Added a test to check for a number path parameter

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

- #31058
  • Loading branch information
timovv authored Oct 10, 2024
1 parent ba31999 commit 60584c7
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 10 deletions.
2 changes: 2 additions & 0 deletions sdk/core/core-client-rest/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Bugs Fixed

- Allow `number` path parameters. PR [#31352](https://github.com/Azure/azure-sdk-for-js/pull/31352/files)

### Other Changes

## 2.3.0 (2024-10-03)
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/core-client-rest/review/core-client.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,15 @@ export interface OperationRequestOptions {

// @public
export type PathParameters<TRoute extends string> = TRoute extends `${infer _Head}/{${infer _Param}}${infer Tail}` ? [
pathParameter: string | PathParameterWithOptions,
pathParameter: string | number | PathParameterWithOptions,
...pathParameters: PathParameters<Tail>
] : [
];

// @public
export interface PathParameterWithOptions {
allowReserved?: boolean;
value: string;
value: string | number;
}

// @public
Expand Down
7 changes: 5 additions & 2 deletions sdk/core/core-client-rest/src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,10 @@ export type PathParameters<
// additional parameters we can call RouteParameters recursively on the Tail to match the remaining parts,
// in case the Tail has more parameters, it will return a tuple with the parameters found in tail.
// We spread the second path params to end up with a single dimension tuple at the end.
[pathParameter: string | PathParameterWithOptions, ...pathParameters: PathParameters<Tail>]
[
pathParameter: string | number | PathParameterWithOptions,
...pathParameters: PathParameters<Tail>,
]
: // When the path doesn't match the template, it means that we have no path parameters so we return
// an empty tuple.
[];
Expand Down Expand Up @@ -438,7 +441,7 @@ export interface PathParameterWithOptions {
/**
* The value of the parameter.
*/
value: string;
value: string | number;

/**
* Whether to allow for reserved characters in the value. If set to true, special characters such as '/' in the parameter's value will not be URL encoded.
Expand Down
11 changes: 5 additions & 6 deletions sdk/core/core-client-rest/src/urlHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function isQueryParameterWithOptions(x: unknown): x is QueryParameterWithOptions
export function buildRequestUrl(
endpoint: string,
routePath: string,
pathParameters: (string | PathParameterWithOptions)[],
pathParameters: (string | number | PathParameterWithOptions)[],
options: RequestParameters = {},
): string {
if (routePath.startsWith("https://") || routePath.startsWith("http://")) {
Expand Down Expand Up @@ -187,19 +187,18 @@ export function buildBaseUrl(endpoint: string, options: RequestParameters): stri

function buildRoutePath(
routePath: string,
pathParameters: (string | PathParameterWithOptions)[],
pathParameters: (string | number | PathParameterWithOptions)[],
options: RequestParameters = {},
): string {
for (const pathParam of pathParameters) {
const allowReserved =
typeof pathParam === "string" ? false : (pathParam?.allowReserved ?? false);
let value = typeof pathParam === "string" ? pathParam : pathParam?.value;
const allowReserved = typeof pathParam === "object" && (pathParam.allowReserved ?? false);
let value = typeof pathParam === "object" ? pathParam.value : pathParam;

if (!options.skipUrlEncoding && !allowReserved) {
value = encodeURIComponent(value);
}

routePath = routePath.replace(/\{\w+\}/, value);
routePath = routePath.replace(/\{\w+\}/, String(value));
}
return routePath;
}
Expand Down
6 changes: 6 additions & 0 deletions sdk/core/core-client-rest/test/urlHelpers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ describe("urlHelpers", () => {
assert.equal(result, `https://example.org/foo/one`);
});

it("should append number path parameters", () => {
const result = buildRequestUrl(mockBaseUrl, "/foo/{id}", [12345]);

assert.equal(result, "https://example.org/foo/12345");
});

it("should append path, fill path parameters and append query parameters", () => {
const result = buildRequestUrl(mockBaseUrl, "/foo/{id}", ["one"], {
queryParameters: { foo: "1", bar: "two" },
Expand Down

0 comments on commit 60584c7

Please sign in to comment.