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

MariaDB: add support for RETURNING clause in DELETE statements #1059

Merged
merged 2 commits into from
Nov 16, 2024

Conversation

capcom6
Copy link
Contributor

@capcom6 capcom6 commented Nov 13, 2024

This PR adds support for RETURNING clauses in DELETE statements specifically for MariaDB databases. MariaDB has supported the RETURNING clause in DELETE statements since version 10.0.5.

Changes:

  • Introduce feature.DeleteReturning because the existing feature.Returning is used in UpdateQuery, but MySQL/MariaDB don't support RETURNING clause in UPDATE statements
  • Add feature.DeleteReturning to all dialects already having feature.Returning for backward compatibility
  • Add feature.DeleteReturning to mysql dialect for MariaDB 10.0.5 and later
  • Replace feature.Returning with feature.DeleteReturning in DeleteQuery
  • Add check for feature.DeleteReturning in DeleteQuery.Returning method because previously, Returning could be used for the mysql dialect but didn't receive any error or result

Example usage:

db.NewDelete().Model(new(Model)).WherePK().Returning("*")

Notes:

  • MariaDB doesn't support RETURNING clause for multi-table deletes but I haven't add any check for that because I don't know is any other database supports this.

Discussion:

  • Not directly related to this PR: There is feature.InsertReturning, but InsertQuery.tryLastInsertID checks feature.Returning. Is this correct?
  • After this PR, feature.Returning will be used only in UpdateQuery. I propose renaming it to feature.UpdateReturning to clarify its usage. This renaming has not been implemented yet.

References:

@capcom6 capcom6 force-pushed the mariadb/delete-returning branch from 3cb8e75 to b8dec9d Compare November 13, 2024 13:13
@capcom6 capcom6 changed the title MariaDB: add support for RETURNING clauses in DELETE statements MariaDB: add support for RETURNING clause in DELETE statements Nov 13, 2024
@capcom6 capcom6 marked this pull request as ready for review November 14, 2024 23:03
@j2gg0s j2gg0s self-requested a review November 15, 2024 02:37
dialect/oracledialect/go.mod Show resolved Hide resolved
q.err = errors.New("bun: returning is not supported for current dialect")
return q
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

For dialects that do not support DELETE RETURNING.
Before: We remained slient on misuse and ignore it when append query.
After: We return error on misuse.

I dont think current solution is correct, but i dont think we should break it,
as that might casue potential failures in upper-level application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, I was very discouraged when I tried to use Returning() with DeleteQuery and received an empty result. I propose adding a note to the method's documentation, something like: "If the dialect does not support DELETE RETURNING, an empty result will be returned without an error."

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont have a strong preference, let @vmihailenco decide.

Copy link
Member

Choose a reason for hiding this comment

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

I think returning an error might be annoying in some cases, but way easier to debug...

@j2gg0s j2gg0s requested a review from vmihailenco November 15, 2024 03:57
@vmihailenco vmihailenco merged commit 4fcf18a into uptrace:master Nov 16, 2024
3 checks passed
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.

3 participants