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

Add DatabaseValueConvertible tip for JSON columns #1649

Merged
merged 2 commits into from
Oct 6, 2024

Conversation

bok-
Copy link
Contributor

@bok- bok- commented Oct 6, 2024

This PR adds a tip to the JSON page of the DocC docs about conforming your JSON column's Codable property to DatabaseValueConvertible for filtering. Adding my first JSON column and this would have saved me a couple of hours of digging.

Please feel free to rewrite if it doesn't match the tone or intent of existing documentation.

Pull Request Checklist

  • CONTRIBUTING: You have read https://github.com/groue/GRDB.swift/blob/master/CONTRIBUTING.md
  • BRANCH: This pull request is submitted against the development branch.
  • DOCUMENTATION: Inline documentation has been updated.
  • DOCUMENTATION: README.md or another dedicated guide has been updated.
  • TESTS: Changes are tested.
  • TESTS: The make smokeTest terminal command runs without failure.

@bok- bok- changed the title Add DatabaseValueConvertible tip Add DatabaseValueConvertible tip for JSON columns Oct 6, 2024
@groue
Copy link
Owner

groue commented Oct 6, 2024

Hello @bok-,

Thank you for sharing your experience and suggesting this tip in the documentation!

I'm happy if your application can profit from this technique, because it is handy.

Now, the sample code has SQLite perform string comparison, which is sensitive to white space and key ordering, etc. It only works reliably if all database values in the address column have the exact same format as a serialized Address instance.

If this precondition is not fulfilled, the test will fail to match some addresses. There are plenty of reasons:

  • Some database addresses are produced by something else than a serialized Address.
  • The Address type has evolved over time, but the json columns were not migrated.
  • The JSONEncoder used by Address was customized and misses the .sortedKeys output formatting option.
  • etc.

In summary:

  • Not all apps can use this technique.
  • All developers that want to use this technique must understand the precondition and its consequences.
  • The precondition can not be tested. One can assert that one particular database works as expected, but it can't be asserted that all databases comply.
  • Failure may never happen on the developer's machine, or in the test suite, but it may happen on a user's device (immensely difficult to notice and debug).
  • Implementing the precondition as a CHECK constraint looks overwhelmingly hard and inefficient at runtime.

In summary, this technique is a clever hack. I love clever hacks, but documentation must not drive users into writing code that may fail them at some point. They would be upset, and rightly so.

Primum non nocere.

@groue
Copy link
Owner

groue commented Oct 6, 2024

Maybe it is possible to find a short and clear way to express the precondition.

What about:

Tip: Conform your Codable property to DatabaseValueConvertible if you want to be able to filter on specific values of it:

extension Address: DatabaseValueConvertible {}

// SELECT * FROM player
// WHERE "address" = '{"street": "...", "city": "...",  "country": "..."}'
let players = try Player
    .filter(JSONColumn("address") == Address(...))
    .fetchAll(db)

Take care that SQLite will compare strings, not JSON objects: make sure that the database contains values that are formatted exactly like a serialized Address (white-space, key ordering, etc.)

Copy link
Owner

@groue groue left a comment

Choose a reason for hiding this comment

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

I made up my mind :-) I hope you'll agree with this little amendment.

GRDB/Documentation.docc/JSON.md Show resolved Hide resolved
@groue groue merged commit 6a4f454 into groue:development Oct 6, 2024
1 of 8 checks passed
@groue
Copy link
Owner

groue commented Oct 6, 2024

Merged :-) Thank you @bok- 👍

@groue
Copy link
Owner

groue commented Oct 6, 2024

⛵ Shipped in 7.0.0-beta.3

@bok-
Copy link
Contributor Author

bok- commented Oct 8, 2024

Forgot to come back to this one! Yep totally agree with the improved wording and the warning about consequences! Thanks very much 🙏

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

Successfully merging this pull request may close these issues.

2 participants