-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: support exploded syntax in sdk #2896
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit a51e001.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files☔ View full report in Codecov by Sentry. |
d8fe62b
to
0868163
Compare
0868163
to
a51e001
Compare
} | ||
|
||
/** @inheritdoc */ | ||
public prepareUrl(url: string, data: { [key: string]: string | undefined }): string; |
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.
public prepareUrl(url: string, data: { [key: string]: string | undefined }): string; | |
public prepareUrl(url: string, data?: { [key: string]: string | undefined }): string; |
if (queryParamSerialization) { | ||
return prepareUrl(url, data as T, queryParamSerialization); | ||
} | ||
return prepareUrl(url, data as { [key: string]: string | undefined }); |
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.
return prepareUrl(url, data as { [key: string]: string | undefined }); | |
return prepareUrl(url, (data || {}) as { [key: string]: string | undefined }); |
/** @inheritdoc */ | ||
public prepareUrl<T extends { [key: string]: any }>(url: string, data?: { [key: string]: string | undefined } | T, queryParamSerialization?: { [K in keyof T]?: ParamSerialization }): string { | ||
if (queryParamSerialization) { | ||
return prepareUrl(url, data as T, queryParamSerialization); |
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.
You could import it with another name to avoid confusion? (prepareUrl as ....)
} | ||
|
||
/** @inheritdoc */ | ||
public prepareUrl(url: string, data: { [key: string]: string | undefined }): string; |
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.
Same comment as for api-angular clients
@@ -92,8 +94,20 @@ export class ApiFetchClient implements ApiClient { | |||
} | |||
|
|||
/** @inheritdoc */ | |||
public prepareUrl(url: string, queryParameters: { [key: string]: string | undefined } = {}) { | |||
return prepareUrl(url, queryParameters); | |||
public serializePathParams<T extends { [key: string]: any }>(data: T, pathParameters: { [K in keyof T]?: ParamSerialization }): { [p in keyof T]: string } { |
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.
Same comments as for api angular client
} | ||
|
||
/** @inheritdoc */ | ||
public prepareUrl(url: string, data: { [key: string]: string | undefined }): string; |
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.
Same comment as the other client angular
public prepareUrl(url: string, queryParameters?: { [key: string]: string }): string { | ||
return prepareUrl(url, queryParameters); | ||
public serializePathParams<T extends { [key: string]: any }>(data: T, pathParameters: { [K in keyof T]?: ParamSerialization }): { [p in keyof T]: string } { | ||
return serializePathParams(data, pathParameters); |
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.
Same comment as the client angular
@@ -115,8 +117,20 @@ export class ApiFetchClient implements ApiClient { | |||
} | |||
|
|||
/** @inheritdoc */ | |||
public prepareUrl(url: string, queryParameters: { [key: string]: string | undefined } = {}) { | |||
return prepareUrl(url, queryParameters); | |||
public serializePathParams<T extends { [key: string]: any }>(data: T, pathParameters: { [K in keyof T]?: ParamSerialization }): { [p in keyof T]: string } { |
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.
Same comment as the client angular
const queryParamExplode = paramSerialization.explode; | ||
const queryParamStyle = paramSerialization.style; | ||
const value = (() => { | ||
if (Array.isArray(queryParamValue)) { |
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.
NB but I would have extracted the logic until line in another function for more readability
* prepares the url to be called | ||
* @param url base url to be used | ||
* @param queryParameters key value pair with the parameters. If the value is undefined, the key is dropped | ||
* Serialize query parameters based on the values of exploded and style |
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.
Could be nice to have the reference to the parameter serialization
const value = (() => { | ||
if (Array.isArray(queryParamValue)) { | ||
if (queryParamExplode) { | ||
switch (queryParamStyle) { |
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.
why a switch case if it is the same logic for everyone?
const pathParamValue = data[pathParamName]; | ||
if (typeof pathParamValue !== 'undefined' && pathParamValue !== null && !!pathParamSerialization) { | ||
const value = (() => { | ||
if (Array.isArray(pathParamValue)) { |
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.
Same remark for the extraction of the code
* @param data Query parameters key value pair. If the value is undefined, the key is dropped. | ||
* @deprecated use `prepareUrl` with query parameter serialization, will be removed in v14. | ||
*/ | ||
export function prepareUrl(url: string, data: { [key: string]: string | undefined }): string; |
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.
export function prepareUrl(url: string, data: { [key: string]: string | undefined }): string; | |
export function prepareUrl(url: string, data?: { [key: string]: string | undefined }): string; |
|
||
const queryPart = Object.keys(data) |
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.
const queryPart = Object.keys(data) | |
const queryPart = Object.keys(data || {}) |
*/ | ||
public static class TokenizedUrlParamReplacerLambda extends CustomLambda { | ||
|
||
public TokenizedUrlParamReplacerLambda() {} | ||
|
||
@Override | ||
public String formatFragment(String fragment) { | ||
return fragment.replaceAll("\\{([\\w_-]+)\\}", "\\${this.piiParamTokens['$1'] || data['$1']}"); | ||
return fragment.replaceAll("\\{([\\w_-]+)\\}", "\\${this.piiParamTokens['$1'] || pathParams['$1']}"); |
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.
Why do you need to change that?
} else if (typeof queryParamValue === 'object') { | ||
if (queryParamStyle === 'form') { | ||
return queryParamExplode | ||
? Object.keys(queryParamValue).map((prop) => `${prop}=${queryParamValue[prop]}`).join('&') |
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.
? Object.keys(queryParamValue).map((prop) => `${prop}=${queryParamValue[prop]}`).join('&') | |
? Object.entries(queryParamValue).map(([propName, propValue]) => `${propName}=${propValue}`).join('&') |
if (queryParamStyle === 'form') { | ||
return queryParamExplode | ||
? Object.keys(queryParamValue).map((prop) => `${prop}=${queryParamValue[prop]}`).join('&') | ||
: `${name}=` + Object.keys(queryParamValue).map((prop) => `${prop},${queryParamValue[prop]}`).join(','); |
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.
: `${name}=` + Object.keys(queryParamValue).map((prop) => `${prop},${queryParamValue[prop]}`).join(','); | |
: `${name}=` + Object.entries(queryParamValue).map(([propName, propValue]) => `${propName},${propValue}`).join(','); |
? Object.keys(queryParamValue).map((prop) => `${prop}=${queryParamValue[prop]}`).join('&') | ||
: `${name}=` + Object.keys(queryParamValue).map((prop) => `${prop},${queryParamValue[prop]}`).join(','); | ||
} else { | ||
return Object.keys(queryParamValue).map((prop) => `${name}[${prop}]=${queryParamValue[prop]}`).join('&'); |
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.
return Object.keys(queryParamValue).map((prop) => `${name}[${prop}]=${queryParamValue[prop]}`).join('&'); | |
return Object.entries(queryParamValue).map(([propName, propValue]) => `${name}[${propName}]=${propValue}`).join('&'); |
if (queryParamSerialization) { | ||
if (Object.keys(queryParamSerialization).length > 0) { | ||
const serializedQueryParams = serializeQueryParams(data as T, queryParamSerialization); | ||
return url + paramsPrefix + Object.values(serializedQueryParams).join('&'); |
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.
If your data is empty (because you don't want to pass parameters) but you have optional parameters in your queryParamSerialization, you will have an extra ?
for example : 'test.com?' instead of 'test.com'
switch (pathParamSerialization.style) { | ||
case 'simple': { | ||
return pathParamSerialization.explode | ||
? Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join(',') |
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.
? Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join(',') | |
? Object.entries(pathParamValue).map(([propName, propValue]) => `${propName}=${propValue}`).join(',') |
case 'simple': { | ||
return pathParamSerialization.explode | ||
? Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join(',') | ||
: Object.keys(pathParamValue).map((property) => `${property},${pathParamValue[property]}`).join(','); |
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.
: Object.keys(pathParamValue).map((property) => `${property},${pathParamValue[property]}`).join(','); | |
: Object.entries(pathParamValue).map(([propName, propValue]) => `${propName},${propValue}`).join(','); |
} | ||
case 'label': { | ||
return pathParamSerialization.explode | ||
? '.' + Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join('.') |
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.
? '.' + Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join('.') | |
? '.' + Object.entries(pathParamValue).map(([propName, propValue]) => `${propName}=${propValue}`).join('.') |
case 'label': { | ||
return pathParamSerialization.explode | ||
? '.' + Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join('.') | ||
: '.' + Object.keys(pathParamValue).map((property) => `${property},${pathParamValue[property]}`).join(','); |
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.
: '.' + Object.keys(pathParamValue).map((property) => `${property},${pathParamValue[property]}`).join(','); | |
: '.' + Object.entries(pathParamValue).map(([propName, propValue]) => `${propName},${propValue}`).join(','); |
} | ||
case 'matrix': { | ||
return pathParamSerialization.explode | ||
? ';' + Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join(';') |
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.
? ';' + Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join(';') | |
? ';' + Object.entries(pathParamValue).map(([propName, propValue]) => `${propName}=${propValue}`).join(';') |
case 'matrix': { | ||
return pathParamSerialization.explode | ||
? ';' + Object.keys(pathParamValue).map((property) => `${property}=${pathParamValue[property]}`).join(';') | ||
: `;${pathParamName}=` + Object.keys(pathParamValue).map((property) => `${property},${pathParamValue[property]}`).join(','); |
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.
: `;${pathParamName}=` + Object.keys(pathParamValue).map((property) => `${property},${pathParamValue[property]}`).join(','); | |
: `;${pathParamName}=` + Object.entries(pathParamValue).map(([propName, propValue]) => `${propName},${propValue}`).join(','); |
Proposed change
Support of exploded syntax for query and path parameters: https://swagger.io/docs/specification/v3_0/serialization/
This is an alternate solution to the existing PR #2865 to avoid breaking changes.
Related issues
#640
- No issue associated -