Loopback-next: Required model property with default value throws error when property not provided in create

[aharbis]

Description / Steps to reproduce / Feature proposal

Given a model property that is defined as required with a default value, trying to create a new model instance and omitting that property from the request yields a 422 error stating the property is missing.

For example, given the following model property definition:

  @property({
    type: 'string',
    required: true,
    default: 'admin',
  })
  username: string;

Trying to create a model instance and omitting the username property yields:

{
    "error": {
        "statusCode": 422,
        "name": "UnprocessableEntityError",
        "message": "The request body is invalid. See error object `details` property for more info.",
        "code": "VALIDATION_FAILED",
        "details": [
            {
                "path": "",
                "code": "required",
                "message": "should have required property 'username'",
                "info": {
                    "missingProperty": "username"
                }
            }
        ]
    }
}

I've recreated the issue using the todo application, and you can view that on my forked feature branch:

https://github.com/aharbis/loopback-next/tree/default-property

I am using Node v10.15.3, and based the recreate on an up-to-date fork of loopback-next.

Current Behavior

The default value defined for the model property is not utilized, and instead a 422 error is thrown stating the required property is missing.

Expected Behavior

~If a required property has a default value, and the given property is not provided in the request to create the model instance, the default value should be used.~

Finalized agreement:
LoopBack follow the OpenAPI standard and make no interpretations.
required means the user needs to provide the field in the request always, independent of the default.

i.e default: xxx, required: true shouldn't be set at the same time.

Accpetance Criteria

• [ ] Update the docs Models.md, add explanations and examples
• [ ] Add validation to check if the two fields is set at the runtime

[aharbis]

It is worth noting that adding a default value to a non-required property does populate the default value if the request does not contain that property. I also demonstrated this behavior in the recreate - just see the added default value for isComplete, and added test validation for the default:

Non-required default property:

  @property({
    type: 'boolean',
    default: false
  })
  isComplete?: boolean;

Test case passes:

  it('default isComplete set if none provided during create', async () => {
    const todo = givenTodo();
    delete todo.isComplete;
    const response = await client
      .post('/todos')
      .send(todo)
      .expect(200);
    expect(response.body.isComplete).to.equal(false);
  });

[dougal83]

I could be wrong but I think if you set the field as required, it needs to be provided regardless of there being a default. If I recall correctly, this is not the same as 'not null' in a db. That being said if I'm mistaken and if you are using a data connector that supports schema then make sure you use the migrate tools so the defaults are in place to avoid not null.

[aharbis]

I could be wrong but I think if you set the field as required, it needs to be provided regardless of there being a default.

IIRC in my experience with LB3, I don't believe this was the case. In the LB3 app I maintain, we regularly would set a property to be both required and have a default. If I don't provide that property in the request, we got back the default property (as it was required). That was always my interpretation of the required / default combination.

[dougal83]

Ah fair enough. I've not used lb3 so I'm fairly taking things as is. I see
your point.

On Thu, 6 Jun 2019, 23:24 Aidan Harbison, [e-mail] wrote:

I could be wrong but I think if you set the field as required, it needs to
be provided regardless of there being a default.

IIRC in my experience with LB3, I don't believe this was the case. In the
LB3 app I maintain, we regularly would set a property to be both required
and have a default. If I don't provide that property in the request, we got
back the default property (as it was required). That was always my
interpretation of the required / default combination.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/strongloop/loopback-next/issues/3058?email_source=notifications&email_token=AAU36CLULTAVX7KKFHZX6F3PZGFCNA5CNFSM4HVKAMO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXEKWHY#issuecomment-499690271,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAU36CNYG4J32ALALBXEWJTPZGFCNANCNFSM4HVKAMOQ
.

[bajtos]

Thank you @aharbis for reporting this issue. I think I know what is causing the problem.

Let me start by quoting JSON Schema Validation:

10.2. "default"

There are no restrictions placed on the value of this keyword. When multiple occurrences of this keyword are applicable to a single sub-instance, implementations SHOULD remove duplicates.

This keyword can be used to supply a default JSON value associated with a particular schema. It is RECOMMENDED that a default value be valid against the associated schema.

And also https://json-schema.org/understanding-json-schema/reference/generic.html:

JSON Schema includes a few keywords, title, description, default, examples that aren’t strictly used for validation, but are used to describe parts of a schema.

None of these “annotation” keywords are required, but they are encouraged for good practice, and can make your schema “self-documenting”.

The title and description keywords must be strings. A “title” will preferably be short, whereas a “description” will provide a more lengthy explanation about the purpose of the data described by the schema.

The default keyword specifies a default value for an item. JSON processing tools may use this information to provide a default value for a missing key/value pair, though many JSON schema validators simply ignore the default keyword. It should validate against the schema in which it resides, but that isn’t required.

In other words, JSON Schema treats default only as a hint for the client, it's not taken into account during validation.

It's also important to realize that our code parsing JSON request bodies is not applying default values either. It's the ORM layer (loopback-datasource-juggler) that applies default values to properties with no value provided by the client.

Because JSON/OpenAPI validation is executed after JSON request is parsed but before the ORM layer can add default values, required properties with a default value are rejected.

I see two options how to address the problem:

1. Modify the code converting LoopBack Model definitions to JSON Schema, so that required properties with a default value are marked as optional in JSON schema.
2. Modify the code parsing JSON request bodies to apply default values before the object is handed over to validation.

I find the second option as more user friendly, because the default values will be already applied by the time the object reaches controller method, i.e. we can rely on default values inside controllers too. With the first option, controllers will see undefined properties, only repository methods will be able to fill default values.

Here is the place where we are parsing request bodies:

https://github.com/strongloop/loopback-next/blob/0d9698ab3bbad2754736b15b8e8880187c167c32/packages/rest/src/body-parsers/body-parser.ts#L65-L66

Please note the parsed body can be also a raw Buffer or a string, we need to take those cases into account and don't apply any default values in such case.

[aharbis]

I've been instrumenting the mentioned code above in the body parser to try and understand how the spec flows through this code path, and how to pull out that spec to apply defaults. It's still a work in progress, but wanted to jot down some notes.

Using the todo example app and leveraging the debug statements in the body parser, I've found that the schema lies in the RequestBody which comes in through the OperationObject.

  loopback:rest:body-parser OperationObject:  { 'x-controller-name': 'TodoController',
  'x-operation-name': 'createTodo',
  tags: [ 'TodoController' ],
  responses:
   { '200': { description: 'Todo model instance', content: [Object] } },
  requestBody: { content: { 'application/json': [Object] } },
  operationId: 'TodoController.createTodo' } +0ms

Digging into the request body after it's matched to the request body spec via _matchRequestBodySpec, I see the RequestBody as follows:

  loopback:rest:body-parser RequestBody: { value: undefined,
  mediaType: 'application/json',
  schema: { '$ref': '#/components/schemas/TodoExcluding[id]' } } +5ms

This is then what is passed into the Object.assign(requestBody, body) above, which applies the actual request content to this RequestBody instance.

So in order to handle the default value(s) here I need to find a way to resolve that $ref in the schema, and then apply the underlying spec, which should be relatively easy.

Really, the only use that I've seen that resolves this $ref is here:

https://github.com/strongloop/loopback-next/blob/9bbc9322ff576dc8edef74c3dd0ac2e566f8a6a7/packages/openapi-v3/src/controller-spec.ts#L257

and I'm not entirely sure that's what I want, as that mainly exists within resolveControllerSpec which is used to build the OpenAPI spec from the controller decorations (right?).

So I think my current blocker / obstacle is trying to understand how to resolve that $ref. Once I understand that, it's just a matter of applying the underlying schema (which will contain the defaults) to the requestBody.

[aharbis]

Going through the code base, trying to find how the schema is resolved and used during validation, as that seems like a logical place to look. I've found validateRequestBody in the request body validator, which calls validateValueAgainstSchema and passes in the same schema that I believe I'm referring to in my above comment:

https://github.com/strongloop/loopback-next/blob/c65b77f92468a9ae9948809bbcf24d25f05059f8/packages/rest/src/validation/request-body.validator.ts#L53-L61

however, this function seems to pass the schema into createValidator which returns an AJV validator object:

https://github.com/strongloop/loopback-next/blob/c65b77f92468a9ae9948809bbcf24d25f05059f8/packages/rest/src/validation/request-body.validator.ts#L129

I'm not see any example in the code that takes this schema and accesses the properties directly defined within - just cases that the schema is passed into a validator and run against a body or some content.

The search continues..

[bajtos]

I'm not see any example in the code that takes this schema and accesses the properties directly defined within - just cases that the schema is passed into a validator and run against a body or some content.

I think that's kind of expected. As I wrote earlier: _It's also important to realize that our code parsing JSON request bodies is not applying default values either. It's the ORM layer (loopback-datasource-juggler) that applies default values to properties with no value provided by the client._

The ORM layer is working with LB model definitions that are different from JSON Schema, that's why there is no code accessing individual properties as defined in JSON schema.

Personally, I would start the by modifying the following code:

https://github.com/strongloop/loopback-next/blob/26bf90708f623106c6777c6f834086f3eb195c1c/packages/rest/src/parser.ts#L90-L91

into something like this:

debug('Applying default values to request body', body);
applyDefaultValues(body, operationSpec.requestBody, globalSchemas, options);

debug('Validating request body - value %j', body);
validateRequestBody(body, operationSpec.requestBody, globalSchemas, options);

The function applyDefaultValues should iterate over all properties defined in the schema and use default field to set missing properties in the supplied body object.

As the next step, it would be great to apply default values to parameters coming from other sources, e.g. from the query string.

[gczobel-f5]

I could be wrong but I think if you set the field as required, it needs to be provided regardless of there being a default. If I recall correctly, this is not the same as 'not null' in a db. That being said if I'm mistaken and if you are using a data connector that supports schema then make sure you use the migrate tools so the defaults are in place to avoid not null.

I agree with @dougal83. As far as I understand, in OpenAPI is possible to set a property as nullable. So, a single property can be nullable, default and required when each of them means:
nullable: the value can be null in the DB.
default: if a value is not specified will be assigned.
required: must be sent in the request.

There is no rule about if default and required contradict one each other. Because OpenAPI is the canvas where the user wrote a contract.

In some apps, I can use required and default together (using default as a predefined value in UI and required to be sure the user fill this value)

IMO, LoopBack should follow the OpenAPI standard and make no interpretations.
TL-DR: required forces the user to provide the field in the request always, independent of the default.

[agnes512]

Finalized agreement:
LoopBack follow the OpenAPI standard and make no interpretations.
required means the user needs to provide the field in the request always, independent of the default.

i.e default: xxx, required: true shouldn't be set at the same time.

Accpetance Criteria

• [ ] Update the docs Models.md, add explanations and examples
• [ ] Add validation to check if the two fields is set at the runtime

[lukeggchapman]

There is another problem with not supporting default + required.
The required prop is used to build the schema for the returned type also, not just the input type.
So if your property has default (or generated) and is not required then the return type will show the value as optional, when it is not the case.

If we do not want to support generating or defaulting prior to validation, is there some way for us to indicate that the model will have additional required props when fetching? Perhaps a way to set additional props as required when calling getModelSchemaRef ?