From 6952cd8ed8cbad007ec36a52d69375483168ee33 Mon Sep 17 00:00:00 2001 From: leaf corcoran Date: Thu, 2 Nov 2023 10:56:16 -0700 Subject: [PATCH] rewrite modle:update to not commit changes to the instance object until the update has completed successfully --- lapis/db/base_model.lua | 107 +++++++++++++++++++++++--------- lapis/db/base_model.moon | 76 ++++++++++++++++------- spec/model_spec.moon | 131 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 256 insertions(+), 58 deletions(-) diff --git a/lapis/db/base_model.lua b/lapis/db/base_model.lua index e3ba21f1..7b17f0bb 100644 --- a/lapis/db/base_model.lua +++ b/lapis/db/base_model.lua @@ -267,6 +267,7 @@ do end, update = function(self, first, ...) local cond = self:_primary_cond() + local update_fields = { } local columns if type(first) == "table" then do @@ -274,9 +275,10 @@ do local _len_0 = 1 for k, v in pairs(first) do if type(k) == "number" then + update_fields[v] = self[v] _accum_0[_len_0] = v else - self[k] = v + update_fields[k] = v _accum_0[_len_0] = k end _len_0 = _len_0 + 1 @@ -288,29 +290,25 @@ do first, ... } + for _index_0 = 1, #columns do + local c = columns[_index_0] + update_fields[c] = self[c] + end end if next(columns) == nil then return nil, "nothing to update" end if self.__class.constraints then - for _, column in pairs(columns) do + for _index_0 = 1, #columns do + local column = columns[_index_0] do - local err = self.__class:_check_constraint(column, self[column], self) + local err = self.__class:_check_constraint(column, update_fields[column], self) if err then return nil, err end end end end - local values - do - local _tbl_0 = { } - for _index_0 = 1, #columns do - local col = columns[_index_0] - _tbl_0[col] = self[col] - end - values = _tbl_0 - end local nargs = select("#", ...) local last = nargs > 0 and select(nargs, ...) local opts @@ -319,7 +317,7 @@ do end if self.__class.timestamp and not (opts and opts.timestamp == false) then local time = self.__class.db.format_date() - values.updated_at = values.updated_at or time + update_fields.updated_at = update_fields.updated_at or time end if opts and opts.where then assert(type(opts.where) == "table", "Model.update: where condition must be a table or db.clause") @@ -334,31 +332,80 @@ do where }) end - local returning - for k, v in pairs(values) do - if v == self.__class.db.NULL then - self[k] = nil - elseif self.__class.db.is_raw(v) then - returning = returning or { } - table.insert(returning, k) + local returning, return_all + if opts and opts.returning then + if opts.returning == "*" then + return_all = true + returning = { + self.__class.db.raw("*") + } + else + returning = { + unpack(opts.returning) + } + end + end + for k, v in pairs(update_fields) do + local _continue_0 = false + repeat + if v == self.__class.db.NULL then + _continue_0 = true + break + end + if self.__class.db.is_raw(v) then + returning = returning or { } + table.insert(returning, k) + end + _continue_0 = true + until true + if not _continue_0 then + break end end local res if returning then - res = self.__class.db.update(self.__class:table_name(), values, cond, unpack(returning)) - do - local update = unpack(res) - if update then - for _index_0 = 1, #returning do - local k = returning[_index_0] - self[k] = update[k] + res = self.__class.db.update(self.__class:table_name(), update_fields, cond, unpack(returning)) + else + res = self.__class.db.update(self.__class:table_name(), update_fields, cond) + end + local did_update = (res.affected_rows or 0) > 0 + if did_update then + for k, v in pairs(update_fields) do + if v == self.__class.db.NULL then + self[k] = nil + else + self[k] = v + end + end + if returning then + do + local result_row = unpack(res) + if result_row then + if return_all then + for k, v in pairs(result_row) do + self[k] = v + end + end + for _index_0 = 1, #returning do + local _continue_0 = false + repeat + local k = returning[_index_0] + if not (type(k) == "string") then + _continue_0 = true + break + end + self[k] = result_row[k] + _continue_0 = true + until true + if not _continue_0 then + break + end + end end end end - else - res = self.__class.db.update(self.__class:table_name(), values, cond) end - return (res.affected_rows or 0) > 0, res + return did_update, res end, refresh = function(self, fields, ...) if fields == nil then diff --git a/lapis/db/base_model.moon b/lapis/db/base_model.moon index acfdf68d..6a9f2300 100644 --- a/lapis/db/base_model.moon +++ b/lapis/db/base_model.moon @@ -671,28 +671,33 @@ class BaseModel -- col3: "Hello" -- } -- NOTE: this implementation depends on support for RETURNING sql synax + -- TODO: update by field name should be deprecated update: (first, ...) => cond = @_primary_cond! - columns = if type(first) == "table" - for k,v in pairs first + update_fields = {} -- the columns and their new values to update + local columns + + if type(first) == "table" + columns = for k,v in pairs first if type(k) == "number" + update_fields[v] = @[v] v else - @[k] = v + update_fields[k] = v k else - {first, ...} + columns = {first, ...} + for c in *columns + update_fields[c] = @[c] return nil, "nothing to update" if next(columns) == nil if @@constraints - for _, column in pairs columns - if err = @@_check_constraint column, @[column], @ + for column in *columns + if err = @@_check_constraint column, update_fields[column], @ return nil, err - values = { col, @[col] for col in *columns } - -- update options nargs = select "#", ... last = nargs > 0 and select nargs, ... @@ -701,7 +706,7 @@ class BaseModel if @@timestamp and not (opts and opts.timestamp == false) time = @@db.format_date! - values.updated_at or= time + update_fields.updated_at or= time if opts and opts.where assert type(opts.where) == "table", "Model.update: where condition must be a table or db.clause" @@ -716,26 +721,51 @@ class BaseModel where } - local returning - for k, v in pairs values - if v == @@db.NULL - @[k] = nil - elseif @@db.is_raw(v) + local returning, return_all -- TODO: verify that returning * works + + if opts and opts.returning + if opts.returning == "*" + return_all = true + returning = {@@db.raw "*"} + else + returning = {unpack opts.returning} + + for k, v in pairs update_fields + continue if v == @@db.NULL -- NULL is raw but handled specially + if @@db.is_raw v returning or= {} table.insert returning, k - local res - - if returning - res = @@db.update @@table_name!, values, cond, unpack returning - if update = unpack res - for k in *returning - @[k] = update[k] + res = if returning + @@db.update @@table_name!, update_fields, cond, unpack returning else - res = @@db.update @@table_name!, values, cond + @@db.update @@table_name!, update_fields, cond - (res.affected_rows or 0) > 0, res + did_update = (res.affected_rows or 0) > 0 + + -- if the update completed, store the values into self + if did_update + -- NOTE: this is redundant if the column name variant is used to issue an update + for k, v in pairs update_fields + if v == @@db.NULL + @[k] = nil + else + @[k] = v + + if returning + if result_row = unpack res + if return_all + for k, v in pairs result_row + @[k] = v + + -- we still have to iterate over the name list to ensure that we nil + -- out explicitly requested fields, since db.NULL is not returned in + -- the result set + for k in *returning + continue unless type(k) == "string" + @[k] = result_row[k] + did_update, res -- reload fields on the instance refresh: (fields="*", ...) => diff --git a/spec/model_spec.moon b/spec/model_spec.moon index f95fcf89..f0ef032c 100644 --- a/spec/model_spec.moon +++ b/spec/model_spec.moon @@ -467,17 +467,25 @@ describe "lapis.db.model", -> thing = Things\load { id: 12 } - -- no query is mocked + -- no query is mocked, update is considered not applied assert.same { false, {} }, { thing\update color: "green", height: 100 } - assert.same { height: 100, color: "green", id: 12 }, thing + assert.same { id: 12 }, thing -- no change to model instance due to failed update mock_query ".", { affected_rows: 1 } + -- query completes now, update is applied + assert.same { + true, {affected_rows: 1} + }, { + thing\update color: "green", height: 100 + } + assert.same { height: 100, color: "green", id: 12 }, thing + thing2 = Things\load { age: 2000, sprit: true } assert.same { true, {affected_rows: 1} @@ -507,6 +515,7 @@ describe "lapis.db.model", -> assert_queries { [[UPDATE "things" SET "color" = 'green', "height" = 100 WHERE "id" = 12]] + [[UPDATE "things" SET "color" = 'green', "height" = 100 WHERE "id" = 12]] [[UPDATE "things" SET "age" = 2000 WHERE "id" IS NULL]] [[UPDATE "timed_things" SET "great" = TRUE, "updated_at" = '2013-08-13 06:56:40' WHERE "a" = 2 AND "b" = 3]] @@ -514,6 +523,77 @@ describe "lapis.db.model", -> [[UPDATE "timed_things" SET "cat" = 'dog' WHERE "a" = 2 AND "b" = 3]] } + it "updates with db.null and db.raw", -> + mock_query ".", { affected_rows: 1, { + banned: false + }} + + class Whoa extends Model + + thing = Whoa\load { id: 92, color: "blue", banned: true } + + assert.same { + true, { + affected_rows: 1 + { banned: false } + } + }, { + thing\update { + color: db.NULL + banned: db.raw "created_at < now()" + } + } + + assert_queries { + [[UPDATE "whoa" SET "banned" = created_at < now(), "color" = NULL WHERE "id" = 92 RETURNING "banned"]] + } + + assert.same { + id: 92 + banned: false + }, thing + + it "updates with returning #ddd", -> + class Returnster extends Model + + mock_query ".", { affected_rows: 1, { + height: 4000 + ignore: "me" + }} + + r = Returnster\load { id: 17 } + + assert.true (r\update { + color: "blue" + }, returning: {"height"}) + + assert.same { + id: 17 + height: 4000 + color: "blue" + }, r + + r2 = Returnster\load { id: 18, zoot: "100", zat: "200" } + r2\update { + one: "two" + height: 300 + zoot: db.NULL + zat: db.raw "NULL" + }, returning: "*" + + assert.same { + id: 18 + one: "two" + height: 4000 + ignore: "me" + }, r2 + + + assert_queries { + [[UPDATE "returnster" SET "color" = 'blue' WHERE "id" = 17 RETURNING "height"]] + [[UPDATE "returnster" SET "height" = 300, "one" = 'two', "zat" = NULL, "zoot" = NULL WHERE "id" = 18 RETURNING *, "zat"]] + } + it "updates model with conditional", -> mock_query ".", { affected_rows: 1 } @@ -547,10 +627,14 @@ describe "lapis.db.model", -> update_id: db.NULL } + -- updated_at set because timestamp: true + assert type(thing2.updated_at) == "string" + assert.same { a: 2 b: 4 actor: "good" + updated_at: thing2.updated_at }, thing2 thing2\update { @@ -605,6 +689,26 @@ describe "lapis.db.model", -> [[UPDATE "timed_things" SET "color" = 'green' WHERE "a" = 2 AND "b" IS NULL AND ("age" = '10')]] } + it "updates model with conditional doesn't store on no update", -> + mock_query ".", { affected_rows: 0 } + + class Things extends Model + thing = Things\load { id: 12 } + + assert.same { + false, { affected_rows: 0 } + }, { + thing\update { color: "green", height: 100}, where: { color: "blue"} + } + + assert.same { + id: 12 + }, thing + + assert_queries { + [[UPDATE "things" SET "color" = 'green', "height" = 100 WHERE "id" = 12 AND ("color" = 'blue')]] + } + it "deletes model", -> mock_query [["id" = 2]], { affected_rows: 1 } @@ -1171,20 +1275,37 @@ describe "lapis.db.model", -> assert.same { nil, "missing `name`"}, { Things\create! } - it "should allow to update values on create and on update", -> + -- NOTE: Previously, there was undocumented support to modify the object + -- argument of the constraint in order to influence the update query. As of + -- version 1.16, this feature is no longer supported for the update method. + it "DEPRECATED: update values in constraint is undefined", -> mock_query "INSERT", { { id: 101 } } + mock_query "UPDATE", { affected_rows: 1 } class Things extends Model @constraints: { - name: (val, column, values) => values.name = 'changed from ' .. val + name: (val, column, values) => + values.name = 'changed from ' .. val + nil } thing = Things\create name: 'create' + + assert.same { + id: 101 + name: "changed from create" + }, thing + thing\update name: 'update' + assert.same { + id: 101 + name: "update" + }, thing + assert_queries { [[INSERT INTO "things" ("name") VALUES ('changed from create') RETURNING "id"]] - [[UPDATE "things" SET "name" = 'changed from update' WHERE "id" = 101]] + [[UPDATE "things" SET "name" = 'update' WHERE "id" = 101]] } describe "inheritance", ->