feat(rest): implement query parameter validation (issue 1573) #2307

YaelGit · 2019-01-30T11:25:10Z

Implements #1573

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

YaelGit · 2019-01-30T11:27:49Z

@bajtos - Please note I pushed my changes to validate query parameters

raymondfeng · 2019-01-30T15:36:40Z

packages/rest/src/parser.ts

+    operationSpec,
+    request,
+  );
+


This is suspicious. Why do we parse the request body again into query?

raymondfeng · 2019-01-30T15:40:48Z

packages/rest/src/validation/request-query.validator.ts

+ * @param globalSchemas The referenced schemas generated from `OpenAPISpec.components.schemas`.
+ */
+export function validateRequestQuery(
+  query: RequestBody,


If we want to reuse RequestBody for query, we should rename the type as something like ValueWithSchema.

@raymondfeng - do you mean we should duplicate the type requestBody in types.ts and then use this new type (ValueWithSchema) in request-query.validator.ts?
should we also duplicate the BodyParser, BodyParserFunction? and then duplicate the code in body-parser.ts?

No duplication. What I meant is to refactor RequestBody to ValueWithSchema into its own file and use it for both body and query validation.

raymondfeng · 2019-01-30T15:42:16Z

packages/rest/src/validation/request-query.validator.ts

+  );
+
+  return ajv.compile(schemaWithRef);
+}


There seem to be quite a bit duplicate code as request-body-validator. Can we refactor the code for better reuse?

@raymondfeng -Thanks for the review and comments.
I will try to reply: I was actually looking to differentiate the 2 in order to allow/take into consideration future changes and needs-do you think this is un-needed? will the only difference be - the extraction of the query params and the structure to be sent to same validation function to leverage the AJV ? we need not forget, we also need different error messages. Please share your thoughts.

I would create a utility function that validates ValueWithSchema using AJV.

raymondfeng

@YaelGit Thank you for the contribution. I left a few comments for you to consider.

See mochajs/mocha#3550

This change greatly simplifies our build and project setup. - Source files are compiled from `src/{foo}` to `dist/{foo}`, the same pattern is applied to test files too. - Both TypeScript sources and JavaScript output are stored in the same path relative to project root. This makes it much easier to refer to test fixtures. This change is also enabling future improvements, for example TypeScript project references and migration to jest test runner.

raymondfeng · 2019-01-31T16:20:57Z

packages/rest/src/validation/request-query.validator.ts

