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

Fix join in delete #7154

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix join in delete #7154

wants to merge 3 commits into from

Conversation

mtsoltan
Copy link

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

In MySQL, delete supports inner joins (but not left joins).
This PR does two things to make it possible for us to use InnerJoin to build a clause into our delete SQL statements.

  1. Abstract the join logic from query.go into join.go to allow it to be re-used in delete (now) and possibly in update (in future work).
  2. Use the join logic in delete, and add table name after the DELETE keyword.

The problems with this PR:

  1. It does not check if the join is an InnerJoins or a Joins - so this will build DELETE t FROM t LEFT JOIN r WHERE ...; statements if it is passed a normal left join - when this is invalid SQL. The logic should be upgraded to silently discard the join, or emit an error that says "You cannot left join in a delete statement. Maybe you meant to inner join instead?".
  2. Tries to make the least amount of changes possible. This is why the introduced HandleJoins function takes two callback arguments - to make it very faithful to what initially happened in query.go and ensure no regressions.
  3. It does not introduce a test. I tried it out and it works, but it should introduce a test.

I'm reluctant to solve those three problems until a contributor goes through the logic and verifies that the work done is meaningful. If that's the case, we can have a discussion about the organization of the code, and error handling / silent discarding of clause for left joins, and once the logic is finalized, I'll introduce tests for it.

User Case Description

// Delete a ModelB based on the value of a column in TableA
db.InnerJoins("TableA").Where("TableA.columnA in ?", values).Delete(&ModelB { ID: someID });

@mtsoltan mtsoltan changed the title Fix join in delete (part 1) Fix join in delete Aug 12, 2024
Copy link
Member

@a631807682 a631807682 left a comment

Choose a reason for hiding this comment

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

Delete/Update Join does not seem to be part of the sql standard, but an implementation of mysql. As far as I know, sqlite/postgres do not support it (they associate other tables through From).
I don't recommend changing it for specific driver, I think we can add clause to support it, and if the new clause requires internal functions similar to HandleJoins, then exporting these functions is also possible.
What do you think?

@mtsoltan
Copy link
Author

Thank you for the very swift reply! 🙇

If it's only supported on one dialect, I agree with you, it makes the most sense to add a clause to support it. Can you point me in the right direction of what I need to take care of to make sure that my work in adding a clause is of high enough quality to be merged?

I'm also interested in what we'd call the clause. Since it's also a delete clause (just one that has an inner join). So maybe DELETE_JOIN? - added by the function Delete() on a statement that already has an inner join?

I can look into this myself, but if you have the extra time to spare, I'd appreciate being pointed to where I can make this dialect-specific to just MySQL, if such a place does exist.

@a631807682
Copy link
Member

a631807682 commented Aug 13, 2024

So maybe DELETE_JOIN? - added by the function Delete() on a statement that already has an inner join?

I think it's OK, we can change the existing behavior through StatementModifier just like SoftDeleteQueryClause. I am not sure if it is possible or if it requires support from internal functions.

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.

2 participants