Skip to content

Respect missing values in session's conflict resolver#1230

Merged
gwenn merged 2 commits intorusqlite:masterfrom
xpepermint:master
Mar 29, 2024
Merged

Respect missing values in session's conflict resolver#1230
gwenn merged 2 commits intorusqlite:masterfrom
xpepermint:master

Conversation

@xpepermint
Copy link
Copy Markdown
Contributor

@xpepermint xpepermint commented Sep 30, 2022

When applying a changeset (e.g. for UPDATE), the conflict resolver’s item may include "missing" values since only changed columns are provided. Trying to read unexisting columns will cause the conflict resolver to panic.

The current code properly handles SqliteFailure related errors (e.g. ParameterOutOfRange) but will panic for operations performed on valid but at the same time missing values.

This small PR adds checks in ChangesetItem object that eliminate these issues and thus properly handle the Result<ValueRef> by throwing InvalidColumnIndex when a column is missing.

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 30, 2022 •
edited
Loading

Codecov Report

Base: 77.08% // Head: 77.16% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (47867c0) compared to base (a100adc).
Patch coverage: 86.20% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1230      +/-   ##
==========================================
+ Coverage   77.08%   77.16%   +0.07%     
==========================================
  Files          48       48              
  Lines        6223     6244      +21     
==========================================
+ Hits         4797     4818      +21     
  Misses       1426     1426
Impacted Files Coverage Δ
src/session.rs 59.72% <86.20%> (+1.49%) ⬆️
src/lib.rs 74.72% <0.00%> (+0.20%) ⬆️
src/error.rs 22.60% <0.00%> (+0.68%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Oct 2, 2022

https://sqlite.org/session/sqlite3changeset_new.html

If the change is an UPDATE and does not include a new value for the requested column, *ppValue is set to NULL and SQLITE_OK returned.

So only sqlite3changeset_new should be affected, no ?

@xpepermint
Copy link
Copy Markdown
Contributor Author

xpepermint commented Oct 2, 2022

I believe so ...

@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Oct 2, 2022

I believe so ...

I mean sqlite3changeset_old and sqlite3changeset_conflict should not be affected. But in your PR they are !

@xpepermint
Copy link
Copy Markdown
Contributor Author

xpepermint commented Oct 2, 2022

A precaution .is_null check should be there anyway.

@gwenn gwenn added the invalid label Oct 5, 2022
@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Apr 16, 2023

I checked again SQLite source code.
And sqlite3changeset_conflict cannot return a null value.
But sqlite3changeset_old can:
https://github.com/sqlite/sqlite/blob/99ace8263426ee9001d3994e6a8275d4fd8b8fc3/ext/session/sqlite3session.c#L3513-L3516

Or, if that particular value is not included in the record (because the change is an UPDATE and the field was not modified and is not a PK column), set *ppValue to NULL.

@gwenn gwenn merged commit 381be98 into rusqlite:master Mar 29, 2024
@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Mar 29, 2024

Sorry for the delay...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

invalid

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xpepermint @gwenn