Skip to content

Conversation

@agnes512
Copy link
Contributor

fixes #5852

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 👈

@agnes512 agnes512 added the Relations Model relations (has many, etc.) label Jun 30, 2020
@agnes512 agnes512 self-assigned this Jun 30, 2020
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.

Looks mostly good 👏 I have few comments to consider, see below.

It would be great to get a review from somebody more familiar with this area of our codebase.


if (where) {
// only delete related through models
const targets = await targetRepository.find({where: where});
Copy link
Member

Choose a reason for hiding this comment

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

Since we already have a list of troughInstances, is there a way how to limit this query to find only target models that are matching the user-provided where and are related to the source model(s)?

I am concerned about performance implication of the current solution, because it may fetch too many records. Consider the case Doctor-Appointment-Patient case, where we want to delete all appointment+patient records of the given doctor where the patient has {died: true}. Let's say our database is large (e.g. ~100k doctors). For each doctor, there will be only few patients that have passed away (e.g. 1-10). Ideally, we should fetch only those 1-10 patients, not ~100k-1000k passed-away patients of all doctors.

I am fine to leave this performance optimization out of scope of the initial implementation, as long as we have some documentation for this known performance issue (a code comment is good enough).

A nitpick on coding style:

Suggested change
const targets = await targetRepository.find({where: where});
const targets = await targetRepository.find({where});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed 👍. I couldn't find a way to fix it without accessing to the database, but we can improve it by limiting how many records it fetches.

I think it needs more helpers to build such a where filter. I left a TODO comment.

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.

👍 LGTM and agreed with Miroslav's comment about the performance.

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.

LGTM 👍

Please remember to clean up the commit history before landing :)

@agnes512 agnes512 merged commit c1ba91f into master Jul 16, 2020
@agnes512 agnes512 deleted the fix/delete branch July 16, 2020 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Relations Model relations (has many, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

should be able to delete target instances based on filters in HasManyThrough relation

5 participants