add async validation support #4762

mastermunj · 2020-02-28T09:16:42Z

Add support for async validations. The need arises from allowing custom formats & keywords for ajv, which can also be async in nature.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

raymondfeng · 2020-02-28T16:01:23Z

packages/rest/src/__tests__/unit/request-body.validator.test.ts

+    } catch (error) {
+      expect(error.message).to.equal(INVALID_MSG);
+      expect(error.code).to.equal('VALIDATION_FAILED');
+    }


We use await expect(aPromise).to.be.rejectedWith(...) style.

raymondfeng · 2020-02-28T16:03:32Z

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

+  let validationResult = validate(body);
+  let validationErrors: AJV.ErrorObject[] = [];
+  try {
+    validationResult = await validationResult;


It's not very friendly to change the type of validationResult in the same function.

Thanks for pointing it out. I've simplified the logic. Is new one better & clear?

raymondfeng

Please squash the commits into one.

mastermunj · 2020-02-29T18:34:40Z

Uh oh! I messed it up it seems.

Edit 1: Apologies for messing up. I will try to fix it with a hard reset. If it doesn't work, I will send another PR.

Edit 2: It seems to have been done properly now. Please help check if it's done right.

dougal83 · 2020-02-29T19:09:38Z

@mastermunj Don't worry if you make mistakes with Git, we've all been there. There are plenty of very helpful people who can help you fix what seems like a disaster. It's source control... most problems can be fixed.

achrinza

Thank you @mastermunj for the PR ❤️

I've added a comment regarding the drop in test coverage and how we might be able to resolve it.

As what @dougal83 mentioned: No worries, if there's any git-related issue, we can help.

If things goes south, there's probably no need to create a new PR. At most a hard-reset against origin/master will bring things back on track.

achrinza · 2020-03-01T06:04:26Z

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

+    // When schema is not available, ajv returns null
+    if (validationResult || validationResult === null) {
+      debug('Request body passed AJV validation.');
+      return;
+    }


Seems like this branch isn't fully covered with tests yet (hence the reduced test coverage).

Is there a case where the schema isn't available when AJV is called?

It'll be great if we can add a test case to prevent regression.

Here's the Coveralls report

Hi @achrinza, this test case doesn't pass without validationResult === null as promise returned by validate returns null.

My comment there seems misleading with schema is not available. Schema is available but body is optional and value is empty.

@achrinza did you get a chance to look into this?

@mastermunj I would suggest editing the comment to be more accurate to the purpose of the conditional statement.

Though @raymondfeng has approved the PR and this issue is a non-blocker for me.

So everything LGTM 👍

My comment there seems misleading with schema is not available. Schema is available but body is optional and value is empty.

I would really like the commit to be updated before this pull request is landed, to avoid confusion in the future.

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

raymondfeng · 2020-03-02T16:57:08Z

@mastermunj I suggest you further refine the commit message as follows:

feat(rest): add async validation support

BREAKING CHANGE: validateRequestBody is now an async function to allow asynchronous validations by custom Ajv keywords and formats. See https://ajv.js.org/#asynchronous-validation
for more details.

mastermunj · 2020-03-03T05:10:38Z

@raymondfeng modified commit message as per your suggestion.

bajtos

This is awesome, I really like how the pull request is focused on one thing only (enabling async validations) 👍

Let's fix the misleading commit please, before we proceed to land the pull request.

bajtos · 2020-03-03T13:39:12Z

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

+    // When schema is not available, ajv returns null
+    if (validationResult || validationResult === null) {
+      debug('Request body passed AJV validation.');
+      return;
+    }


My comment there seems misleading with schema is not available. Schema is available but body is optional and value is empty.

I would really like the commit to be updated before this pull request is landed, to avoid confusion in the future.

BREAKING CHANGE: `validateRequestBody` is now an async function to allow asynchronous validations by custom Ajv keywords and formats. See https://ajv.js.org/#asynchronous-validation for more details.

dhmlau · 2020-03-03T14:29:51Z

@slnode test please

mastermunj requested review from deepakrkris, emonddr and hacksparrow as code owners February 28, 2020 09:16

raymondfeng reviewed Feb 28, 2020

View reviewed changes

mastermunj requested a review from raymondfeng February 28, 2020 18:38

raymondfeng approved these changes Feb 28, 2020

View reviewed changes

dhmlau added the community-contribution label Feb 28, 2020

mastermunj requested review from agnes512, bajtos, jannyHou, marioestradarosa and nabdelgadir as code owners February 29, 2020 18:33

mastermunj force-pushed the async-validation branch 2 times, most recently from f023e0c to 25965ba Compare February 29, 2020 19:03

mastermunj requested a review from raymondfeng March 1, 2020 05:47

achrinza reviewed Mar 1, 2020

View reviewed changes

achrinza added the REST Issues related to @loopback/rest package and REST transport in general label Mar 1, 2020

achrinza reviewed Mar 1, 2020

View reviewed changes

packages/rest/src/validation/request-body.validator.ts Show resolved Hide resolved

mastermunj force-pushed the async-validation branch from 25965ba to 8a0d955 Compare March 2, 2020 09:22

mastermunj force-pushed the async-validation branch from 8a0d955 to 5c63168 Compare March 3, 2020 05:09

raymondfeng approved these changes Mar 3, 2020

View reviewed changes

bajtos added breaking-change feature labels Mar 3, 2020

bajtos approved these changes Mar 3, 2020

View reviewed changes

feat(rest): add async validation support

3b49284

BREAKING CHANGE: `validateRequestBody` is now an async function to allow asynchronous validations by custom Ajv keywords and formats. See https://ajv.js.org/#asynchronous-validation for more details.

mastermunj force-pushed the async-validation branch from 02da30e to 3b49284 Compare March 3, 2020 14:14

hacksparrow approved these changes Mar 3, 2020

View reviewed changes

raymondfeng merged commit 5b9a1ef into loopbackio:master Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add async validation support #4762

add async validation support #4762

mastermunj commented Feb 28, 2020

raymondfeng Feb 28, 2020

mastermunj Feb 28, 2020

raymondfeng Feb 28, 2020

mastermunj Feb 28, 2020

raymondfeng left a comment

mastermunj commented Feb 29, 2020 •

edited

dougal83 commented Feb 29, 2020

achrinza left a comment •

edited

achrinza Mar 1, 2020 •

edited

mastermunj Mar 2, 2020

mastermunj Mar 3, 2020

achrinza Mar 3, 2020 •

edited

bajtos Mar 3, 2020

raymondfeng commented Mar 2, 2020 •

edited by achrinza

mastermunj commented Mar 3, 2020

bajtos left a comment

bajtos Mar 3, 2020

dhmlau commented Mar 3, 2020

add async validation support #4762

add async validation support #4762

Conversation

mastermunj commented Feb 28, 2020

Checklist

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

mastermunj commented Feb 29, 2020 • edited

dougal83 commented Feb 29, 2020

achrinza left a comment • edited

Choose a reason for hiding this comment

achrinza Mar 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achrinza Mar 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Mar 2, 2020 • edited by achrinza

mastermunj commented Mar 3, 2020

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhmlau commented Mar 3, 2020

mastermunj commented Feb 29, 2020 •

edited

achrinza left a comment •

edited

achrinza Mar 1, 2020 •

edited

achrinza Mar 3, 2020 •

edited

raymondfeng commented Mar 2, 2020 •

edited by achrinza