Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Desktop,Mobile,Cli: Fixes #11630: Adjust how items are queried by ID #11657

Closed
wants to merge 5 commits into from
Closed

Desktop,Mobile,Cli: Fixes #11630: Adjust how items are queried by ID #11657

wants to merge 5 commits into from
+117 −45

Conversation

personalizedrefrigerator
Copy link
Collaborator

Summary

This pull request adjusts how items are queried by ID.

Fixes #11630.

@personalizedrefrigerator
Desktop,Mobile,Cli: Fixes laurent22#11630: Adjust how items are queri…
e969d7e 
…ed by ID

Note: One possible alternative would be to validate IDs when fetching
from the sync target. However, IDs can also be created from other sources
(e.g. plugins).
@personalizedrefrigerator
Fix invalid SQL
f245085
@personalizedrefrigerator
Fix indentation
b17fc09
@personalizedrefrigerator
Revert changes to database.ts
b888206 
These changes should go in a separate pull request.
@personalizedrefrigerator
Tests
3933180
personalizedrefrigerator
personalizedrefrigerator commented Jan 15, 2025
View reviewed changes
packages/lib/BaseModel.ts
@@ -345,15 +351,24 @@ class BaseModel {
return this.modelSelectAll(q.sql, q.params);
}

public static whereIdsInSql({ ids, field = 'id', negate = false }: IdsWhereClauseOptions) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whereIdsInSql is responsible for generating the ids IN (...) clause used elsewhere.

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Jan 15, 2025 •
edited
Loading

Possible issue: From @mrjo118:

Queries which make use of the IN clause could use bind parameters to protect against sql injection, however there is a limit to the number of bind parameters which you can set on a query in sqlite, and it may be too low if the user syncs a very outdated profile which has since had a lot of changes.

Indeed, from SQLite limits:

To prevent excessive memory allocations, the maximum value of a host parameter number is SQLITE_MAX_VARIABLE_NUMBER, which defaults to 999 for SQLite versions prior to 3.32.0 (2020-05-22) or 32766 for SQLite versions after 3.32.0.

The above might cause issues during sync, especially on old Android versions with an old version of SQLite. I'm closing this pull request for now, until the above has been resolved.

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
None yet
Projects
None yet
Milestone
No milestone
1 participant
@personalizedrefrigerator