feat: coerce query object with schema #5808

jannyHou · 2020-06-23T03:04:04Z

Connect to #4992

Filter with non-string properties like ?filter[fields][name]=false are not coerced, so false turns to be 'false'. This PR applies coercion for parameter query.

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 👈

agnes512 · 2020-06-23T13:44:09Z

packages/rest/src/__tests__/acceptance/coercion/coercion.acceptance.ts

@@ -98,9 +115,9 @@ describe('Coercion', () => {
      // This is because all values are encoded as strings on URL queries
      // and we did not specify any schema in @param.query.object() decorator.
      where: {
-        id: '1',


Do we want to add fields filter to this test case? You might want to remove the comment above as it is fixed.

@agnes512 I started adding feature by manually verify the field filter in todo app, so the fields filter does work.

where: {active: true}
and
fields: {name: true}
doesn't make any difference for testing purpose, so I would say the current test is good enough to cover our case(field filter) :p

Oh I mentioned it because I remember when I was testing it, it works for where for some reasons 😂

@agnes512 I removed my change and tested with todo app, filter ?[where][isComplete]=false does return correct result for some reason, weird.

But the filter it self is not coerced, see my screenshot in:

isComplete is "false" not false.

I need more time to investigate the reason, but the updated test case here should be correct. For the parameter coerce function, it wouldn't make sense if

where: {active: true} get coerced while fields: {name: true} not.

raymondfeng · 2020-06-23T18:00:26Z

packages/rest/src/coercion/coerce-parameter.ts

+  const schema = extractSchemaFromSpec(spec);
+  if (schema) {
+    // apply coercion based on properties defined by spec.schema
+    const ajv = new Ajv({coerceTypes: true});


It's a bit concerning that we hard-code the instantiation of Ajv here. We typically want to inject the ajv options.

@raymondfeng The ajv here is not used for validation, but for coercion purpose, I know this sounds a bit weird, but I couldn't find a better&simpler way to coerce based on schema.

See ajv doc in https://github.com/ajv-validator/ajv#coercing-data-types

If you search options in file coerce parameter, there is no options available for coercing object.

We should add an optional options parameter for coerceParameter. Its main usage is in parse.ts which has access to the configuration.

@raymondfeng Let me submit another PR or at least a separate commit for options.

raymondfeng · 2020-06-23T19:46:27Z

packages/rest/src/coercion/coerce-parameter.ts

@@ -18,6 +23,7 @@ import {
  matchDateFormat,
 } from './utils';
 import {Validator} from './validator';
+const Ajv = require('ajv');


Please use import instead of require.

raymondfeng · 2020-06-23T19:47:19Z

packages/rest/src/coercion/coerce-parameter.ts

+  if (schema) {
+    // apply coercion based on properties defined by spec.schema
+    const ajv = new Ajv({coerceTypes: true});
+    const validate = ajv.compile(schema);


We should not create the Ajv instance per call. It's fairly expensive.

True, I refactored to use one global instance. Or should I use a singleton scope provider instead?

packages/rest/src/__tests__/acceptance/coercion/coercion.acceptance.ts

raymondfeng · 2020-06-24T16:23:13Z

packages/rest/src/coercion/coerce-parameter.ts

+  const schema = extractSchemaFromSpec(spec);
+  if (schema) {
+    // Apply coercion based on properties defined by spec.schema
+    const validate = AJV.compile(schema);


See more details at #1757. The compile call is probably the most expensive one.

We probably have to maintain a weak map (schema -> ajv).

raymondfeng · 2020-06-24T16:27:53Z

packages/rest/src/coercion/coerce-parameter.ts

+ * The AJV instance is created for coercing object parameters
+ * with provided schema. Not for validation.
+ */
+const AJV = new AjvCtor({coerceTypes: true});


We have an AjvFactory being injected and it's available via options.

bajtos

IIUC, the observed behavior is caused by the fact that we are not validating non-body parameters yet, see #1573 and linked PRs that attempted to solve that, but were never finished.

Instead of trying to add coercion-only step and having to deal with problems like how to obtain the AJV instance in a way that's consistent with the existing REST-layer validations, can we please implement proper validation of all non-body parameters and thus get coercion for "free"?

Even though my old TODO comment says coerceObject should take into account spec.schema, I think it maybe better to keep coerce-parameter implementation schema-less and leave it up to the validation step to perform additional coercion as needed.

bajtos · 2020-06-25T09:38:35Z

packages/rest/src/coercion/coerce-parameter.ts

+  const schema = extractSchemaFromSpec(spec);
+  if (schema) {
+    // Apply coercion based on properties defined by spec.schema
+    await validateValueAgainstSchema(


validateValueAgainstSchema throws an error when AJV validation fails. Is it ok for coerceObject to throw validation errors?

validateValueAgainstSchema prints debug logs referring to request body. If we are going to use validateValueAgainstSchema for non-body parameters (which consider as a good idea), then the debug logs need to be updated accordingly.

We should also take a look at the validation errors produced for non-body parameters and make sure they provide enough information about which parameter failed the validation.

Another aspect to consider: should we report the first validation error only (I think that's the current design)? IMO, it's easier for users when all errors are reported at once. A possible solution for this is to build a "composite" object from all parameters (e.g. {filter: <value from query string>, authorization: <value from headers>}), a "composite" schema from Operation Object, and then call AJV only once. I think this way the error messages will include the parameter name ("filter", "authorization") as part of the description (path to the property with a wrong value).

Having an aggregated schema that stitches all incoming params (including the body) is an interesting idea that we should explore. But let's leave it out of scope for this PR.

Having an aggregated schema that stitches all incoming params (including the body) is an interesting idea that we should explore.

@bajtos @raymondfeng Thanks for suggestion, I also tend to create a separate story for it. In this PR, I turned validateValueAgainstSchema a generic function that validates any value:

rename 1st parameter: data --> value

ValueValidationOptions extends RequestBodyValidationOptions with one more property position

throws invalid request body error when position is 'body', throws invalid data error otherwise.

The new story can create a new rest http error for other invalid data.
WDYT?

creating the follow-up story, post here soon.

bajtos · 2020-06-25T09:39:35Z

packages/rest/src/__tests__/acceptance/coercion/coercion.acceptance.ts

@@ -94,13 +111,10 @@ describe('Coercion', () => {
      })
      .expect(200);
    sinon.assert.calledWithExactly(spy, {
-      // Notice that numeric and boolean values are converted to strings.
-      // This is because all values are encoded as strings on URL queries
-      // and we did not specify any schema in @param.query.object() decorator.


Please preserve the "notice" comment and update it to describe (point out) the new behavior.

On the second though, I think we should preserve the current test as-is to describe how are schema-less values handled, and add a new test to verify how does schema-based coercion work.

I think we should preserve the current test as-is to describe how are schema-less values handled, and add a new test to verify how does schema-based coercion work.

Good point, I will edit the test.

bajtos · 2020-06-25T14:11:59Z

packages/rest/src/parser.ts

@@ -83,7 +83,7 @@ async function buildOperationArguments(
    }
    const spec = paramSpec as ParameterObject;
    const rawValue = getParamFromRequest(spec, request, pathParams);
-    const coercedValue = coerceParameter(rawValue, spec);
+    const coercedValue = await coerceParameter(rawValue, spec);


Building on top of my previous comments, perhaps this is the place where to call validation?

Suggested change

const coercedValue = await coerceParameter(rawValue, spec);

const coercedValue = coerceParameter(rawValue, spec);

const finalValue = await validateParameterValue(rawValue, spec);

paramArgs.push(finalValue);

(Of course, this will not help with collecting all validation error together and other aspects mentioned in my other comments, but it may be good enough as a short-term fix.)

We have also access to globalSchemas and options here, which is needed to resolve {$ref} schemas and use the same AJV factory as the request body validator does.

Suggested change

const coercedValue = await coerceParameter(rawValue, spec);

const coercedValue = await coerceParameter(rawValue, spec);

const coercedValue = coerceParameter(rawValue, spec);

const finalValue = await validateParameterValue(rawValue, spec, globalSchemas, options);

paramArgs.push(finalValue);

Thanks @bajtos I tried calling validateValueAgainstSchema in the new place, and got 20+ test failures. This story is to coerce the object based on schema, but coerceParameter handles all types, and same to validateValueAgainstSchema if I call it after coerceParameter.

I am a bit reluctant to investigate and verify why other types fail, and considering schema is usually used for object, can we deal with other types next time?

raymondfeng · 2020-06-25T19:28:34Z

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

@@ -115,7 +115,7 @@ function getKeyForOptions(options: RequestBodyValidationOptions) {
 * @param globalSchemas - Schema references.
 * @param options - Request body validation options.
 */
-async function validateValueAgainstSchema(
+export async function validateValueAgainstSchema(


Let's change the reference to body with a generic one, such as value.

packages/rest/src/coercion/coerce-parameter.ts

packages/rest/src/types.ts

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

agnes512

Not sure what validateValueAgainstSchema does but the rest LGTM 👍

packages/rest/src/coercion/coerce-parameter.ts

jannyHou requested review from deepakrkris, emonddr and hacksparrow as code owners June 23, 2020 03:04

agnes512 reviewed Jun 23, 2020

View reviewed changes

raymondfeng reviewed Jun 23, 2020

View reviewed changes

agnes512 reviewed Jun 23, 2020

View reviewed changes

packages/rest/src/__tests__/acceptance/coercion/coercion.acceptance.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Jun 24, 2020

View reviewed changes

bajtos reviewed Jun 25, 2020

View reviewed changes

raymondfeng reviewed Jun 25, 2020

View reviewed changes

jannyHou force-pushed the coerce-object branch 3 times, most recently from f3d89d4 to b545786 Compare June 28, 2020 20:13

raymondfeng reviewed Jun 29, 2020

View reviewed changes

packages/rest/src/coercion/coerce-parameter.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Jun 29, 2020

View reviewed changes

packages/rest/src/types.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Jun 29, 2020

View reviewed changes

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

raymondfeng approved these changes Jun 29, 2020

View reviewed changes

agnes512 approved these changes Jun 30, 2020

View reviewed changes

packages/rest/src/coercion/coerce-parameter.ts Show resolved Hide resolved

jannyHou force-pushed the coerce-object branch from 7bf25e4 to a67298a Compare June 30, 2020 14:04

feat: coerce query object with schema

96dd076

jannyHou force-pushed the coerce-object branch from a67298a to 96dd076 Compare June 30, 2020 14:23

jannyHou merged commit ccea25f into master Jun 30, 2020

jannyHou deleted the coerce-object branch June 30, 2020 14:59

agnes512 mentioned this pull request Jul 6, 2020

Unable to perform nested include in filter since @loopback-rest 5.2.0 #5872

Closed

bajtos mentioned this pull request Aug 13, 2020

feat(rest): improve validation errors for invalid parameter value #6105

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: coerce query object with schema #5808

feat: coerce query object with schema #5808

jannyHou commented Jun 23, 2020

agnes512 Jun 23, 2020

jannyHou Jun 23, 2020 •

edited

Loading

agnes512 Jun 23, 2020

jannyHou Jun 23, 2020 •

edited

Loading

raymondfeng Jun 23, 2020

jannyHou Jun 23, 2020 •

edited

Loading

raymondfeng Jun 23, 2020

jannyHou Jun 23, 2020

raymondfeng Jun 23, 2020

raymondfeng Jun 23, 2020

jannyHou Jun 23, 2020

raymondfeng Jun 24, 2020

raymondfeng Jun 24, 2020

raymondfeng Jun 24, 2020

bajtos left a comment

bajtos Jun 25, 2020

raymondfeng Jun 25, 2020 •

edited

Loading

jannyHou Jun 28, 2020

jannyHou Jun 30, 2020

bajtos Jun 25, 2020

bajtos Jun 25, 2020

jannyHou Jun 25, 2020

bajtos Jun 25, 2020

bajtos Jun 25, 2020

jannyHou Jun 25, 2020

raymondfeng Jun 25, 2020

jannyHou Jun 28, 2020

agnes512 left a comment

feat: coerce query object with schema #5808

feat: coerce query object with schema #5808

Conversation

jannyHou commented Jun 23, 2020

Checklist

Choose a reason for hiding this comment

jannyHou Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jun 23, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Jun 25, 2020 • edited Loading

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

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

agnes512 left a comment

Choose a reason for hiding this comment

jannyHou Jun 23, 2020 •

edited

Loading

jannyHou Jun 23, 2020 •

edited

Loading

jannyHou Jun 23, 2020 •

edited

Loading

raymondfeng Jun 25, 2020 •

edited

Loading