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

Prevent schemas from using nullable type for top-level field definitions #104

Closed
achou11 opened this issue Aug 7, 2023 · 0 comments · Fixed by #122
Closed

Prevent schemas from using nullable type for top-level field definitions #104

achou11 opened this issue Aug 7, 2023 · 0 comments · Fixed by #122
Assignees

Comments

@achou11
Copy link
Member

achou11 commented Aug 7, 2023

We currently do not define any top-level schema fields as nullable because Protobuf doesn't have a proper representation of null. However, there's a misalignment between this and how SQLite works, where there's no undefined / void column type in SQLite and so optional fields are stored and read as null. In Mapeo Core, after reading values from SQLite (via Drizzle), we update fields with null values to undefined to match the Mapeo Schema definition of an optional field (link).

However, there's currently no protection against us defining a nullable top-level field in the schema, which will cause an issue at the SQLite-to-MapeoCore level because we will replace an intentional null value with undefined as opposed to leaving it untouched. This does not affect schema fields with a nested JSON-like value definition since we store the JSON as-is in SQLite.

Recommendations:

  • add a test to prevent maintainers from defining a top-level nullable field on a schema
  • update the schema generation implementation to warn/error if it detects a change that introduces this
@achou11 achou11 changed the title Prevent schemas from using nullable type for top-leve field definitions Prevent schemas from using nullable type for top-level field definitions Aug 7, 2023
@gmaclennan gmaclennan self-assigned this Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants