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

Question: persist nested entities #678

Closed
davenathanael opened this issue Jul 21, 2020 · 18 comments
Closed

Question: persist nested entities #678

davenathanael opened this issue Jul 21, 2020 · 18 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@davenathanael
Copy link

Hello, I'm trying to persist an entity with a nested entity inside it, but the nested one does not persisted on DB and the return value of the OneToMany is not an array either (when logged to console or returned as JSON). Here's the structure:

// recipe.entity.ts
@Entity()
export class Recipe extends BaseEntity {
  @Property({ unique: true, length: 100 })
  name: string;

  @OneToMany({
    entity: () => IngredientRecipeUsage,
    mappedBy: 'recipe',
    orphanRemoval: true,
    cascade: [Cascade.ALL],
  })
  ingredients = new Collection<IngredientRecipeUsage>(this);

  constructor(data: Partial<Recipe>) {
    super();
    Object.assign(this, data);
  }
}

// ingredient-recipe-usage.entity.ts
@Entity()
export class IngredientRecipeUsage extends BaseEntity {
  @ManyToOne()
  ingredient: Ingredient; // another entity, omited for brevity

  @ManyToOne()
  recipe: Recipe;

  @Property({ columnType: 'int' })
  amount: number;
}

MikroORM is in use together with NestJS, here's a snippet of the controller and service:

// recipe.controller.ts
@Post()
  async createOne(@Body() data: Recipe): Promise<Recipe> {
    const { em } = this.orm;
    const mappedData: Recipe = em
      .getDriver()
      .mapResult(data, em.getMetadata().get(Recipe.name));
    console.log(mappedData);
    return this.service.createOne(mappedData);
  }

// recipe.service.ts
async createOne(data: EntityData<Recipe>): Promise<Recipe> {
    const recipe = this.repo.create(data); // this.repo: EntityRepository<Recipe>
    await this.repo.persistAndFlush(recipe);
    return recipe;
  }

Given the request payload and its response:

// request
{
    "name": "Recipe 1",
    "ingredients": [
        {
            "ingredient": "cff72889-e5f4-4d91-a6ec-43b16564fdfb", // an Ingredient record with such id exists on DB
            "amount": "1"
        }
    ]
}

// response
{
    "id": "21a1505b-9b47-4142-994e-d8ff48ca874b",
    "ingredients": {
        "initialized": true,
        "dirty": false
    },
    "name": "Recipe 1",
    "created_at": "2020-07-21T10:10:06.892Z"
}

I'm wondering what did I do wrong and how to resolve:

  1. The ingredients JSON returned an object while I wanted an array (since its a OneToMany, should be IngredientRecipeUsage[], seems it's the Collection<IngredientRecipeUsage> instead?
  2. The ingredients field, while provided on the request body, does not persisted on the DB.

What I have read is that entities are by default cascading when persisted, eventhough I added the Cascade.ALL just to make sure. What did I miss/do wrong?

@B4nan
Copy link
Member

B4nan commented Jul 22, 2020

The ingredients JSON returned an object while I wanted an array (since its a OneToMany, should be IngredientRecipeUsage[], seems it's the Collection instead?

Nope, it should not be an array, collections are represented via Collection wrapper. You can call toArray() to serialize it. This happens automatically if you serialize the whole entity via JSON.stringify() (not just the collection). But due to your second problem, this is not working as you are overriding the Collection instance with god knows what (via the Object.assign()).

The ingredients field, while provided on the request body, does not persisted on the DB.

Your problem is probably in the constructor, you cannot replace the collection instance (which this code does).

constructor(data: Partial<Recipe>) {
  super();
  Object.assign(this, data); // this is wrong
}

You can use wrap(this).assign(data) - that way collection updates are supported too.

Anyway, first enable debug mode and add a console.log(recipe) after you create the entity, so you can understand what you get there from em.create() call. But in general, forget about Object.assign(this, data) when it comes to entities (and use wrap(e).assign() instead).

@B4nan B4nan added the question Further information is requested label Jul 22, 2020
@davenathanael
Copy link
Author

I see. I tried to change it to wrap(this).assign(data) on the Recipe constructor like so:

constructor(data: EntityData<Recipe>) {
    super();
    wrap(this).assign(data);
  }

and got this error:

[Nest] 21525   - 07/22/2020, 11:01:59 PM   [ExceptionsHandler] Cannot convert undefined or null to object +1286ms
TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Function.assign (/home/dep/Development/proj/foodcrate/kitchen/node_modules/mikro-orm/dist/entity/EntityAssigner.js:17:16)
    at Recipe.prototype.assign (/home/dep/Development/proj/foodcrate/kitchen/node_modules/mikro-orm/dist/entity/EntityHelper.js:35:56)
    at new Recipe (/home/dep/Development/proj/foodcrate/kitchen/dist/src/common/entities/recipe.entity.js:22:32)
    at TransformOperationExecutor.transform (/home/dep/Development/proj/foodcrate/kitchen/node_modules/class-transformer/TransformOperationExecutor.js:117:32)
    at ClassTransformer.plainToClass (/home/dep/Development/proj/foodcrate/kitchen/node_modules/class-transformer/ClassTransformer.js:17:25)
    at Object.plainToClass (/home/dep/Development/proj/foodcrate/kitchen/node_modules/class-transformer/index.js:20:29)
    at ValidationPipe.transform (/home/dep/Development/proj/foodcrate/kitchen/node_modules/@nestjs/common/pipes/validation.pipe.js:41:39)
    at transforms.reduce (/home/dep/Development/proj/foodcrate/kitchen/node_modules/@nestjs/core/pipes/pipes-consumer.js:16:33)
    at processTicksAndRejections (internal/process/task_queues.js:86:5)

I've turned on debug mode and no sql has been executed yet after the change above.

@B4nan
Copy link
Member

B4nan commented Jul 22, 2020

And what is in the data?

@davenathanael
Copy link
Author

Also, after re-reading some pages, all the examples have the objects initialized manually, as in this page: https://mikro-orm.io/docs/entity-manager#persisting-and-cascading

const author = new Author('Jon Snow', 'snow@wall.st');
author.born = new Date();

const publisher = new Publisher('7K publisher');

const book1 = new Book('My Life on The Wall, part 1', author);
book1.publisher = publisher;

Is it needed that all the relations are initialized beforehand (via constructors, or findOne from a repository for existing ones)? Can the ORM handle nested creation like so, or is there a preferable way to do so:

const book1 = new Book('My Life on The Wall, part 1', { name: 'Jon', email: 'snow@wall.st' }); // a new Book record as well as a new Author record created

since that's what I'm aiming to do with my code at the OP. Sorry if this kind of derails the issue, but I think its similar.
Basically I want to be able to create new record for both Recipe and IngredientRecipeUsage hopefully in one method without initializing everything manually and linking them together by hand, if possible.

@B4nan
Copy link
Member

B4nan commented Jul 22, 2020

Maybe create a reproduction. It should work with the assign method as well as with em.create(). PK will be considered as existing entity, and an object as new one.

@davenathanael
Copy link
Author

Okay, I'll try to setup a reproduction repo

@davenathanael
Copy link
Author

Okay, I made a repository here: https://github.com/davenathanael/mikro-orm-678

While I made it, I fixed the TypeError: Cannot convert undefined or null to object error is because Nest is trying to create a Recipe object for parsing the request body on the controller, I suppose. Not related to MikroORM, I'm sorry.

Regarding to the Collection being printed as an object, I tried the repo without using class-transformer's @Expose to change the serialized JSON keys, and the Collection printed correctly, as an array. When I installed class-transformer and add the @Expose decorator to an entity, then the Collection printed as an object. I'm not sure if it's a bug/incompability or a mistake on my setup.

The nested entity is not being persisted on DB is explained on the repository readme. Thanks! :)

@B4nan
Copy link
Member

B4nan commented Jul 22, 2020

There is no need to use class-transformer here, MikroORM handles the serialization internally (each entity has toJSON() method, you can also override it to customize the bahaviour). Cant help with that, never used it myself (never even needed it).

@davenathanael
Copy link
Author

Ah I see. I'm using class-transformer for serializing multi-word fields as snake_case, I guess I'll handle them separately. How about the ingredients collection not persisted to DB? Is it because of the usage of class-transformer as well?

@B4nan
Copy link
Member

B4nan commented Jul 23, 2020

You are passing ingredient_usage into the em.create(), which is the first problem, as the ORM does not know anything about such key (you feed only class-transformer with that metadata). So it basically gets ignored, as an unknown key.

Also having those object ctor params, ORM will never pass them to your constructor - it works only when you have one param per property (and the names must match, e.g. Book has ctor with title and author, then those will be provided, but you will never get the "data object" there). If you want to have it like this, you need to call the constructor yourself.

Unfortunately em.create() does not handle the collection updates properly, so you really need that wrap().assign() call - but that requires to have EM instance provided explicitly, so you cannot use it from the constructor (as at that time the entity never has EM assigned).

-const recipe = em.create(Recipe, data);
+data.ingredients = (data as any).ingredient_usage;
+const recipe = wrap(new Recipe()).assign(data, { em: this.em });

Then it will properly construct the entity and assign the collection items, firing 2 queries in a transaction:

[query] begin
[query] insert into `recipe` (`name`) values ('Recipe 1') [took 1 ms]
[query] insert into `ingredient_recipe_usage` (`amount`, `ingredient_id`) values (1, 1) [took 1 ms]
[query] commit

Will need to verify if this still happens in v4, as it should work with the em.create() too.

@davenathanael
Copy link
Author

You are passing ingredient_usage into the em.create(), which is the first problem, as the ORM does not know anything about such key (you feed only class-transformer with that metadata). So it basically gets ignored, as an unknown key.

My bad, sorry. It is a bad example because my use case is to have the request payload the same as how it is stored at DB following the ORM's naming strategy (Underscore), I didn't think of that when I made that example. Now it is sorted out because I use em.getDriver().mapResult() to convert the fields.

Okay, currently I'm able to follow along and produce the same result as yours there, with wrap instead of create, and get the same queries. But the second insert query didn't specify any recipe_id value, that should be the id of the first insert query. I'm not quite confident with my SQL knowledge, but is it not possible to specify the recipe_id of the inserted ingredient_recipe_usage record within the same transaction?

@B4nan
Copy link
Member

B4nan commented Jul 23, 2020

Mmmm I see, yes that is supported, the problem is how the assign helper handles the collection update - if you did it manually, things would be propagated correctly and the FK would be set:

const recipe = new Recipe();
recipe.ingredients.add(...); // this way it would be propagated to the owning side

Will try to do something about this in v4, its really confusing, and as I said, the way with em.create() should work here too.

@B4nan B4nan added bug Something isn't working and removed question Further information is requested labels Jul 23, 2020
@B4nan B4nan added this to the 4.0 milestone Jul 23, 2020
@B4nan B4nan self-assigned this Jul 23, 2020
@davenathanael
Copy link
Author

Ah I see, thanks for explaining :), its ok I can do manually for now. Tried to do it and decided to use a DTO for the controller, and changed the service method like so:

async createOne(dto: CreateRecipeDto): Promise<Recipe> {
    const recipe = new Recipe();
    recipe.name = dto.name;
    recipe.sellingPrice = dto.sellingPrice;
    
    const ingredients = await Promise.all(dto.ingredients.map(async (u) => {
      const usage = new IngredientRecipeUsage();
      usage.amount = u.amount;
      usage.ingredient = await this.ingredientRepo.findOneOrFail({ id: u.ingredient });
      return usage;
    }));
    console.log(ingredients);
    recipe.ingredients.add(...ingredients);

    console.log('created recipe:');
    console.log(recipe);
    await this.repo.persistAndFlush(recipe);
    return recipe;
  }

and this is the log I printed, with the [constructor] dto: part is print of the DTO received at the constructor, thus the data given:

[constructor] dto:
{
  name: 'Recipe 5',
  test: 2,
  ingredients: [ { ingredient: 1, amount: 11 } ],
  selling_price: 200
}
[Nest] 7432   - 07/23/2020, 8:39:40 PM   [MikroORM] [query] select `e0`.* from `ingredient` as `e0` where `e0`.`id` = 1 limit 1 [took 2 ms]
[
  IngredientRecipeUsage { amount: 11, ingredient: Ingredient { id: 1, name: 'Ingredient 1' } }
]
created recipe:
Recipe {
  ingredients: Collection {
    '0': IngredientRecipeUsage { amount: 11, ingredient: [Ingredient] },
    initialized: true,
    dirty: false
  },
  name: 'Recipe 5',
  sellingPrice: undefined
}
[Nest] 7432   - 07/23/2020, 8:39:40 PM   [MikroORM] [query] begin
[Nest] 7432   - 07/23/2020, 8:39:40 PM   [MikroORM] [query] insert into `recipe` (`name`, `selling_price`) values ('Recipe 5', NULL) [took 1 ms]
[Nest] 7432   - 07/23/2020, 8:39:40 PM   [MikroORM] [query] insert into `ingredient_recipe_usage` (`amount`, `ingredient_id`) values (11, 1) [took 1 ms]
[Nest] 7432   - 07/23/2020, 8:39:40 PM   [MikroORM] [query] commit

Ignore the sellingPrice and selling_price

I'm still getting no recipe_id on the query executed, is the way I do the collection update correct?

@B4nan
Copy link
Member

B4nan commented Jul 23, 2020

Can you upgrade your repository? For me it does work:

[query] begin
[query] insert into `recipe` (`name`) values ('Recipe 1') [took 3 ms]
[query] insert into `ingredient_recipe_usage` (`amount`, `ingredient_id`, `recipe_id`) values (1, 1, 6) [took 1 ms]
[query] commit

Btw if you do not want to load the Ingredient entity, you can use repo.getReference() instead, which is a sync method to construct a reference to an entity (which is basically an entity that has only the PK set).

(I am also getting some class-transformer recursion error, but that is outside of the scope of what I can help with - in general you should serialize the entity via wrap(e).toJSON() first, which will handle the conversion from a Collection wrapper to a plain array. - commenting out the ClassSerializerInterceptor fixes that)

@davenathanael
Copy link
Author

Btw if you do not want to load the Ingredient entity, you can use repo.getReference() instead, which is a sync method to construct a reference to an entity (which is basically an entity that has only the PK set).

Ah, that's new to me. It'll be cleaner since I do not want to actually load the entity and having the repository there. Thanks!

(I am also getting some class-transformer recursion error, but that is outside of the scope of what I can help with - in general you should serialize the entity via wrap(e).toJSON() first, which will handle the conversion from a Collection wrapper to a plain array. - commenting out the ClassSerializerInterceptor fixes that)

Yeah sorry didn't mention that on my last message, I do comment out the ClassSerializerInterceptor at the app start. I no longer use class-transformer and use the toJSON instead.

Can you upgrade your repository? For me it does work:

Anyway, I've update the issue repository https://github.com/davenathanael/mikro-orm-678, when you have the time please kindly check it :)

Thank you so much in advance:)

B4nan added a commit that referenced this issue Jul 24, 2020
Previously `em.create()` did not handle collection updates correctly, basically ignoring all the
values if the property was not M:N owning side.

With this fix, both `em.create()` and `wrap(e).assign()` properly handled collection updates,
including propagation to the owning sides.

Closes #678
B4nan added a commit that referenced this issue Jul 24, 2020
Previously `em.create()` did not handle collection updates correctly, basically ignoring all the
values if the property was not M:N owning side.

With this fix, both `em.create()` and `wrap(e).assign()` properly handled collection updates,
including propagation to the owning sides.

Closes #678
@B4nan
Copy link
Member

B4nan commented Jul 24, 2020

Both assign() and em.create() are fixed in v4 via #683.

The problem with your example repository is that you do not define the 1:m correctly - you are missing mappedBy there, that would point to the owning side. Without that the ORM does not know where to propagate to.

  @OneToMany({
    entity: () => IngredientRecipeUsage,
    orphanRemoval: true,
    eager: true,
    mappedBy: usage => usage.recipe,
  })
  ingredients = new Collection<IngredientRecipeUsage>(this);

I also made the mappedBy required in v4, iirc the reason why it's optional is that you can specify the opposite only (owner -> inverse), but in general we should always specify this on the inverse side (which 1:m relation is).

@B4nan B4nan closed this as completed Jul 24, 2020
@davenathanael
Copy link
Author

My bad! Sorry didn't realised I missed the mappedBy haha. Thank you for all the help! @B4nan

Whoa that's great, I'm looking forward to v4! Cheers :D

@B4nan
Copy link
Member

B4nan commented Jul 25, 2020

Well not really your bad, there should have been some validation, pretty much all the other entity definition mistakes are validated, its weird that I forgot to validate this one :]

B4nan added a commit that referenced this issue Aug 2, 2020
Previously `em.create()` did not handle collection updates correctly, basically ignoring all the
values if the property was not M:N owning side.

With this fix, both `em.create()` and `wrap(e).assign()` properly handled collection updates,
including propagation to the owning sides.

Closes #678
B4nan added a commit that referenced this issue Aug 9, 2020
Previously `em.create()` did not handle collection updates correctly, basically ignoring all the
values if the property was not M:N owning side.

With this fix, both `em.create()` and `wrap(e).assign()` properly handled collection updates,
including propagation to the owning sides.

Closes #678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants