Loopback-datasource-juggler: No strict property validations in update operations while extendedOperators active

[mitsos1os]

Description/Steps to reproduce

When using the MongoDB Connector, and having enabled the option allowExtendedOperators most property validations are skipped. For example let's say that I have a Model with strict schema:

{
  id: "Number",
  propA: "String",
  propB: "String"
}

Issuing an update operation (either updateAll or patchAttributes) on the model with a non existent property such as model.patchAttributes({ propC: 'something' }) will not take into account the strict setting of the model and skip property validation due to the existence of allowExtendedOperators and save the non existent property in the database. This takes place in this line.

Expected result

This should be taken into account somehow in order to be able to distinguish between non existent properties and operators. Maybe this could be done in isValid of model where it only checks the results of __unknownProperties which are generated from applyStrictCheck

[dhmlau]

@mitsos1os , just wanna make sure I understand your request. Currently if propC is included in the updated json, the operation is performed successfully and propC does not get persisted.

Are you suggesting that the update operation should not be successful because there are unknown properties?

[mitsos1os]

@dhmlau That's exactly the problem. The operation completes successfully and propC does get persisted when it shouldn't. It should throw a ValidationError

[dhmlau]

Interesting. When I tried it, the extra field did not get persisted, though there is no error.
I'm using

[email protected]
[email protected]

Would you like to submit a PR?

[mitsos1os]

I will have to try this tomorrow since I am out of office. I do not think it has anything to do with the MongoDB connector since if it passes the validation from the juggler the update object is passed on to the mongodb connector.

[mitsos1os]

I have just downloaded the newest version and tried... Same result... property gets persisted...
Issuing a pull request will require some conversation on how to implement this. Because the juggler DAO does not have any knowledge to what the underlying db-connector uses as extendedOperators in order to separate from undefined model properties in update object

[dhmlau]

I think we want to call applyStrictCheck even if allowExtendedOperators is true. The problem is that the validation would fail because those extended operators are not really in the model.
My suggestion would be inside applyStrictCheck, ignores the key when it begins with $.
https://github.com/strongloop/loopback-datasource-juggler/blob/f1d10b47cec29b5a4e56d417b5992e999d57011d/lib/dao.js#L88

Does it make sense?
What do you think, @jannyHou ?

[mitsos1os]

@dhmlau I have thought of that, but this takes as granted the all extendedOperators follow the pattern of starting with $ which I am pretty sure is MongoDB specific... I don't know how this would comply with the logic that the juggler does not handle any db-connector logic

[dhmlau]

You're right. I have the same concern too. I was thinking that only mongodb connector supports extended operators, so it might be safe to assume all extended operators begin with $.

On a second thought, I'm thinking...

• Expose the acceptedOperators in the connector
• Instead of assuming what the extended operators look like in the juggler, we get the list from getConnector().getAcceptedOperators()

[mitsos1os]

If this is verified then I believe it would be ok to consider this as the proper way to go on

[dhmlau]

@jannyHou , any thoughts?

[jannyHou]

Thank you @dhmlau for explaining and discussing the issue with me, it's awesome to support operators when model's strict is true, but I am afraid the flag allowExtendedOperators might not be sweet enough to satisfy all scenarios, eg. the use case in this issue.

From my understanding a field in a mongodb query is not the same concept as a loopback model property, so when we want a mongodb query includes operator

{ 
    name: 'jim',
    age:15,
    $set: {
        name: 'john'
    }
}

It doesn't mean juggler has to provide connector a data like this. It would be better if we can provide the operators in a loopback api's options but not data, since data is supposed to only contain model properties.

And it's a bit tricky to use an operator flag controls model property level behaviours. For example, if we provide data contains both operator and invalid property but begin with $:

{ 
    name: 'jim',
    age:15,
    $set: {
        name: 'john'
    },
    $dangerousProperty: 'destroy'
}

juggler itself could not tell which one is the real operator, it has to call some function in mongodb to validate it, hopefully https://github.com/strongloop/loopback-connector-mongodb/blob/master/lib/mongodb.js#L510 could do this.

There are two proposal in my mind:

1. provide the operator in options:

Model.udpateAll(
  {where: cond},
  {
   property1: '1',
   property2: '2'
  },
  {
    operatorData: {
       // mongodb connector will check `operatorData` in options, 
       // then append this object to 
       // the data passed into mongodb query
    }
  }
)

1. Create a connector function, temporarily call it getExceptionProperties(), which returns a list of properties that don't exist in model's definition file but still be allowed when strict is true. And when we do the strict check in juggler, call that function and skip the check for those properties.
   This method may still need that flag to control whether a user want to skip the check or not, and to be honest I am tending to avoid such a flag control if possible.

[mitsos1os]

@jannyHou Thank you for joining the conversation.
Regarding the MongoDB update operations. MongoDB update documents consist solely of update operators.
The simplest update operator is $set where it just sets the value of the property. Using just a document with properties and values is a shorthand for assigning the object to a $set operator which mongodb does this automatically for us. ex:

{
  name: 'Jim',
  age: 15
}

is a shortcut which gets extended to:

{
  $set: {
    name: 'Jim',
    age: 15
  }
}

So you cannot have both root level properties and also update operators.
Also I am not sure, but MongoDB does not allow property names starting with a $ so there is no way that it will have the case you mention above.
The problem with all this logic is that is strictly tied to MongoDB so you cannot apply it globally to all connectors.

What I think is the best solution is for the connector to also provide a getExtendedOperators as you suggested, to be able to apply strict validation in the juggler for every connector without trying to guess or enforce a single connector logic.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.