-
Notifications
You must be signed in to change notification settings - Fork 25
Fix timestamp representation #331
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
Conversation
🦋 Changeset detectedLatest commit: b8338d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
da0954b to
d9a8f65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure I posted this earlier, but can't find it now...
I'm happy with the implementation, but I'm wondering if we could phrase the settings as "config" rather than "quirks", and provide some form of version number to easily get the latest recommended defaults. I'm thinking something like this (other ideas welcome):
config:
edition: 2 # single version number to select a set of defaults - we can include this by default in new sync rules. Another option here is `preset`
timestamps_iso8601: true # or set this one directly
json_extract_quirks: false # not sure if there's a better name here
versioned_bucket_ids: trueFor reference, some examples I know of from other projects:
Rust:
edition = "2024"Rails:
config.load_defaults "6.1"
config.yjit = true|
I agree with giving the configuration keys a more positive name 👍 How would we deal with a combination of |
Migration could perhaps be easier if we do also allow explicitly opting in to the old behavior. In that case we'd default to say edition 2 for sync streams, but still allow specifying edition 1. Does that complicate anything in the implementation? |
Not substantially - just need more tests. Did we want want to allow sync streams and sync rules in the same file to ease the transition? I think there was a discussion on this at some point, but I don't remember the outcome (edit: we do, but secretly). But if so, I think defaulting to I think my preferred approach would be to:
This approach is a bit similar to some features requiring new editions in Rust, and step 6 is similar to how Dart 3.x still supports older language versions but at least |
|
I'm happy with that approach. I'm starting to think it makes sense to just encourage specifying an explicit edition (i.e. have it in all our templates), rather than setting the default with sync streams.
Before we drop support for older editions we'll need to have a proper versioning and support policy. For example, we could potentially drop it in version 2.x or 3.x of the service, while still maintaining some level of support (bugfixes) for older major versions. |
32c548a to
1ce5d65
Compare
|
I've implemented that approach now 👍 |
Co-authored-by: Ralf Kistner <[email protected]>
The mongo and postgres replicators used to represent timestamp values with a space between the date and time parts, whereas ISO-6801 would require the letter
T. While we would like to align the format with that standard, doing so is a backwards-incompatible change since existing rows would sync differently.This PR introduces two new concepts to fix this issue:
SqliteInputValue: This expands theSqliteValuetype used by sync rules to represent SQLite rows to also support aCustomSqliteValue. Values of that type can decay dynamically intoSqliteValueby giving them additional context. We can thus represent date values asCustomSqliteValues, that, depending on context, use the legacy or the ISO-6801 format.CustomSqliteValues, this adds theQuirkclass representing a historical issue of the sync service we can't fix without a backwards-incompatible change. Users can opt in to fixes with thefixed_quirksoption in the sync-rules config, and there's also aCompatibilityLevelto fix some quirks by default. The idea is that streams would implicitly raise that level internally.When evaluating parameter and data rows,
CustomSqliteValues are mapped into the actual values using the context specified in sync rules.The Mongo and Postgres adapters have been fixed to return these custom values instead of applying a fixed format for date values.
Users can opt into this fix by adding this section to their sync rules:
There are a number of other issues also worth fixing (in particular, #299, handling postgis types properly, fixing the
->>operator, encoding sync rule id in bucket identifiers). Since most of this PR is just laying the groundwork for those changes, I'll open additional PRs for those.Closes #286.