feature: Add support for LIMIT query in UPDATE for sql/sqlite #452
Conversation
Signed-off-by: Rohan Hasabe <[email protected]>
|
Fixed the failing tests. |
* For multiple-table syntax, ORDER BY and LIMIT cannot be used.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #452 +/- ##
==========================================
+ Coverage 91.26% 91.28% +0.01%
==========================================
Files 134 134
Lines 7765 7782 +17
==========================================
+ Hits 7087 7104 +17
Misses 509 509
Partials 169 169 ☔ View full report in Codecov by Sentry. |
|
@go-jet Ping on this^, can I get some review comments so I can refactor the change if needed. |
mysql/update_statement.go
Outdated
| @@ -12,15 +12,19 @@ type UpdateStatement interface { | |||
| MODEL(data interface{}) UpdateStatement | |||
|
|
|||
| WHERE(expression BoolExpression) UpdateStatement | |||
| LIMIT(limit int64) UpdateStatement | |||
| INNER_JOIN(table ReadableTable, onCondition BoolExpression) UpdateStatement | |||
There was a problem hiding this comment.
Multi table update is already supported, check this test:
jet/tests/mysql/update_dvds_test.go
Lines 11 to 15 in 00b8155
sqlite/update_statement.go
Outdated
| @@ -76,3 +84,11 @@ func (u *updateStatementImpl) RETURNING(projections ...Projection) UpdateStateme | |||
| u.Returning.ProjectionList = projections | |||
| return u | |||
| } | |||
|
|
|||
| func (u *updateStatementImpl) LIMIT(limit int64) UpdateStatement { | |||
| if u.hasFrom { | |||
sqlite/update_statement.go
Outdated
| @@ -64,6 +69,9 @@ func (u *updateStatementImpl) MODEL(data interface{}) UpdateStatement { | |||
|
|
|||
| func (u *updateStatementImpl) FROM(tables ...ReadableTable) UpdateStatement { | |||
| u.From.Tables = readableTablesToSerializerList(tables) | |||
| u.hasFrom = true | |||
| // Remove any existing LIMIT since it's not allowed with FROM | |||
| u.Limit.Count = -1 | |||
There was a problem hiding this comment.
This would introduce hidden behavior. It's better to fail at runtime rather than silently remove clauses.
There was a problem hiding this comment.
Added a straight panic for this.
Signed-off-by: Rohan Hasabe <[email protected]>
| } | ||
|
|
||
| func TestUpdateWithMultiTableAndLimit(t *testing.T) { | ||
| defer func() { |
There was a problem hiding this comment.
You can use testutils.AssertPanicErr to catch and compare panic errors.
| @@ -23,6 +24,8 @@ type updateStatementImpl struct { | |||
| SetNew jet.SetClauseNew | |||
| Where jet.ClauseWhere | |||
| Returning jet.ClauseReturning | |||
| Limit jet.ClauseLimit | |||
| hasFrom bool | |||
|
|
||
| func (u *updateStatementImpl) LIMIT(limit int64) UpdateStatement { | ||
| if len(u.From.Tables) > 1 { | ||
| panic("jet: SQLite does not support LIMIT with multi-table UPDATE statements") |
There was a problem hiding this comment.
How can we verify this? I haven't find any golang sqlite driver that compiles sqlite with SQLITE_ENABLE_UPDATE_DELETE_LIMIT.
There was a problem hiding this comment.
Oh, I wasn't aware of this. I shall read up on it. Do you have anything in your mind for this?
| } | ||
|
|
||
| func TestUpdateWithMultiTableAndLimit(t *testing.T) { | ||
| defer func() { |
| WHERE(Link.Name.NOT_EQ(String(""))). | ||
| LIMIT(2) | ||
|
|
||
| testutils.ExecuteInTxAndRollback(t, db, func(tx qrm.DB) { |
There was a problem hiding this comment.
Execute can be removed, since panic will occur before.
Closes #448