-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Strip primary keys from the update payload #375
Conversation
🦋 Changeset detectedLatest commit: 64ae41f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Someone is attempting to deploy a commit to a Personal Account owned by @psteinroe on Vercel. @psteinroe first needs to authorize it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 97.29% 91.79% -5.51%
==========================================
Files 145 48 -97
Lines 2111 634 -1477
Branches 520 138 -382
==========================================
- Hits 2054 582 -1472
+ Misses 57 45 -12
- Partials 0 7 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@lauri865 can you write a changes? |
Added changest @psteinroe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a test 👍
|
||
if (stripPrimaryKeys) { | ||
payload[key] = undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not happen before we pass the payload to qb.update
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't since we don't call the query before, and qb.update just sets a stable reference to the payload object. Thought it would keep the code simpler for future changes if we do related manipulations in one pass.
It should have tests though, I agree with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to move it up - makes it simpler to read and there is no other reason to put it down.
@psteinroe this seems like a bug worth fixing, did you close it because of a lack of response from @lauri865? I might have time to continue working on this if you're still interested in a fix (I've just ran into it on my first attempt to use |
@elyobo yes, if you have the time I would appreciate this getting fixed very much 🙏🏼 |
Happy to take a poke at it, but it seems like tests are failing even before running any changes; after installing with Edit: looking through the Edit: going through all the steps (build packages, run supabase, run tests with concurrency 1) generally works, although there are some intermittent failures. Should be enough to get things working. |
Fixes #374.