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

[SQLite Dialect] Properly translate .ignore() #916

Closed
BitPhinix opened this issue Mar 18, 2024 · 4 comments · Fixed by #976 · May be fixed by #1278
Closed

[SQLite Dialect] Properly translate .ignore() #916

BitPhinix opened this issue Mar 18, 2024 · 4 comments · Fixed by #976 · May be fixed by #1278
Labels
api Related to library's API breaking change Includes breaking changes enhancement New feature or request good first issue Good for newcomers greenlit Ready for implementation mysql Related to MySQL sqlite Related to sqlite

Comments

@BitPhinix
Copy link

Currently .ignore() using the SQLite dialect gets translated into INSERT IGNORE INTO ....

Screenshot 2024-03-18 at 14 56 02

Which doesn't match the SQLite expected INSERT OR IGNORE INTO (see here)

Screenshot 2024-03-18 at 14 57 55
@BitPhinix BitPhinix changed the title [SQLLite Dialect] Properly translate .ignore() [SQLite Dialect] Properly translate .ignore() Mar 18, 2024
@koskimas koskimas added greenlit Ready for implementation good first issue Good for newcomers sqlite Related to sqlite labels Mar 19, 2024
@igalklebanov
Copy link
Member

igalklebanov commented Mar 19, 2024

Hey 👋

Nice catch!

@koskimas I wonder what the solution should be.

The low hangin' fruit is just adding an override in SqliteQueryCompiler that appends an or. The purist in me screams "But it's not WhatYouSeeIsWhatYouGet!!!".

WYSIWYG alternatives are:

db.insertInto('person').or('abort')
db.insertInto('person').or('fail')
db.insertInto('person').or('ignore')
db.insertInto('person').or('replace')
db.insertInto('person').or('rollback')
db.insertInto('person').orAbort()
db.insertInto('person').orFail()
db.insertInto('person').orIgnore()
db.insertInto('person').orReplace()
db.insertInto('person').orRollback()

But having or('ignore') or orIgnore() AND ignore() might be confusing.

@igalklebanov igalklebanov added the bug Something isn't working label Mar 19, 2024
@koskimas
Copy link
Member

koskimas commented Mar 19, 2024

It's not wysiwyg but it's so close that I could live with it.

But supporting all those other or thingies is a good idea, and in that case the separate method(s) is better. I'd go with separate methods.

We might still override ignore to work as well to avoid mistakes?

@igalklebanov
Copy link
Member

We might still override ignore to work as well to avoid mistakes?

I definitely think that's the nice thing to do.

@igalklebanov igalklebanov added enhancement New feature or request api Related to library's API labels Mar 19, 2024
@igalklebanov igalklebanov removed the bug Something isn't working label May 1, 2024
@joaobzrr
Copy link

Any update on this?

@igalklebanov igalklebanov added mysql Related to MySQL breaking change Includes breaking changes labels Jan 6, 2025
@igalklebanov igalklebanov linked a pull request Jan 6, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API breaking change Includes breaking changes enhancement New feature or request good first issue Good for newcomers greenlit Ready for implementation mysql Related to MySQL sqlite Related to sqlite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants