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

feat(repository): rejects CRUD operations for navigational properties #4148

Merged
merged 1 commit into from
Dec 4, 2019

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented Nov 19, 2019

implements #3439.

please review it with "Hide whitespace changes"

Add two new protected methods:
DefaultCrudRepository.ensurePersistable and DefaultCrudRepository.entityToData.

This new function checks create*(), update*(), save(), delete*(), and replaceById() methods. Throws when the target data ( the one we are going to create, update) contains navigational properties:

target data: {id: 1, name: 'cusotmer', orders: [{id: 1...}]}
  • in the ensurePersistable, the //FIXME from @bajtos says mongoDB would break replaceById with ObjectID issue if we apply ensurePersistable to replaceIdBy.
    By debugging, I think it's caused by replacing the id project (ObjectID type) to a string type id. So in my proposal, I remove the id property from the target data for replaceById method in ensurePersistable since we don't need it anyway.
    I decide to open a follow up issue to handle the issue cause it relates to dao.js and mongo.js files, which is not a easy fix for me according to my knowledge of the code base 😶

  • tests for create/update with nav properties are added to
    legacy-juggler-bridge.unit.ts (for basic functionality) and
    has-many.relation.acceptance.ts ( against real DBs).

  • tests for replacement(save, replaceById) are added to
    has-many-inclusion-resolver.relation.acceptance.ts to check if the new instance can be found with inclusion ( these tests are skip for Cloudant as it needs _rev token).

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 👈

Followups

Commented the objectId issue in #3456
Simplify fixtures and tests #4249
Enable replaceById + inclusion tests run on Cloudant #4251

@agnes512 agnes512 force-pushed the reject-navigational branch 5 times, most recently from 2fba490 to 9043d23 Compare November 20, 2019 19:47
@agnes512 agnes512 marked this pull request as ready for review November 20, 2019 20:25
@agnes512 agnes512 force-pushed the reject-navigational branch 3 times, most recently from 92a6e0d to 8a7f78d Compare November 21, 2019 15:10
@bajtos
Copy link
Member

bajtos commented Nov 22, 2019

Add a new protected method DefaultCrudRepository.ensurePersistable.
( I didn't use name fromEntity cause I get confused easily with the name while doing the task..)

+1 to find a better name than fromEntity.

Originally, I wanted to introduce fromEntity as something that's useful beyond validation of navigational properties, it has the potential to serve as a solution for LB3 hook persist.

The idea is to have a protected Repository method that's called whenever a CRUD method needs to convert data supplied by the user to data passed down to juggler. This new method can modify the data (e.g. by returning a new data object), but also perform validations (e.g. reject invalid data by throwing an error).

It makes me wonder, is this something we should aim for as part of this work?

const data: AnyObject = new this.entityClass(entity);

const def = this.entityClass.definition;
for (const r in def.relations) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way how we detect the navigational property is reasonable to me 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improvement to consider: not the scope of this PR, if the same check applies to all CRUD functions, maybe we should define it as an interceptor?

);
}
}
if (options.replacement === true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why do we need to deal with the idProperty in ensurePersistable? I don't think for replacement people can include the id from input, we should prevent the id field by excluding it from the request body schema defined in controller.

See the type used for request body, I think the solution should be changing it to sth like

@requestBody({
      content: {
        'application/json': {
          schema: getModelSchemaRef(Todo, {title: 'NewTodo', exclude: ['id']}),
        },
      },
    })
    todo: Omit<Todo, 'id'>,

Copy link
Contributor Author

@agnes512 agnes512 Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested out the code with @bajtos 's fix of ObjectId: loopbackio/loopback-connector-mongodb#553. I think I can remove the options.replacement part cause it's fixed 👏

And yes I agree that id should be excluded from update/replace methods at both controller and repository level. Currently, update/replacement methods take id property and ignore it in the operation. The result is correct but I think it should throw errors when id is included.

So I am thinking, as Miroslav suggested above, the new function can perform validations such as checking nav properties, and checking if id is included for update/replace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested out the code with @bajtos 's fix of ObjectId: loopbackio/loopback-connector-mongodb#553. I think I can remove the options.replacement part cause it's fixed 👏

Awesome!

And yes I agree that id should be excluded from update/replace methods at both controller and repository level. Currently, update/replacement methods take id property and ignore it in the operation. The result is correct but I think it should throw errors when id is included.

I believe it was an intentional design to require id property to be present in the payload of replaceById request. This operation does exactly what is says - replaces the entire model instance in the database with the supplied data. This is important when free-form properties come to play, because it's not possible to remove a property via patch calls. The call patchById(id, {someProp: undefined}) is a no-op, because undefined cannot be represented in JSON.

I feel it's also better UX when the id is allowed in the model data sent to both patch and replace methods. Clients often store the model instance in a variable, then make some change to some properties (e.g. via UI) and then want to persist that changes. If the PK property is forbidden in the payload, then the clients must do extra work to remove it.

@jannyHou
Copy link
Contributor

@agnes512 The added tests LGTM in general(I will do a line by line review later), left two comments about the design of ensurePersistable:

#4148 (comment) feel free to improve it in a separate story or at least a separate PR

#4148 (comment) can be done in a separate story.

@jannyHou
Copy link
Contributor

Originally, I wanted to introduce fromEntity as something that's useful beyond validation of navigational properties, it has the potential to serve as a solution for LB3 hook persist.

@bajtos Could you elaborate more about ^? are you looking for something like an ensurePersistant interceptor? my related comment #4148 (comment)

@agnes512 agnes512 force-pushed the reject-navigational branch 3 times, most recently from af3410e to da0fdc4 Compare November 22, 2019 21:16
@bajtos
Copy link
Member

bajtos commented Nov 25, 2019

Originally, I wanted to introduce fromEntity as something that's useful beyond validation of navigational properties, it has the potential to serve as a solution for LB3 hook persist.

Could you elaborate more about ^? are you looking for something like an ensurePersistant interceptor? my related comment #4148 (comment)

The use case I have in mind is storing certain properties as encrypted in the database, see #2095.

  • Inside toEntity method, we can decrypt the value fetched from the database and store plain-text value in the Entity property.
  • Inside fromEntity method, we can encrypt the plain-text value stored in the Entity property and send encrypted value in the "raw data" sent to to underlying DAO/connector methods.

skipIf(
!features.hasRevisionToken,
it,
'returns inclusions after running save() operation',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please provide more details on why are these new test cases skipped? IIUC, you are running these tests only for CouchDB/Cloudant and skipping them for all other databases (memory, PostgreSQL, MongoDB). The code in the test looks legit to me, I find it suspicious that it does not work on so many database types.

Ideally, I'd like to find a way how to enable these tests for all databases we support.

If that's not possible or if it requires too much effort, then I would like you to add code comments near !features.hasRevisionToken line to explain why each test is skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should run on all databases except Cloudant cause it needs the _rev property. I will add comments to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agnes512 thank you for replying in time, I am running these 2 tests and I think I understand why they fail for Cloudant and mongodb, fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos The test case here skips Cloudant connector because the Customer model in fixtures doesn't have _rev property defined.

skipIf(features.hasRevisionToken, testcase){//tests}means when features.hasRevisionToken is true(which only applies for the cloudant connector), the test will be skipped.

I am not very familiar with the shared test case, so agreed with Agnes's solution,
if you want to enable same tests for cloudant, I can add them in a separate PR (within same story).

Copy link
Contributor

@jannyHou jannyHou Nov 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos The test fails for mongodb because the id type is object not string, therefore fails at the check in https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L2621, hmm...any suggestion on how to fix it?


Update, just talked to Agnes, she points me to the fix code commented in https://github.com/strongloop/loopback-next/blob/49ecf1b86f058576404f28c88d4650651fc31419/packages/repository/src/repositories/legacy-juggler-bridge.ts#L580-L585 :)

Do you agree on the solution? If it is acceptable I will recover this code. (Then I believe all tests will pass)

@jannyHou
Copy link
Contributor

jannyHou commented Nov 25, 2019

@bajtos Thank you for the explanation. I am looking at your original proposal as

class DefaultCrudRepository<T, ID> /*...*/{
  protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    // "persist hook" - no-op by default
    return entity;
  }

  protected async dataToEntity(data: DataObject<T>, options?: Options): DataObject<T> {
    // "load hook" - no-op by default
    return this.toEntity(data);
  }

  async create(entity: DataObject<T>, options?: Options): Promise<T> {
    const data = await this.entityToData(entity, options);
    const model = await ensurePromise(this.modelClass.create(data, options));
    const result = await this.dataToEntity(model);
    return result;
  }

  // etc.
}

Do you mean we implement no-op entityToData and dataToEntity in DefaultCrudRepository then add Agnes's navigation property check in generated Repositories by CLI?

// Check navigation properties
// This check will be added in EACH created Repository that extends `DefaultCRUDRepository`
// as the built-in implementation for `entityToData`
// If developer wants to custom the logic how would they do it? Create a custom
// `BaseCRUDRepository` with their implementations of `entityToData` and `dataToEntity`, 
// then make the created Repositories extend the `BaseCRUDRepository`?

protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    const data: AnyObject = new this.entityClass(entity);

    const def = this.entityClass.definition;
    for (const r in def.relations) {
      const relName = def.relations[r].name;
      if (relName in data) {
        throw new Error(
          `Navigational properties are not allowed in model data (model "${this.entityClass.modelName}" property "${relName}")`,
        );
      }
    }
}

@bajtos
Copy link
Member

bajtos commented Nov 25, 2019

Do you mean we implement no-op entityToData and dataToEntity in DefaultCrudRepository then add Agnes's navigation property check in generated Repositories by CLI?

I don't think the navigation property check belongs to generated repositories, IMO this check should be executed by DefaultCrudRepository.

When I was writing those comments for entityToData and dataToEntity, I was not aware that these methods can be useful for checking the presence of navigational properties.

I think we should change the default implementation of entityToData from no-op to a check rejecting navigational properties. Something along the following lines:

class DefaultCrudRepository<T, ID> /*...*/{
  // "persist hook"
  protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    ensurePersistable(entity, options);
    return entity;
  }

  // ...
}

Please note the name entityToData may not be the best one, especially considering that we already have toEntity method.

@jannyHou
Copy link
Contributor

class DefaultCrudRepository<T, ID> /*...*/{
  // "persist hook"
  protected async entityToData(entity: DataObject<T>, options?: Options): DataObject<T> {
    ensurePersistable(entity, options);
    return entity;
  }

  // ...
}

Cool thank you 👍 now I understand your plan

@bajtos
Copy link
Member

bajtos commented Nov 28, 2019

This is tricky, I'll need to find more time to look into the issue in more depth.

My initial feeling is that we are trying to work around a flaw in the way how repository code and the shared test suite is designed. Instead of adding more and more workarounds, I would prefer to find a better design that will not require these hacks.

in the ensurePersistable, the //FIXME from @bajtos says mongoDB would break replaceById with ObjectID issue if we apply ensurePersistable to replaceIdBy.
By debugging, I think it's caused by replacing the id project (ObjectID type) to a string type id. So in my proposal, I remove the id property from the target data for replaceById method in ensurePersistable since we don't need it anyway.

Ideally, when we run the test suite against MongoDB, all PK and FK values should be always strings. This is typically achieved by defining the properties as {type: 'string', mongodb: {dataType: 'ObjectID'}} (plus id, required and generated flags). Unfortunately, when a PK property is generated, LoopBack ignores the user-provided type and uses the type defined by the connector. We have recently added a flag to disable that behavior, see loopbackio/loopback-datasource-juggler#1783. What happens when we enable that flag in repository-tests models - will it solve the issues we are observing here?

The test case here skips Cloudant connector because the Customer model in fixtures doesn't have _rev property defined.

This is similar to the discussion we had in #3968, see #3968 (comment) IMO, we should find a way how to modify Customer model to include _rev property when testing Cloudant.


IMO, the current way of setting up test models in repository-tests is no longer supporting the needs of different connectors.

  • MongoDB connector wants to set mongodb: {dataType: 'ObjectID'}.
  • Cloudant wants to add _rev property
  • SQL connectors want to use number as the type for primary & foreign keys. NoSQL connectors want to use string type.

Ideally, CrudFeatures should provide generic extension points allowing connectors to supply additional model/property configuration that will be applied by the shared test suite.

One of the problems I see in the current setup is that models used by relations are hard-coded in TypeScript source files (see e.g. packages/repository-tests/src/crud/relations/fixtures/models/address.model.ts). This makes it difficult to create a model that's tailored to the connector being used. See for example how we are using MixedIdType and modifying PK property metadata in the tests.

IMO, our life would be much easier if model classes were created dynamically by a factory function, as can be see in the non-relation tests - e.g. here:

https://github.com/strongloop/loopback-next/blob/1de6e122da3b27190dc8a08b739f0e484470d524/packages/repository-tests/src/crud/create-retrieve.suite.ts#L29-L53

Anyhow, I understand such change may require more effort that we want to invest right now. Even if we decide to do it, it belongs to a new pull request, not here.

In that light, let's try to find a way how to patch the current test setup so that we don't have to implement workarounds in the production code, and instead have the test models use the right model settings & property definitions, ones that our users should apply too.

I think that means we need to add _rev property to the Customer model in the fixture. I am not happy to use _rev when testing SQL databases, but I am ok to accept it as a temporary workaround for the limitations of current test fixture setup. We also need to fix definition of PK/FK properties and/or enable useDefaultIdType, so that PK/FK properties are always converted to string when testing MongoDB. The last step is to open a follow-up issue to refactor the shared test suite to better support needs of different connectors.

Thoughts?

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress 👏

const reheatedPizza = toJSON(pizza);

// coerce the id for mongodb connector to pass the id equality check
// in juggler's replaceById function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still necessary? I believe that if we configure all PK properties with {useDefaultIdType: false, mongodb: {dataType: 'ObjectId'}, then juggler will take care of the conversion from ObjectID to string and vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed const reheatedPizza = toJSON(pizza);

found.name = 'updated name';
found.orders.push(wrongOrder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a requirement to modify find.orders array? Shouldn't the check be triggered even when the array remains the same as returned by customerRepo.findById? If the behavior is different depending on whether the array was changed or not, then we should have two tests to cover both variants.

@@ -32,6 +32,12 @@ export class Customer extends Entity {
})
name: string;

@property({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment to explain that this property is needed for CouchDB/Cloudant tests.

delete(entity: T, options?: Options): Promise<void> {
async delete(entity: T, options?: Options): Promise<void> {
// perform persist hook
await this.entityToData(entity, options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if delete is supposed to reject requests when entity contains navigational properties, but I don't have a strong opinion on what's better.

* @param entity The entity passed from CRUD operations' caller.
* @param options
*/
protected async entityToData<R extends T>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since entityToData is a new public API, we should have tests to describe and verify the intended behavior:

  • entityToData is called for create/patch/replace operations (probably for delete too)
  • the value returned by entityToData is sent to persisted model, i.e. modifications made by this function are applied

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test for entityToData is added. It is invoked by create() method cause it's protected.

* @param entity The entity passed from CRUD operations' caller.
* @param options
*/
protected ensurePersistable<R extends T>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to expose this method as protected? We already have entityToData, I think that's good enough, and thus ensurePersistable can be private.

Thoughts?

@jannyHou
Copy link
Contributor

jannyHou commented Dec 2, 2019

@bajtos Thanks for the review and very detailed suggestion!

We have recently added a flag to disable that behavior, see loopbackio/loopback-datasource-juggler#1783. What happens when we enable that flag in repository-tests models - will it solve the issues we are observing here?

I tried useDefaultIdType but not working, maybe I didn't update the juggler to include that fix, let me try again, and that's why I used the solution in the last commit as a workaround(while it turns out to be a wrong solution).

I think that means we need to add _rev property to the Customer model in the fixture.

👍 I updated in the 2nd last commit, and all tests pass EXCEPT the one failure in the mongodb test suite. The changes in the last commit are actually quite broken.

@jannyHou jannyHou force-pushed the reject-navigational branch 2 times, most recently from 2729720 to 4311a04 Compare December 2, 2019 17:32
@agnes512 agnes512 force-pushed the reject-navigational branch 3 times, most recently from 425512d to a476fbe Compare December 3, 2019 13:57
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the tests are passing for all connectors, I guess it's time to move on and create follow-up issues to clean up the shared tests.

We already have an issue for improving ObjectID handling: #3456 Can you please post a comment there with a link to this PR and explanation what issues we need to fix?

I think there are two more issues to open: one to enable skipped tests on Cloudant, another to make the shared models smaller, so that shippment_id property is present only in tests verifying the use case where the DB foreign key is different from the LB4 property name.

PTAL at the comments I posted during my last review, I think few of them are not addressed yet.

Other than that, I am ok to land this pull request mostly as it is now.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 :shipit: LGTM

@agnes512 agnes512 merged commit 01de327 into master Dec 4, 2019
@agnes512 agnes512 deleted the reject-navigational branch December 4, 2019 03:38
@agnes512 agnes512 changed the title feat(repository): rejects create/update operations for navigational properties feat(repository): rejects CRUD operations for navigational properties Dec 4, 2019
@CedricBad
Copy link

CedricBad commented Dec 19, 2019

Hi there, I am trying to perform a crud operation (post and patch) without any navigational property, but the error shows off, I can't understand why...

To make it short, I have a simple model :

class Bag extends Entity {
@property() id;
@property() name;
@hasMany(() => Stuff)
  stuffs?: Array<Stuff> = [];
}

In my repository I do something like:

this.updateById(1, {name: 'new bag');

And I got this error : Error: Navigational properties are not allowed in model data (model "Bag" property "stuffs");

Am I missing something ?

--- EDIT ---
All right it was because of the assigment of stuffs in my class, I just had to remove "= []".

@agnes512
Copy link
Contributor Author

@CedricBad with the hasMany decorator,

@hasMany(() => Stuff)
  stuffs?: Array<Stuff>

if a Bag instance doesn't have any Stuff, stuffs would be empty and won't be included in the result if you travers bagRepository.find({include:[{relation: 'stuffs'}]}).

Please let me know if you have any questions.

@CedricBad
Copy link

@agnes512

I was looking for a way to have my stuffs always included, even if there is none. That is why I assigned an empty array to the stuffs. But then I have the error mentionned aboved, so I guess there is no way to have a default value assigned to a relationnal item.
Sorry if I'm not clear, I'm giving you my best english 💃

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

Successfully merging this pull request may close these issues.

None yet

4 participants