-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: coerce object arguments from query strings #1667
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,25 +3,32 @@ | |
| // This file is licensed under the MIT License. | ||
| // License text available at https://opensource.org/licenses/MIT | ||
|
|
||
| import {ServerRequest} from 'http'; | ||
| import * as HttpErrors from 'http-errors'; | ||
| import {REQUEST_BODY_INDEX} from '@loopback/openapi-v3'; | ||
| import { | ||
| isReferenceObject, | ||
| OperationObject, | ||
| ParameterObject, | ||
| isReferenceObject, | ||
| SchemasObject, | ||
| } from '@loopback/openapi-v3-types'; | ||
| import {REQUEST_BODY_INDEX} from '@loopback/openapi-v3'; | ||
| import * as debugModule from 'debug'; | ||
| import {ServerRequest} from 'http'; | ||
| import * as HttpErrors from 'http-errors'; | ||
| import * as parseUrl from 'parseurl'; | ||
| import {parse as parseQuery} from 'qs'; | ||
| import {promisify} from 'util'; | ||
| import {OperationArgs, Request, PathParameterValues} from './types'; | ||
| import {ResolvedRoute} from './router/routing-table'; | ||
| import {coerceParameter} from './coercion/coerce-parameter'; | ||
| import {validateRequestBody} from './validation/request-body.validator'; | ||
| import {RestHttpErrors} from './index'; | ||
| import {ResolvedRoute} from './router/routing-table'; | ||
| import {OperationArgs, PathParameterValues, Request} from './types'; | ||
| import {validateRequestBody} from './validation/request-body.validator'; | ||
|
|
||
| type HttpError = HttpErrors.HttpError; | ||
| import * as debugModule from 'debug'; | ||
|
|
||
| const debug = debugModule('loopback:rest:parser'); | ||
|
|
||
| export const QUERY_NOT_PARSED = {}; | ||
| Object.freeze(QUERY_NOT_PARSED); | ||
|
|
||
| // tslint:disable-next-line:no-any | ||
| type MaybeBody = any | undefined; | ||
|
|
||
|
|
@@ -134,22 +141,31 @@ function getParamFromRequest( | |
| request: Request, | ||
| pathParams: PathParameterValues, | ||
| ) { | ||
| let result; | ||
| switch (spec.in) { | ||
| case 'query': | ||
| result = request.query[spec.name]; | ||
| break; | ||
| ensureRequestQueryWasParsed(request); | ||
| return request.query[spec.name]; | ||
| case 'path': | ||
| result = pathParams[spec.name]; | ||
| break; | ||
| return pathParams[spec.name]; | ||
| case 'header': | ||
| // @jannyhou TBD: check edge cases | ||
| result = request.headers[spec.name.toLowerCase()]; | ||
| return request.headers[spec.name.toLowerCase()]; | ||
| break; | ||
| // TODO(jannyhou) to support `cookie`, | ||
| // see issue https://github.com/strongloop/loopback-next/issues/997 | ||
| default: | ||
| throw RestHttpErrors.invalidParamLocation(spec.in); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| function ensureRequestQueryWasParsed(request: Request) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How is the above done here where we're calling
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left that out of this initial pull request and use Determining the minimum depth is a bit involved, as we have to visit all nested schemas and |
||
| if (request.query && request.query !== QUERY_NOT_PARSED) return; | ||
|
|
||
| const input = parseUrl(request)!.query; | ||
| if (input && typeof input === 'string') { | ||
| request.query = parseQuery(input); | ||
| } else { | ||
| request.query = {}; | ||
| } | ||
| debug('Parsed request query: ', request.query); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Shall we name it as
deepObject? Or something could infer that{in: 'query', style: 'deepObject'}is hardcorded for this decorator.I understand that for most situations the object is provided in query. While just in case people misuse it with parameters got from
path(with stylematrixorlabel), thought?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.
I think we need to distinguish between two bits of information provided by
@param.query.deepObject:deepObjectstyle.Object values provided in query can use the following encoding styles (see the section "Style values"):
?color[R]=100&color[G]=200&color[B]=150)explode: true(e.g.?color=R,100,G,200,B,150)explode: false(e.g.?R=100&G=200&B=150)As I see it,
@param.query.objectis saying that the value is ofobjecttype, similar how@param.query.numberor@param.query.booleanworks. To me, the fact that we allowdeepObjectstyle only, is a limitation we can lift later in the future. For example, we can allow callers to specify a differentstyleandexplodevalues by adding an optional third parameter:At the moment,
objectmethod is defined onparam.queryonly. I agree we should eventually addparam.path.objectandparam.header.objectin the future, supporting encoding styles that are appropriate for those sources. To me, it still makes sense to use the same method nameobject, because the method name is describing the value type, not the encoding style.Maybe I don't understand your concern, what kind of misuse do you have in mind?
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.
@bajtos
ok I got it 👍 I think we are on the same page now, previously I thought you are creating a decorator particularly for
{style: 'deepObject'}.And my bad I mixed the usage of
param.query.objectandparam.object.Let's keep the name as
param.query.object