+      new HttpErrors.BadRequest('Request query is required'),
+      {
+        code: 'MISSING_REQUIRED_PARAMETER',
+        parameterName: 'request query',


It would be nice to see which query param.

is this sufficient?

- Source files are compiled from `src/{foo}` to `dist/{foo}`, the same pattern is applied to test files too. - Both TypeScript sources and JavaScript output are stored in the same path relative to project root. This makes it much easier to refer to test fixtures. This is a follow-up for 066d525.

fix loopbackio#2149

Allow the user to specify a custom Repository class to inherit from. CLI supports custom repository name * via an interactive prompt * via CLI options * via JSON config Two tests modified to use the new parameter to pass Modified tests: * generates a kv repository from default model * generates a kv repository from decorator defined model

The greenkeeper.json is now automatically updated as part of `npm install` based on lerna packages.

- Source files are compiled from `src/{foo}` to `dist/{foo}`, the same pattern is applied to test files too. - Both TypeScript sources and JavaScript output are stored in the same path relative to project root. This makes it much easier to refer to test fixtures. This is a follow-up for 066d525 and 91a37dc.

bajtos · 2019-02-05T07:31:03Z

Conflicting files

packages/rest/test/unit/coercion/paramObject.unit.ts

In #2330, we have moved test files from test to src/__tests__. Please run follow the steps in How to rebase your branch to resolve the merge conflict. Please do not run git merge, it's important to use git rebase master.

Sorry for the inconvenience.

bajtos · 2019-02-08T13:47:03Z

@YaelGit ping 👋

What's the status of this pull request? Are you still keen to get it finished?

Merge branch 'master' into issue-1573

Please note we are avoiding merge commits to keep our git history neatly linear. We use git rebase instead, see How to rebase your branch.

bajtos · 2019-02-08T13:49:37Z

packages/rest/src/parser.ts

+    query.value = queryValue;
+    globalSchemas = {properties: schemasValue};
+    query.schema = globalSchemas;
+    validateRequestQuery(query, operationSpec.requestBody, globalSchemas);


IIUC, you are building an object with all parameters from the query string, and then validate this object in one pass. I have two reservations about this approach:

It seems overly complicated to me.

There are more parameter sources besides query and body, we need to validate parameters from all sources (e.g. path and header).

Have you considered to call ajv validator repeatedly, once for each parameter?

Hello @bajtos ,
sorry for the delay - yes i do plan to close the issue soon and in regards to your comments:

I will do the rebase as requested.

Regarding the validation of all parameters all together - i did it in this way due to following same validation standard done for body parameters.

Regrading the request for handling other parameter types, i think a new issue should be opened for other parameter types as the initial issue was for query only.

- @loopback/benchmark@1.1.10 - @loopback/docs@1.7.3 - @loopback/example-hello-world@1.1.3 - @loopback/example-log-extension@1.1.3 - @loopback/example-rpc-server@1.1.2 - @loopback/example-soap-calculator@1.3.3 - @loopback/example-todo-list@1.4.3 - @loopback/example-todo@1.4.3 - @loopback/authentication@1.0.12 - @loopback/boot@1.0.12 - @loopback/build@1.3.0 - @loopback/cli@1.6.0 - @loopback/context@1.5.1 - @loopback/core@1.1.6 - @loopback/http-caching-proxy@1.0.6 - @loopback/http-server@1.1.5 - @loopback/metadata@1.0.6 - @loopback/openapi-spec-builder@1.0.6 - @loopback/openapi-v3-types@1.0.6 - @loopback/openapi-v3@1.2.1 - @loopback/repository-json-schema@1.3.1 - @loopback/repository@1.1.5 - @loopback/rest-explorer@1.1.8 - @loopback/rest@1.5.5 - @loopback/service-proxy@1.0.8 - @loopback/testlab@1.0.6

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

…sue-1573

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

…sue-1573

YaelGit · 2019-02-12T12:44:57Z

Unfortunately due to some technical issue with my Laptop - things went wrong while trying to rebase.
I will need to reopen a new PR to re-enter my code.

YaelGit · 2019-02-13T11:34:22Z

Hello @bajtos @raymondfeng,
Due to technical issues, I have opened a new PRin the following link:
#2382
The PR includes implementation regarding all of the comments above.

YaelGit added 2 commits January 30, 2019 13:14

feat(rest): validate query parameters against their schema

72594eb

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

feat(rest): validate query parameters against their schema

0e04254

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

YaelGit requested review from bajtos and raymondfeng as code owners January 30, 2019 11:25

Merge branch 'master' into issue-1573

8e3248c

raymondfeng changed the title ~~Issue 1573~~ feat(rest): implement query parameter validation (issue 1573) Jan 30, 2019

raymondfeng reviewed Jan 30, 2019

View reviewed changes

raymondfeng requested changes Jan 30, 2019

View reviewed changes

raymondfeng and others added 5 commits January 30, 2019 13:35

chore: fix test cases to be compatible with mocha@6.x

a4c2086

See mochajs/mocha#3550

docs: fix instructions for verifying TypeScript setup

4c6b176

docs(testlab): describe test fixture directories

f946d48

fix: remove unused juggler import

0121c10

raymondfeng reviewed Jan 31, 2019

View reviewed changes

raymondfeng and others added 12 commits January 31, 2019 09:51

chore(context): add more unit tests for binding filters

181e1f1

chore: upgrade dependencies for build

0710055

docs(cli): use a custom repository base class

a624b95

fix loopbackio#2149

docs(cli): use a custom repository base class

0e92b88

chore: add required config for greenkeeper integration

4695e3a

The greenkeeper.json is now automatically updated as part of `npm install` based on lerna packages.

chore: update dependencies proposed by greenkeeper

857868e

chore: whitelist greenkeeper branches

bfe8c27

chore: add Greenkeeper badge

a8a409c

chore: add dominique to maintainers

84c6a88

bajtos added community-contribution and removed community-contribution labels Feb 8, 2019

bajtos self-assigned this Feb 8, 2019

bajtos added OpenAPI REST Issues related to @loopback/rest package and REST transport in general labels Feb 8, 2019

bajtos reviewed Feb 8, 2019

View reviewed changes

bajtos and others added 7 commits February 8, 2019 07:55

chore: exclude "good first issue" from stalebot

6ef5d85

fix: update to the most recent lodash version

65ee865

feat(rest): validate query parameters against their schema

95e919e

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

feat(rest): validate query parameters against their schema

112847b

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

Merge branch 'issue-1573' of github.com:YaelGit/loopback-next into is…

f14bd93

…sue-1573

fix(docs): broken link in Crafting in LB

7707fa5

YaelGit requested review from b-admike, hacksparrow, jannyHou and marioestradarosa as code owners February 12, 2019 07:24

YaelGit added 5 commits February 12, 2019 09:45

feat(rest): validate query parameters against their schema

bf22e5c

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

feat(rest): validate query parameters against their schema

131ccd9

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

feat(rest): validate query parameters against their schema

1fe7f9c

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

feat(rest): validate query parameters against their schema

14d7909

openapi spec allows developers to provide additional restrictions on input parameters via parameter schema. For example, pageSize can include schema restrictions fix loopbackio#1573

Merge branch 'issue-1573' of github.com:YaelGit/loopback-next into is…

c2c1c07

…sue-1573

YaelGit closed this Feb 12, 2019

YaelGit reopened this Feb 12, 2019

YaelGit closed this Feb 12, 2019

YaelGit mentioned this pull request Feb 19, 2019

feat(rest): add query parameter validation #2382

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rest): implement query parameter validation (issue 1573) #2307

feat(rest): implement query parameter validation (issue 1573) #2307

YaelGit commented Jan 30, 2019 •

edited by raymondfeng

Loading

YaelGit commented Jan 30, 2019

raymondfeng Jan 30, 2019

raymondfeng Jan 30, 2019

YaelGit Feb 3, 2019

raymondfeng Feb 4, 2019

raymondfeng Jan 30, 2019

YaelGit Jan 31, 2019

raymondfeng Jan 31, 2019

raymondfeng left a comment

raymondfeng Jan 31, 2019

YaelGit Feb 3, 2019

raymondfeng Feb 4, 2019

bajtos commented Feb 5, 2019

Conflicting files

bajtos commented Feb 8, 2019

bajtos Feb 8, 2019

YaelGit Feb 11, 2019

YaelGit commented Feb 12, 2019

YaelGit commented Feb 13, 2019

feat(rest): implement query parameter validation (issue 1573) #2307

feat(rest): implement query parameter validation (issue 1573) #2307

Conversation

YaelGit commented Jan 30, 2019 • edited by raymondfeng Loading

Checklist

YaelGit commented Jan 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 5, 2019

Conflicting files

bajtos commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaelGit commented Feb 12, 2019

YaelGit commented Feb 13, 2019

YaelGit commented Jan 30, 2019 •

edited by raymondfeng

Loading