Skip to content
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

Complex OpenAPI Validations with @property #1624

Closed
1 of 5 tasks
virkt25 opened this issue Aug 20, 2018 · 16 comments
Closed
1 of 5 tasks

Complex OpenAPI Validations with @property #1624

virkt25 opened this issue Aug 20, 2018 · 16 comments

Comments

@virkt25
Copy link
Contributor

virkt25 commented Aug 20, 2018

Description / Steps to reproduce / Feature proposal

LoopBack's @requestBody decorator is supposed to be capable of validation complex OpenAPI Schemas such as:

{
  type: string,
  minLength: 10,
  format: 'email'
}

This only works when this schema is passed to the spec via the @model decorator (top-down approach) ... vs. the more common route / CLI recommended path of using the @property decorator.

If you add validation properties such as minLength, format, etc. to a property using the @property decorator the validation properties don't even show up in /openapi.json and as such are never validated against the requestBody since those validations aren't a part of the schema.

Current Behavior

  • Can't add validation properties using the @property decorator.

Expected Behavior

  • Should be able to add model validation properties to a property via the @property decorator.

Acceptance Criteria

  • @property() decorator should accept additional validation properties as per the OpenAPI Spec and add them to the schema of the Model (should show in /openapi.json) - see Refactor metaToJsonProperty to accept AJV keywords #2685
  • Incoming @requestBody() should honour validation properties added via @property decorator.
  • Confirm @model() also works in preserving the validation properties
  • Update any relevant docs (if needed)
  • Tests

See Reporting Issues for more tips on writing good issues

@bajtos
Copy link
Member

bajtos commented Aug 27, 2018

+1 for allowing models to provide additional validation rules. I don't think this is as simple as allowing @property decorator to receive arbitrary OpenAPI spec constraints. @property is using LoopBack Definition Language (the schema format used by juggler & LB 3.x).

IMO, we need to find a (generic) solution that will support other transports than just REST/OpenAPI, for example gRPC and GraphQL and ideally allow connectors to setup some of these restrictions at the database level too. For example, maxLength can be leveraged to specify the storage size of a column (maxLen: 5 means the column can be defined as nvarchar(5)).

See also #699

@bajtos
Copy link
Member

bajtos commented Aug 27, 2018

IMO, this is beyond 4.0 GA scope.

@bajtos bajtos added post-GA and removed LB4 GA labels Aug 27, 2018
@virkt25
Copy link
Contributor Author

virkt25 commented Aug 27, 2018

Well ... shouldn't it be up to the extension implementing GRPC / GraphQL to determine what it wants to do with the metadata. I don't think it's feasible for us to have a common way of supporting all of these protocols.

This issue came from me working on loopback4-example-shopping where we can avoid a lot of custom validation on a per route basis by simply supporting such validations through the @property layer so the information gets passed to Ajv for validation. Some of this may / may not be needed for GRPC / GraphQL (or they might require additional metadata) and it's up to those extensions to decide how they want to deal with the metadata imo.

@bajtos
Copy link
Member

bajtos commented Sep 3, 2018

Well ... shouldn't it be up to the extension implementing GRPC / GraphQL to determine what it wants to do with the metadata. I don't think it's feasible for us to have a common way of supporting all of these protocols.

This issue came from me working on loopback4-example-shopping where we can avoid a lot of custom validation on a per route basis by simply supporting such validations through the @Property layer so the information gets passed to Ajv for validation. Some of this may / may not be needed for GRPC / GraphQL (or they might require additional metadata) and it's up to those extensions to decide how they want to deal with the metadata imo.

First of all, I see the pain point you have described and agree LB4 should provide an easy solution for that.

My objection against adding REST-specific validations to @property decorator is that @property is not tied to REST transport, it's a generic model-level decorator that should work with any and all transports. IMO, by using JSON-Schema language in @property decorator, we would signal to LB4 users that all such JSON-Schema validation rules are supported by all parts of the framework (ORM, different transports, etc.), which is not true.

For example, gRPC uses Protocol Buffers to specify transport format, translating validations specified as JSON schema to Protocol Buffers may be difficult if possible at all (Protocol Buffer have very minimal validation rules).

I would like to propose to define an extension format allowing @property metadata to include transport-specific validations. This can be similar to what we already have for database-specific configuration (e.g. column mapping).

Then we can define a convention for leveraging the extension format to provide additional JSON-Schema validations in property definition; and finally modify the code translating LB models to JSON Schema to take these extensions into account.

An example showing a jsonSchema extension specifying email format:

{
  type: string,
  minLength: 10,
  // the LDL extension: jsonSchema
  jsonSchema: {
    // JSON-Schema constraints follow
    format: 'email'
  }
}

Optionally, we can also review OpenAPI constraints, pick those that seem to be "universal", promote them to top-level LDL (LoopBack definition language) and implement support for them in juggler/connectors/transports. For example, format: email is a common validation rule that we have already implemented in LoopBack 3.x core:

https://github.com/strongloop/loopback/blob/c9164aed1794defbf7eaeafa797567ee74658507/common/models/user.js#L1398-L1407

function emailValidator(err, done) {
  var value = this.email;
  if (value == null)
    return;
  if (typeof value !== 'string')
    return err('string');
  if (value === '') return;
  if (!isEmail.validate(value))
    return err('email');
}

https://github.com/strongloop/loopback/blob/c9164aed1794defbf7eaeafa797567ee74658507/common/models/user.js#L1254-L1256

    UserModel.validate('email', emailValidator, {
      message: g.f('Must provide a valid email'),
    });

As I am thinking about this more, my conclusion is that for a validation rule to be eligible for including in top-level property definition (in LDL), we must have the validation implemented at juggler level, because that's the place where all data operations must go through. If a validation is not implemented at transport level (e.g. because gRPC/ProtocolBuffers do not allow that), then it's ok, because the values will be still validated by repository (juggler) operations.

On the other hand, if a validation is implemented at transport level only, then we have a problem - code working with models/repositories directly via TS/JS API is effectively bypassing such validation rules! For example, test code or database seeding script can easily created invalid model instances.

In that light, I am actually not sure if JSON-Schema validations applied at REST transport level are actually the proper solution for avoiding a lot of custom validation on a per route basis.

Thoughts?

@raymondfeng Please join our discussion, what's your opinion on this matter?

@virkt25
Copy link
Contributor Author

virkt25 commented Sep 5, 2018

Hmm, I see the point you're making. I'm ok if we did the validation in Juggler as long as we do it somewhere. That said I think my biggest concern is some inconsistency within the framework right now where by @requestBody() does the validation against the OpenAPI spec for the Model ... and if someone passes in the spec to @model or overrides it via app.api() and it includes these validations, they will be enforced by the framework but @property won't allow such validations -- my main intent is that we should be consistent with our approach and ideally that empower such validations because we can for free using AJV.

IMO the decorators should support storing the openapi validations and allow other layers of the framework to leverage the metadata to see if they wish to support the validation or not. I get the concern for allowing transport specific validations / juggler validations ... and for that I would propose the following:

  • Support decorators storing validations by transport @property({type: 'string', rest: {format: 'email'}}) ... documenting it only applies to REST validation.
  • Eventually move validation support from REST layer to juggler and we can eliminate the transport specific validation OR do this now ... and say juggler supports OpenAPI style validations.

@David-Mulder
Copy link
Contributor

@bajtos If my understanding of what you're proposing is correct that means that there would be different extensions to the LDL which would do the validation on the ORM-level regardless of the transport. Is that correct?

Because if that's what you're proposing that sounds like a good idea 👍 .

Right now I am going to take a leap of faith/hope and include the validation in the property decorators according to the

{
    jsonSchema: {}
}

specification and do the validation in the frontend and hope that that syntax will get to be supported later on in loopback 4 itself through some extension that one needs to enable.

Given proper instruction I would be very open to helping contribute to such a solution.

@bajtos
Copy link
Member

bajtos commented Oct 26, 2018

If my understanding of what you're proposing is correct that means that there would be different extensions to the LDL which would do the validation on the ORM-level regardless of the transport. Is that correct?

Yes, that's what I would like to see!

Right now I am going to take a leap of faith/hope and include the validation in the property decorator and do the validation in the frontend and hope that that syntax will get to be supported later on in loopback 4 itself through some extension that one needs to enable.

I think this should be relatively straightforward to implement. The package repository-json-schema is responsible for converting model definitions in LoopBack format into JSON schema documents.

Here is the code building JSON Schema definition for a model property:

https://github.com/strongloop/loopback-next/blob/f9d708978cdd11483cd5d839a4e65342c68ce16d/packages/repository-json-schema/src/build-schema.ts#L88-L131

Now if my understanding of this part of our codebase is correct, then we simply need to add few lines of code to metaToJsonProperty to copy properties provided in jsonSchema of the model property definition into the JSON Schema object returned by metaToJsonProperty. Something along the following lines:

Object.assign(propDef, meta.jsonSchema);

Besides this tiny code change, the pull request need to include tests and documentation.

@David-Mulder I am happy to help you along the way if you decide to contribute this feature. See our CONTRIBUTING guide to get started.

