Desktop,Mobile,Cli: Fixes #11630: Adjust how items are queried by ID #11734
Conversation
| @@ -345,13 +345,17 @@ class BaseModel { | |||
| return this.modelSelectAll(q.sql, q.params); | |||
| } | |||
|
|
|||
| public static escapeIdsForSql(ids: string[]) { | |||
| return this.db().escapeValues(ids).join(', '); | |||
There was a problem hiding this comment.
The solution and usage of escapeIdsForSql looks ok, but I just have one question:
How / are single quotes added at the start and end of the contents of the IN clause?
The previous logic used:
IN ('${ids.join('','')}')
new logic uses:
(${this.escapeIdsForSql(ids)})
So I don't understand how enclosing quotes can be added when they are omitted for usages of the escapeIdsForSql function and not added within the function.
There was a problem hiding this comment.
In addition to escaping enclosed quotes by doubling them, .escapeValues surrounds each of the IDs in 's.
There was a problem hiding this comment.
Ok, that makes sense. Didn't notice the join here no longer includes quotes in the value
| 'Test\'\'\'a\'\'', | ||
| '% test', | ||
| ])('should support querying items with IDs containing special characters (id: %j)', async (id) => { | ||
| const note = await Note.save({ id }, { isNew: true }); |
There was a problem hiding this comment.
It would be good to add a test which includes more than 1 id passed at the same time, so that the join logic is tested
There was a problem hiding this comment.
Thank you for the feedback! That would be useful.
Note that for standard IDs, there's already several tests that indirectly test this case. For example,
joplin/packages/lib/models/Note.test.ts
Line 68 in 9b82578
However, these tests only check standard IDs without 's.
Summary
This pull request adjusts how ID SQL queries are written. Follow-up to #11657.
Fixes #11630.