fix(model): falsy values and soft-deleted rows leaking through the model layer - #6
Merged
Merged
Conversation
…alue NormRecord:__init used `persisted and model:parse(col, value) or value`. When parse() returns false for a boolean column, that idiom falls through to the raw driver value, so `admin = 0` stayed the number 0, which is truthy in Lua. Every `if record.admin then` was true for non-admins. Replaced with an explicit branch and added regression tests covering 0/1 decoding and reload().
…thing
Three defects in the include() path, two of them the same and/or idiom:
resolve(single and nil or records) -- first() resolved {} on no match
(kind == "has_one") and (g[1] or nil) or g -- empty has_one became {}
Both produced a truthy empty table where the contract, and the lazy load
path, return nil. `if post then post:save() end` passed the guard and
then failed on the method call.
The third: when no parent had a usable key, one single empty table was
assigned to every parent, so inserting into one parent's collection was
visible on all the others. Each parent now gets its own table.
_find_by_attrs built its state without utils.soft_scope, unlike find, find_by and every relation loader. On a soft-deleting model, find_or_create, find_or_new and update_or_create therefore matched rows that had been deleted: instead of creating a new row they revived a trashed one, and update_or_create wrote into it.
upsert() reads the canonical row back by its conflict columns, which default to the primary key. With an auto-increment key the caller does not supply it, so the read-back compiled to `WHERE id IS NULL` and the promise resolved nil even though the row had been written. The existing assertion only checked that the conflict list was non-empty. The columns are now required in the data, and the read-back applies the soft-delete scope like every other lookup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four fixes in the model and relation layer. Follow-up to #1, already merged.
Booleans were never decoded.
NormRecord:__initusedpersisted and model:parse(col, value) or value.parse()returnsfalsefor a boolean column, which that idiom swaps back to the raw driver value, soadmin = 0stayed the number0. That is truthy in Lua, soif user.admin thenwas true for every non-admin.reload()cast it correctly, so the same record changed type after reloading. The existing test only covered1 -> true, which passes by chance.Eager loading returned an empty table instead of nil. The same idiom, twice:
first()combined withinclude()resolved{}on no match, and an emptyhas_onebecame{}. Both are truthy, soif post then post:save() endpassed the guard and failed on the call. The lazyload()path returned nil correctly, and so didbelongs_to.Related, in the same function: when no parent had a usable key, one single empty table was assigned to every parent, so inserting into one parent's collection showed up on all the others.
find_or_ ignored the soft-delete scope.*
_find_by_attrsnever applied it, unlike find, find_by, load and every relation loader. On a soft-deleting model,find_or_createrevived a trashed row instead of creating a new one, andupdate_or_createwrote into it.upsert resolved nil after a successful write. The row is read back by the conflict columns, which default to the primary key. With an auto-increment key the caller does not supply it, so the read-back compiled to
WHERE id IS NULL. The existing assertion only checked that the conflict list was non-empty. The columns are now required in the payload, and the read-back applies the soft-delete scope like every other lookup.Suite: 315 passing.