@David-Mulder
Copy link
Contributor

@bajtos Well, I added that "hack" (just because of it temporary nature) in myself already yeah (plus some code in the sequence.ts file to add the schema as a header to the response), but the part which I don't know how I would have to design it is where I should hook in such a validation extension. The way I am right now envisioning this would be a system where you can load any validation library you wish and it will consume whatever things it needs from the PropertyDefinition. The main thing that confuses me is that obviously the ORM is reponsible for such validation, however as I understand it Juggler right now still is going to be refactored greatly?

Anyway, even once it would be possible to develop validation extensions it will be necessary to allow different options for JSON Schemas, as every implementation is different. In my case I am for example using ajv which allows refering other data in the JSON for validation purposes which isn't a part of the JSON Schema spec, but definitely is amazing.

@dhmlau dhmlau removed the post-GA label Nov 2, 2018
@bajtos
Copy link
Member

bajtos commented Nov 23, 2018

the part which I don't know how I would have to design it is where I should hook in such a validation extension. The way I am right now envisioning this would be a system where you can load any validation library you wish and it will consume whatever things it needs from the PropertyDefinition.

Hmm, this is tricky. PropertyDefinition is decoupled from REST-level validations, we have conversion steps that convert PropertyDefinition into JSON Schema and then into OpenAPI Schema. The REST layer sees only the OpenAPI Schema for each parameter, it does not have access to the original Property/Model definitions.

To allow a validator to consume custom PropertyDefinition properties, we would need a different way of converting model/property definitions into OpenAPI Schema.

In order to plug in custom validations at REST API layer, I think we haven't fully considered this aspect yet. I think the easiest option is to leverage Dependency Injection and allow applications to plug in a custom validator. Example usage:

app.bind(RestBindings.VALIDATOR).to(CustomParameterValidator);

Here is the entry point where the pluggable validator would be used instead of the current hard-wired implementation:

https://github.com/strongloop/loopback-next/blob/b9570c8f9e6b5b9e57271605e8e132b5815a2198/packages/rest/src/parser.ts#L225

Anyway, even once it would be possible to develop validation extensions it will be necessary to allow different options for JSON Schemas, as every implementation is different. In my case I am for example using ajv which allows refering other data in the JSON for validation purposes which isn't a part of the JSON Schema spec, but definitely is amazing.

We are using ajv inside @loopback/rest too. Personally, I am fine to keep using ajv as the built-in JSON Schema validator, and using the Dependency Injection-based approach described above as an extension point allowing developers to provide a different JSON Schema validator (or even a completely different engine like joi).

Obviously, this is all about validation at REST API level. Customizing validations at juggler/repository level is a whole new story.

@clayrisser
Copy link
Contributor

Currently what is the best approach for validating something like an email?

@bajtos
Copy link
Member

bajtos commented Dec 3, 2018

Validating email addresses is tricky. We have had good experience with isemail module, I would recommend it to do the actual validation.

I don't have a good answer for how to wire up a custom validation in LB4 right now. I think we will need to improve LoopBack first and define an extension point allowing applications to register custom validations, see the discussion above for few ideas.

@gleme
Copy link

gleme commented Apr 22, 2019

Validating email addresses is tricky. We have had good experience with isemail module, I would recommend it to do the actual validation.

I don't have a good answer for how to wire up a custom validation in LB4 right now. I think we will need to improve LoopBack first and define an extension point allowing applications to register custom validations, see the discussion above for few ideas.

Any updates on that? I really appreciated the idea of maintaining all the validation at ORM level, so it is better to write integration tests that accomplish the expected values in the database. That helps a lot in cases where people write ETL scripts to extract data from CSV files or another sources, to persist these data using Loopback repository API. 😀

@mamiller93
Copy link

Or what's the best way of validating a model that isn't a request body endpoint? Using the isemail module is fine for email but a generic validator would be much more helpful, no?

@dhmlau
Copy link
Member

dhmlau commented Aug 20, 2019

In discussion with @raymondfeng @bajtos:
We've added some enhancement recently in adding AJV keywords, see PR #3539.

Another way to do validation is through interceptors.

@mamiller93
Copy link

@dhmlau for us, the payload of our endpoint is encoded which would make the AJV validation difficult.

Instead, we did exactly what you recommended - created a method decorator that does the AJV validation for us, with similar defaults to the OpenAPI validation, and throws an error if it isn't valid.

@dhmlau
Copy link
Member

dhmlau commented Mar 27, 2020

According to the above comment, closing this issue as done.
Validation example created: https://github.com/strongloop/loopback-next/tree/master/examples/validation-app.
Docs are added via PR #4874 (merged now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants