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.

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

Fix case-sensitivity of region-based database observation #956

Merged
merged 6 commits into from
Apr 10, 2021

Conversation

groue
Copy link
Owner

@groue groue commented Apr 7, 2021

SQLite is not a case-sensitive database. A table named Player can be named player or PLAYER in SQL requests, without any consequence.

GRDB database observation features ought to support this behavior. When your app observes a request on player, Player or PLAYER, it should be notified of changes applied to player, Player or PLAYER, regardless of the actual name of the database table (player, Player or... PLAYER, yes).

#954 has revealed it was not the case.

When the database table had a name (Player) that did not exactly match the databaseTableName of a record type (player), and you observe Player.fetchCount(db), then all stars would get aligned and trigger a bug:

  • The SELECT COUNT(*) FROM player is one of the rare SQL requests that SQLite does not expose to Compile-Time Authorization Callbacks in a regular way. The authorizer is not given the canonical table name used in the schema (Player), but the raw name used in the SQL request: player.
  • Thus GRDB would track player.
  • And it would miss all subsequent changes performed on Player.

#954 has revealed another bug, with unclear consequences, in the union of database region (see #954 (comment)).

This pull request fixes both bugs by making DatabaseRegion fully case-insensitive.

@groue groue force-pushed the dev/observation-case-insensitivity branch from 73c6773 to a44c518 Compare April 10, 2021 14:46
@groue groue merged commit c0f537c into development Apr 10, 2021
@groue groue deleted the dev/observation-case-insensitivity branch April 10, 2021 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant