-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(rest): add query parameter validation #2382
Conversation
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.
@YaelGit Thank you for opening the new PR! I left a comment regarding the query parser. Let me know if you need other help.
@@ -38,11 +39,16 @@ export async function parseOperationArgs( | |||
operationSpec, | |||
request, | |||
); | |||
const query = await requestBodyParser.loadRequestBodyIfNeeded( |
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 doubt whether we can get the query using requestBodyParser
, according to https://github.com/strongloop/loopback-next/blob/2e12925f2dcb89d2ef0262873615336446390386/packages/rest/src/body-parsers/body-parser.helpers.ts#L54, it eventually focuses and returns the body
field in a request.
I also understand that there would be a good amount of logic that could be shared among body parsers and query parsers, I would suggest:
- We start from creating a working
query-parser.json.ts
like https://github.com/strongloop/loopback-next/blob/2e12925f2dcb89d2ef0262873615336446390386/packages/rest/src/body-parsers/body-parser.json.ts - Refactor the common logic into utils so that body/query/header parser could share
- Implement the query parser and call its functions here.
Thank you @YaelGit for the pull request. I am afraid you are going in a wrong direction here. Conceptually, I would like our validation logic to be independent from the parameter source (body vs. query vs. headers vs. other sources). Let's take a look at the following code in our parameter parser: What I would like to see is a solution that simply adds validation step to that algorithm, e.g. const spec = paramSpec as ParameterObject;
const rawValue = getParamFromRequest(spec, request, pathParams);
const coercedValue = coerceParameter(rawValue, spec);
validateParameterValue(coercedValue, spec, globalSchemas); // <--- ADDED
paramArgs.push(coercedValue); The function This is the first step. The next step is to group all validation errors into a single error response. The idea is that when there are multiple parameters with invalid values, the response should describe validation errors for all of them. To keep things simple, I am proposing to leave this usability improvement out of scope of the initial pull request. |
@bajtos - i would only like to verify that this is the exact requirement (described by you above). |
I apologize for a late reply.
As I understand @raymondfeng's https://github.com/strongloop/loopback-next/pull/2307/files/8e3248c1e2930db1867bb27961cddef8cbbe500d#r252310803, he was asking to use a more descriptive name for the type that's used to validate request query. So yes, there is no need to rename that type. OTOH, depending on how you implement |
Hello @bajtos , if i understood you correctly the requested addition you require is a simple function which validates each parameter - validateRequestParameter(paramName, coercedValue, spec, paramSchema); and the removal of usage of current body validation. Plus a modification of the type requestBody in types.ts into 'ValueWithSchema' name. |
Hi @YaelGit, what's the status of this pull request? Are you still keen to work on this improvement? |
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
fix #1573
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated