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(core): allow specifying custom pivot table entity #2901

Merged
merged 1 commit into from
Mar 13, 2022
Merged

Conversation

B4nan
Copy link
Member

@B4nan B4nan commented Mar 13, 2022

  @ManyToMany({ entity: () => Product, pivotEntity: () => OrderItem })
  products = new Collection<Product>(this);

@B4nan B4nan changed the title Allow specifying custom pivot table entity feat: allow specifying custom pivot table entity Mar 13, 2022
@B4nan B4nan changed the title feat: allow specifying custom pivot table entity feat(core): allow specifying custom pivot table entity Mar 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2022

Codecov Report

Merging #2901 (8d25d48) into master (f359929) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2901   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          193       193           
  Lines        11764     11770    +6     
  Branches      2715      2716    +1     
=========================================
+ Hits         11764     11770    +6     
Impacted Files Coverage Δ
packages/core/src/decorators/ManyToMany.ts 100.00% <ø> (ø)
packages/core/src/drivers/DatabaseDriver.ts 100.00% <100.00%> (ø)
packages/core/src/metadata/MetadataDiscovery.ts 100.00% <100.00%> (ø)
packages/core/src/metadata/MetadataStorage.ts 100.00% <100.00%> (ø)
packages/core/src/typings.ts 100.00% <100.00%> (ø)
...ges/core/src/unit-of-work/CommitOrderCalculator.ts 100.00% <100.00%> (ø)
packages/knex/src/AbstractSqlDriver.ts 100.00% <100.00%> (ø)
packages/knex/src/query/QueryBuilder.ts 100.00% <100.00%> (ø)
packages/knex/src/query/QueryBuilderHelper.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f359929...8d25d48. Read the comment docs.

@B4nan B4nan marked this pull request as ready for review March 13, 2022 16:44
```ts
@manytomany({ entity: () => Product, pivotEntity: () => OrderItem })
products = new Collection<Product>(this);
```
@B4nan B4nan merged commit 8237d16 into master Mar 13, 2022
@B4nan B4nan deleted the pivot-entity branch March 13, 2022 18:20
@yackinn
Copy link

yackinn commented Mar 14, 2022

Is this supposed to add another column specified in the OrderItem entity to the pivot table?
Can this be used for sorting purposes?

@B4nan
Copy link
Member Author

B4nan commented Mar 14, 2022

Full example is in the docs: https://mikro-orm.io/docs/collections#custom-pivot-table-entity

The OrderItem entity represents the pivot table, so yes, it allows you to create additional columns in that table. You can't sort by them from the M:N relation currently, we could allow defining default order by based on that, if you'd like to do it from the FindOptions API, you would need to use the old 1:m - m:1 approach instead as in https://mikro-orm.io/docs/composite-keys#use-case-3-join-table-with-metadata

Or if you can think of some public API that would allow sorting thru the pivot properties, I am not against it, I just don't see how it could be configured.

@yackinn
Copy link

yackinn commented Mar 14, 2022

If I add the LocationMedium entity to the entiies array it will error.
Error: Please provide either 'type' or 'entity' attribute in LocationMedium.location

If I don't add it to the entities array it doesn't change anything in the pivot table though.

I think that's how it's stated in the docs.

@Entity()
export class LocationMedium {

  @ManyToOne({ primary: true })
  location: Location;

  @ManyToOne({ primary: true })
  medium: Medium;

  @Property({ default: 1 })
  prop: number;

}


// location.entity.ts
@Entity()
export class Location extends OmputBaseEntity {
  @Property()
  title: string;
  ...

  @OneToMany(() => Event, courseDate => courseDate.location, { nullable: true })
  event?: Event[];

  @ManyToMany({ entity: () => Medium, nullable: true, pivotEntity: () => LocationMedium })
  media?: Medium[];
}

Bildschirmfoto 2022-03-14 um 12 47 25

@B4nan
Copy link
Member Author

B4nan commented Mar 14, 2022

The entity indeed needs to be discovered. You need to specify the type in decorator options (unless you are using ts-morph):

  @ManyToOne(() => Location, { primary: true })
  location: Location;

  @ManyToOne(() => Medium, { primary: true })
  medium: Medium;

Will fix the examples, I am planning to use the code tabs there as well, to show the definition with entity schema as well.

@B4nan
Copy link
Member Author

B4nan commented Mar 14, 2022

Note that this is wrong too, you need to use Collection wrapper:

  @OneToMany(() => Event, courseDate => courseDate.location, { nullable: true })
  event?: Event[]; // should be `new Collection<Event>(this)`

  @ManyToMany({ entity: () => Medium, nullable: true, pivotEntity: () => LocationMedium })
  media?: Medium[]; // should be `new Collection<Medium>(this)`

@yackinn
Copy link

yackinn commented Mar 14, 2022

Thank you.
I should have tried that.

The version without the Collection initilization also works though. The table is update with the new column.

By the way I noticed the search functionality is broken on the 5.1 docs.

Bildschirmfoto 2022-03-14 um 12 54 34

@B4nan
Copy link
Member Author

B4nan commented Mar 14, 2022

Its not broken, the index needs to be rebuild on algolia after each version is published. It happens once a week, will see if I can trigger it manually.

edit: fixed

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

3 participants