[PoC - WIP] very rough PoC on request parameter type coercion/validation #1256

shimks · 2018-04-13T19:53:34Z

Note: this is NOT a comprehensive PR which covers all aspects and features we need in coercion and validation; it is merely a proof of concept to highlight certain approaches we can take and have room to be extended out into a full blown feature-complete PR. This PR will eventually be closed in favor of feature-separate PRs.

A general LoopBack types system is used to coerce data from a request into sensible JS type
The PoC shows for coercion:
- metadata from @operation and @param/@requestBody being used to infer the parameter type on the controller
- a converter (from @loopback/types) choosing the 'LoopBack' type from the metadata which will be used to coerce the request data
- the LoopBack type coercing the data into an appropriate type
For validation:
- tier 1 and tier 2 (3rd and 4th commit):
  - method level decorator @validatable and parameter level decorator @validate are used to do validation of parameters using JSON Schemas

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

jannyHou

@shimks reviewing the first commit from you(feature: do a very short PoC on type coercion):

Nice start!

The overall implementation looks reasonable to me 👍 : invoke the coerce in parser, or maybe we can have a separate action for it after parser,
IMO two things we should consider as implementation is:

the context we need when invoke coerce
take options in the coerce function.
we need the type coercion for both input and output:
- input: invoke in/after parser
- output: invoke in/before writer

shimks · 2018-04-23T18:18:19Z

@jannyHou what type of options should we consider when doing coercion?

jannyHou · 2018-04-23T20:18:42Z

@shimks It's just a very general option param that leave as the last parameter of a coercion function.

bajtos · 2018-04-27T12:27:46Z

packages/types/index.js

+// License text available at https://opensource.org/licenses/MIT
+
+const nodeMajorVersion = +process.versions.node.split('.')[0];
+module.exports = nodeMajorVersion >= 7 ? require('./dist') : require('./dist6');


We no longer support Node.js 6.x, where does this line come from? Could you please use the latest project infrastructure, e.g. as scaffolded by lb4 extension?

This PR builds on top of #947, which was proposed before we dropped Node 6. I didn't bother with pretty easily fixable stuff like this one to focus on the subject of the PoC itself. If we decide to go forward with the PoC, I (or someone else building on top of the PoC) will definitely need to fix this.

Agreed 👍

bajtos · 2018-04-27T12:32:33Z

packages/validator/test/acceptance/validate.decorator.acceptance.ts

+      @validate({format: 'email'})
+      str: string,
+      @param.path.number('num1')
+      @validate({


-1 for introducing another decorator @validate. IMO, developers should define validation rules as part of the OpenAPI Spec parameter definition.

@param.path('num1', {type: 'number', multipleOf: 5});

That way the OpenAPI spec of the application include all additional constraints imposed on method arguments.

If there are use cases for using @validate outside of REST/OpenAPI context, then I am fine with having @validate decorator, as long as @param decorators invoke the @validate decorator under the hood.

I agree with you. The PoC was to show that we can use @validate as a base for openapi-v3 decorators, and potentially any other specs like for GraphQL. I didn't have the time to work on the integration itself with openapi-v3, but that doesn't seem like something that's difficult to do (call @validate on converted spec from @param, and call @validatable after @operation)

bajtos · 2018-04-27T12:35:43Z

packages/validator/src/validate.decorator.ts

+    }
+    // tslint:disable-next-line:no-any
+    descriptor.value = async function(...args: any[]) {
+      const ajv = new AJV();


+1 for using AJV for validations.

To get the best performance, we need to cache compiled validators and reuse them across requests. Please consider this requirement in your design. See https://www.npmjs.com/package/ajv#performance and https://www.npmjs.com/package/ajv#compileobject-schema---functionobject-data.

bajtos · 2018-04-27T12:39:21Z

packages/rest/src/parser.ts

+          if (isSchemaObject(paramObject.schema)) {
+            // basic type
+            const serializer = getSerializer(paramObject.schema.type!);
+            coercedParamArgs.push(serializer.coerce(paramArgs[i]));


I have mixed feelings about rolling out our own coercion implementation. At one hand, it allows us to use the same coercion algorithm in multiple places (e.g. REST, ORM/database, etc.) and have full control over the coercion rules. At the other hand, there are many edge cases that are tricky to get right and we can easy end up with a behavior that's surprising for users because it's inconsistent with other coercion implementations (LB 3.x, AJV coercion).

Unfortunately, I don't think there is a popular coercion package that we can leverage to fit to our needs.

I feel coercion rules from string to whatever seem consistent for most coercion implementations. I like your suggestion of having a comprehensive testing suite in your main comment down below and I think it will make for a good starting point.

In this context, it's probably better to talk about deserialization. The responsibility is to extract typed/formatted values from http requests (a set of strings from path/query/header/body/...).

bajtos · 2018-04-27T12:41:35Z

packages/validator/src/validate.decorator.ts

+    // tslint:disable-next-line:no-any
+    descriptor: TypedPropertyDescriptor<(...args: any[]) => any>,
+  ) {
+    const originalMethod = descriptor.value;


I am not a fan of decorators that are directly modifying properties and methods of the decorated objects, I think the complexity of handling edge cases (like the number of arguments stored in .length property) is not worth the benefits.

What are the arguments for using this approach, as opposed to letting the REST layer to perform the actual validation based on metadata provided by @param and/or other metadata-only decorators?

My main point for using this approach is for the clean error tracing. Our sequence makes the error tracing irrelevant, but I personally thought having the arguments validated right as the controller method is invoked seemed like it just made sense at high level.

Another argument for it is that the decorators would be automatically compatible with any future server extensions. This approach would let extension developers implement validation for their servers just by having the users import the package.

As for the edge cases, the only ones I see aside from length are name and toString(). Are there any that I may have missed?

I see your points.

To me, replacing controller methods seems too magic, I prefer to have more control. As I was envisioning validation, different server extensions will call the validation step in their transport-specific sequence and each server extension will decide when is the right time to perform the validation and coercion.

Few things to consider:

Does it make sense to split coercion and validation into two steps? Are these steps really independent? Consider the REST sequence where parseParams action is responsible for extracting typed arguments from the HTTP request (and thus coerce the values from string sources like query string values to correct types). If the validation happens inside invoke action, then how are we going to treat invalid input values? Are we going to perform a partial validation as part of coercion, and then another validation as part of invoking controller method?

Let's say a controller method accepts an argument of type "unsigned 32bit integer" and the caller provided a string (e.g. foo) or a value that's out of bounds (e.g. -1 or 17e10). Using the default sequence (see below), what should parseParams return for this input argument? https://github.com/strongloop/loopback-next/blob/43383f5ff8ab98880e5a3bdb91d91c03e10c5ba5/packages/rest/src/sequence.ts#L106-L108

In my opinion, both coercion and validation should happen at the same time.

bajtos · 2018-04-27T12:56:57Z

Coercion is a tricky business and we have been bitten by many user-reported issues back in the LoopBack 2.x days. When I was reworking this area for LoopBack 3.x, I came to the conclusion that we need to apply different coercion algorithm depending on where the value comes from:

When the input argument value comes from a source that preserves (some) type information (typically JSON), then the argument should be converted with minimal coercion.
When the input argument value comes from a "sloppy" source (typically string-encoded source like a query string), then coercion rules should be applied.

For example, when coercing a Date from a JSON value, we may want to treat "0" (a string) and 0 (a number) differently. However, when the value is coming for a query string, then it's always a string, but we should probably treat it as if it was a number.

Consider also any type, which is typically used for primary key that can be either a number or a string (oneOf(number, string) in OAIv3). In LoopBack 3.x, we have smart logic to decide which query string & url path values should be recognized as a number and which should be preserved as strings. This is important for primary keys to work correctly, especially with MongoDB.

To implement a good type & coercion library, we need an extensive test suite covering all different edge cases. See the description in strongloop/strong-remoting#312 for a glimpse and all test files in strong-remoting/test/rest-coercion to see all edge cases to consider.

Other related issues that may be helpful here:

bajtos · 2018-06-07T11:22:26Z

@shimks is this pull request still relevant? Can we close it?

shimks · 2018-06-07T17:14:04Z

Nope, closing it

shimks requested review from bajtos and raymondfeng as code owners April 13, 2018 19:53

shimks added the needs-priority label Apr 13, 2018

0candy added review and removed needs-priority labels Apr 13, 2018

shimks mentioned this pull request Apr 13, 2018

[Spike] Validation and type conversion at REST/Model/Database layer #1057

Closed

shimks changed the title ~~[PoC - DO NOT REVIEW] very rough PoC on request parameter type coercion~~ [PoC - WIP] very rough PoC on request parameter type coercion Apr 16, 2018

shimks force-pushed the coercion-poc branch from f71f9e3 to 97e1a7c Compare April 16, 2018 22:20

shimks changed the title ~~[PoC - WIP] very rough PoC on request parameter type coercion~~ [PoC - WIP] very rough PoC on request parameter type coercion/validation Apr 16, 2018

shimks force-pushed the coercion-poc branch 2 times, most recently from 6b40a33 to 3f721fd Compare April 18, 2018 03:56

shimks requested a review from jannyHou as a code owner April 18, 2018 03:56

jannyHou reviewed Apr 23, 2018

View reviewed changes

shimks force-pushed the coercion-poc branch 2 times, most recently from 49d9b52 to f3dbd67 Compare April 23, 2018 19:54

shimks force-pushed the coercion-poc branch 2 times, most recently from 1bbba35 to afa4262 Compare April 24, 2018 15:24

raymondfeng and others added 5 commits April 26, 2018 11:04

feat: add @loopback/types to be the typing system

e5119da

feature: do a very short PoC on type coercion

f3b2b8e

feat: add tier 1 validation support

94977f9

add 2nd tier validation and helpful error messages

f41eebc

add valdation function support for @validator

b5ec200

shimks force-pushed the coercion-poc branch from afa4262 to b5ec200 Compare April 26, 2018 15:05

This was referenced Apr 26, 2018

[PoC] feat(rest): add type coercion for parseParams #1285

Closed

chore(repository-json-schema): replace JsonDefinition with JSONSchema6 #1286

Merged

bajtos reviewed Apr 27, 2018

View reviewed changes

shimks closed this Jun 7, 2018

bajtos deleted the coercion-poc branch October 4, 2018 06:49

[PoC - WIP] very rough PoC on request parameter type coercion/validation #1256

[PoC - WIP] very rough PoC on request parameter type coercion/validation #1256

Uh oh!

Conversation

shimks commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jannyHou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks commented Apr 23, 2018

Uh oh!

jannyHou commented Apr 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 27, 2018

Uh oh!

bajtos commented Jun 7, 2018

Uh oh!

shimks commented Jun 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shimks commented Apr 13, 2018 •

edited

Loading

jannyHou left a comment •

edited

Loading

shimks Apr 27, 2018 •

edited

Loading