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

perf(core): optimization for assignToChannels method #2743

Conversation

monrostar
Copy link
Contributor

@monrostar monrostar commented Mar 16, 2024

Description

This is an optimization of the channel-aware update request for Entity. The method offers 3 separate queries without additional data processing by TypeORM.

  1. Get the object without unnecessary links and check that it exists.
  2. Make a separate, more optimized query into the ManyToMany table to reduce the amount of data to process
  3. Change the save method to Remove or Add (unbinds, binds) given value from entity relation.
relation('channels')
.of(entity.id)
.add(newChannelIds) or .remove(existingChannelIds);

Breaking changes

No

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

@monrostar monrostar changed the title feat(core): optimization for assignToChannels method perf(core): optimization for assignToChannels method Mar 16, 2024

const junctionTableName = channelsRelation.junctionEntityMetadata?.tableName;
const junctionColumnName = channelsRelation.junctionEntityMetadata?.columns[0].databaseName;
const inverseJunctionColumnName = channelsRelation.junctionEntityMetadata?.inverseColumns[0].databaseName;
Copy link
Member

Choose a reason for hiding this comment

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

With the latest updates we explicitly added inverse relations to the Channel entity.

I'm wondering what would happen if someone defines a custom channel-aware entity which does not have the inverse relation on the Channel? Would this break?

Copy link
Contributor Author

@monrostar monrostar Mar 18, 2024

Choose a reason for hiding this comment

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

This is quite normal, there should be no errors because even without reverse side entity is still available
But if there are problems we replace them with pure SQL

With the latest updates we explicitly added inverse relations to the Channel entity.

I'm wondering what would happen if someone defines a custom channel-aware entity which does not have the inverse relation on the Channel? Would this break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or just manually parse the column names in the table. This can also be done. We just need to test it on a real project

@michaelbromley michaelbromley merged commit c69e4ac into vendure-ecommerce:minor Mar 18, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants