Skip to content

Update chrono scalars according to the graphql-scalars.dev #1010

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

Merged
merged 118 commits into from
Mar 4, 2022

Conversation

ilslv
Copy link
Member

@ilslv ilslv commented Dec 23, 2021

Revealed from #1006 (review)

Synopsis

For now scalars from time and chrono crates are different.

Solution

Update chrono scalars according to the graphql-scalars.dev

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed

@ilslv ilslv added enhancement Improvement of existing features or bugfix semver::breaking Breaking change in terms of SemVer k::api Related to API (application interface) labels Dec 23, 2021
@ilslv ilslv self-assigned this Dec 23, 2021
@ilslv
Copy link
Member Author

ilslv commented Dec 23, 2021

FCM

Update `chrono` scalars according to the `graphql-scalars.dev` (#1010)

- remove `scalar-naivetime` feature

@ilslv ilslv marked this pull request as ready for review December 23, 2021 06:35
@ilslv ilslv changed the title Draft: Update chrono scalars according to the graphql-scalars.dev Update chrono scalars according to the graphql-scalars.dev Dec 23, 2021
@ilslv ilslv requested a review from tyranron December 23, 2021 07:41
@tyranron tyranron added k::integration Related to integration with third-party libraries or systems and removed k::api Related to API (application interface) labels Dec 24, 2021
[3]: https://docs.rs/chrono/*/chrono/offset/struct.FixedOffset.html",
specified_by_url = "https://graphql-scalars.dev/docs/scalars/date-time"
)]
impl<S> GraphQLScalar for DateTime<FixedOffset>
Copy link
Member

Choose a reason for hiding this comment

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

What we do really want here is

impl<S: ScalarValue, Tz: TimeZone> GraphQLScalar for DateTime<Tz>

but #[graphql_scalar] macro doesn't allows us to make generic impls.

So, let's first rewrite #[graphql_scalar] so we can do that.

Another problem arosen here is that chrono is very bad at relations between TimeZones. We have no relations like (From) between Utc, FixedOffset and Local, and the only thing how we can create Tz: TimeZone is TimeZone::from_offset(), where Offset is another generic parameter.

So we should write up some FromUtcOffset (or similar) machinery here, to wire them up, so, after parsing DateTime<FixedOffset> we can convert it into arbitrary DateTime<Tz>.

Copy link
Member Author

@ilslv ilslv Feb 24, 2022

Choose a reason for hiding this comment

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

@tyranron

So we should write up some FromUtcOffset (or similar) machinery here, to wire them up, so, after parsing DateTime<FixedOffset> we can convert it into arbitrary DateTime<Tz>.

All in all the main problem is that to convert into arbitrary DateTime<Tz> you need an instance of a type, not just type itself. So even with custom machinery we can support only FixedOffset and Utc. chrono-tz on the other hand, uses big enum instead of separate types to represent all timezones, so our users will have to implement needed timezones manually even to use them as outputs.

I propose different approach: we allow only DateTime<Utc> and DateTime<FixedOffsets> as inputs, while any DateTime<Tz> as output. So usage will look something like that:

#[graphql_object]
impl QueryRoot {
    fn transform(input: DateTime<Utc>) -> DateTime<chrono_tz::Tz> {
        input.with_timezone(&chrono_tz::CET)
    }
}

The only downside is that we won't be able to use macros to implement scalar and fallback to manual implementation. This happens because with don't have a mechanism to enforce different trait bounds on different resolvers.

UPD: it's wasn't so easy, as I expected, as trait bound are tightly coupled together. I'm investigating whether we can have different trait bounds on serialisation/deserialisation on the same scalar type.

UPD2: The main problem is with storing scalars in registry:

impl<'r, S: 'r> Registry<'r, S> {
    /// Creates a [`ScalarMeta`] type.
    pub fn build_scalar_type<T>(&mut self, info: &T::TypeInfo) -> ScalarMeta<'r, S>
    where
        T: GraphQLType<S> + FromInputValue<S> + ParseScalarValue<S>,
        T::Error: IntoFieldError<S>,
        S: ScalarValue,
    {
        let name = T::name(info).expect("Scalar types must be named. Implement `name()`");

        ScalarMeta::new::<T>(Cow::Owned(name.to_string()))
    }
}

Those trait bounds are reasonable, but enforce same trait bounds on both serialisation and deserialisation.

@@ -35,7 +35,7 @@ Juniper has built-in support for a few additional types from common third party
crates. They are enabled via features that are on by default.

* uuid::Uuid
* chrono::DateTime
* chrono::{DateTime, NaiveDate, NaiveDateTime, NaiveTime}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should update chrono-tz integration as well inside this PR.

ilslv added 19 commits February 21, 2022 09:01
# Conflicts:
#	integration_tests/juniper_tests/src/codegen/mod.rs
#	juniper/CHANGELOG.md
#	juniper_codegen/src/lib.rs
# Conflicts:
#	juniper/CHANGELOG.md
#	juniper/src/integrations/chrono.rs
# Conflicts:
#	integration_tests/juniper_tests/src/codegen/mod.rs
#	integration_tests/juniper_tests/src/codegen/scalar_attr_derive_input.rs
#	integration_tests/juniper_tests/src/codegen/scalar_attr_type_alias.rs
#	integration_tests/juniper_tests/src/codegen/scalar_value_transparent.rs
#	integration_tests/juniper_tests/src/custom_scalar.rs
#	juniper/CHANGELOG.md
#	juniper/src/lib.rs
#	juniper/src/types/scalars.rs
#	juniper/src/value/scalar.rs
#	juniper_codegen/src/derive_scalar_value.rs
#	juniper_codegen/src/graphql_scalar/attr.rs
#	juniper_codegen/src/graphql_scalar/derive.rs
#	juniper_codegen/src/graphql_scalar/mod.rs
#	juniper_codegen/src/lib.rs
#	juniper_codegen/src/result.rs
# Conflicts:
#	juniper/src/integrations/chrono.rs
#	juniper/src/integrations/chrono_tz.rs
# Conflicts:
#	integration_tests/codegen_fail/fail/scalar_value/missing_attributes.rs
#	integration_tests/codegen_fail/fail/scalar_value/missing_attributes.stderr
#	integration_tests/codegen_fail/fail/scalar_value/multiple_named_fields.rs
#	integration_tests/codegen_fail/fail/scalar_value/multiple_unnamed_fields.rs
#	integration_tests/juniper_tests/src/custom_scalar.rs
#	juniper/CHANGELOG.md
#	juniper/src/value/scalar.rs
#	juniper_codegen/src/lib.rs
@ilslv ilslv requested a review from tyranron March 4, 2022 10:00
/// Format of a [`Date` scalar][1].
///
/// [1]: https://graphql-scalars.dev/docs/scalars/date
const DATE_FORMAT: &str = "%Y-%m-%d";
Copy link
Member

Choose a reason for hiding this comment

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

It's better to move such constants inside module where they're used.

Tz: chrono::TimeZone,
Tz::Offset: fmt::Display,
{
Value::scalar(v.to_rfc3339_opts(SecondsFormat::AutoSi, true))
Copy link
Member

Choose a reason for hiding this comment

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

According to the spec we should convert into UTC here first.

/// ```
///
/// [CET]: https://en.wikipedia.org/wiki/Central_European_Time
pub trait FromFixedOffset: TimeZone {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can throw this squat out, because according to the spec we should convert into UTC, as we do for time crate. So allowing to preserve the input timezone in some cases is a helpful hatch in some cases, not the strict requirement.

@tyranron tyranron removed the blocked label Mar 4, 2022
@tyranron tyranron merged commit fec998d into master Mar 4, 2022
@tyranron tyranron deleted the chrono-scalars branch March 4, 2022 15:53
@tyranron tyranron added this to the 0.16.0 milestone Apr 13, 2022
LukasKalbertodt added a commit to LukasKalbertodt/tobira that referenced this pull request Dec 11, 2024
The update was easier than expected. Here are some of the bigger
changes:

- Our custom graphql scalars needed some rewriting, as they are defined
  differently now.
- Lots duplicate methods were removed where the main function was
  defined in a normal `impl` block and the `graphql_object` impl block
  contained a method with same name and just called the first one. With
  this update, Juniper automatically adds all methods defined in the
  special impl block as normal methods, so this dance is not necessary
  anymore.
- This also updates GraphiQL, which looks nicer now. The changelog
  mentions that this requires websocket integration now, but I can't
  find any information about this and everything seems to work.
- The generated schema seems to have a deterministic order, which is
  great.
- Also see below.

I ran `graphql-inspector diff` on schema.graphql and the following
changes were printed. For one, that means no one has to manually review
schema.graphql: lots of stuff moved around, but hardly anything actually
changed. Below I explain every change.

✖  Type DateTimeUtc was removed
✔  Type DateTime was added
✖  Field AuthorizedEvent.created changed type from DateTimeUtc! to DateTime!
✖  Field AuthorizedEvent.tobiraDeletionTimestamp changed type from DateTimeUtc to DateTime
✖  Input field Filters.end changed type from DateTimeUtc to DateTime
✖  Input field Filters.start changed type from DateTimeUtc to DateTime
✖  Field SearchEvent.created changed type from DateTimeUtc! to DateTime!
✖  Field SearchEvent.endTime changed type from DateTimeUtc to DateTime
✖  Field SearchEvent.startTime changed type from DateTimeUtc to DateTime
✖  Field Series.created changed type from DateTimeUtc to DateTime
✖  Field SyncedEventData.endTime changed type from DateTimeUtc to DateTime
✖  Field SyncedEventData.startTime changed type from DateTimeUtc to DateTime
✖  Field SyncedEventData.updated changed type from DateTimeUtc! to DateTime!

The integration of `chrono` was slightly adjusted to better comply with public
standards. See graphql-rust/juniper#1010
Not really a problem for us, I just adjusted the relay config.

✖  AuthorizedEventData object type no longer implements Node interface
✖  SyncedEventData object type no longer implements Node interface

Removed some useless and wrong `impl = NodeValue` attributes, that were ignored
before and now error. This was a mistake, those types cannot implement `Node` as
they don't have an ID. I am confused how this compiled before.

✖  Type RealmNameSourceBlock was removed
✖  Field RealmNameFromBlock.block changed type from RealmNameSourceBlock! to Block!
✖  PlaylistBlock object type no longer implements RealmNameSourceBlock interface
✖  SeriesBlock object type no longer implements RealmNameSourceBlock interface
✖  VideoBlock object type no longer implements RealmNameSourceBlock interface

Removed `RealmNameSourceBlock` interface: it did not serve any practical
purpose. It only hinted at the fact that not all blocks can be used as name
source, but again, not actually needed for API or type safety. And making it
work was annoying.

✖  Type for argument order on field User.myVideos changed from EventSortOrder to EventSortOrder!

This is now using a proper "default value" that GraphQL understands. It's
`foo: Baz! = ...` which is correct according to the GraphQL spec. The argument
can still be omitted, in which case the default is used. Curiously, GraphiQL
does not seem to understand that yet. In any case, the runtime behavior did
not change.

⚠  AuthorizedPlaylist object implements Node interface
⚠  Series object implements Node interface

`Series` and `AuthorizedPlaylist` implement `Node` now. This was just an
oversight before and caused compiler errors after the update.

⚠  Default value [] was added to argument newRealms on field Mutation.mountSeries

The runtime behavior was like this already, but now it's properly
specified in the schema.

✔  Description A string in different languages on type TranslatedString has changed to A string in different languages.

Added a '.' at the end.
LukasKalbertodt added a commit to LukasKalbertodt/tobira that referenced this pull request Dec 11, 2024
The update was easier than expected. Here are some of the bigger
changes:

- Our custom graphql scalars needed some rewriting, as they are defined
  differently now.
- Lots duplicate methods were removed where the main function was
  defined in a normal `impl` block and the `graphql_object` impl block
  contained a method with same name and just called the first one. With
  this update, Juniper automatically adds all methods defined in the
  special impl block as normal methods, so this dance is not necessary
  anymore.
- This also updates GraphiQL, which looks nicer now. The changelog
  mentions that this requires websocket integration now, but I can't
  find any information about this and everything seems to work.
- The generated schema seems to have a deterministic order, which is
  great.
- Also see below.

I ran `graphql-inspector diff` on schema.graphql and the following
changes were printed. For one, that means no one has to manually review
schema.graphql: lots of stuff moved around, but hardly anything actually
changed. Below I explain every change.

✖  Type DateTimeUtc was removed
✔  Type DateTime was added
✖  Field AuthorizedEvent.created changed type from DateTimeUtc! to DateTime!
✖  Field AuthorizedEvent.tobiraDeletionTimestamp changed type from DateTimeUtc to DateTime
✖  Input field Filters.end changed type from DateTimeUtc to DateTime
✖  Input field Filters.start changed type from DateTimeUtc to DateTime
✖  Field SearchEvent.created changed type from DateTimeUtc! to DateTime!
✖  Field SearchEvent.endTime changed type from DateTimeUtc to DateTime
✖  Field SearchEvent.startTime changed type from DateTimeUtc to DateTime
✖  Field Series.created changed type from DateTimeUtc to DateTime
✖  Field SyncedEventData.endTime changed type from DateTimeUtc to DateTime
✖  Field SyncedEventData.startTime changed type from DateTimeUtc to DateTime
✖  Field SyncedEventData.updated changed type from DateTimeUtc! to DateTime!

The integration of `chrono` was slightly adjusted to better comply with public
standards. See graphql-rust/juniper#1010
Not really a problem for us, I just adjusted the relay config.

✖  AuthorizedEventData object type no longer implements Node interface
✖  SyncedEventData object type no longer implements Node interface

Removed some useless and wrong `impl = NodeValue` attributes, that were ignored
before and now error. This was a mistake, those types cannot implement `Node` as
they don't have an ID. I am confused how this compiled before.

✖  Type RealmNameSourceBlock was removed
✖  Field RealmNameFromBlock.block changed type from RealmNameSourceBlock! to Block!
✖  PlaylistBlock object type no longer implements RealmNameSourceBlock interface
✖  SeriesBlock object type no longer implements RealmNameSourceBlock interface
✖  VideoBlock object type no longer implements RealmNameSourceBlock interface

Removed `RealmNameSourceBlock` interface: it did not serve any practical
purpose. It only hinted at the fact that not all blocks can be used as name
source, but again, not actually needed for API or type safety. And making it
work was annoying.

✖  Type for argument order on field User.myVideos changed from EventSortOrder to EventSortOrder!

This is now using a proper "default value" that GraphQL understands. It's
`foo: Baz! = ...` which is correct according to the GraphQL spec. The argument
can still be omitted, in which case the default is used. Curiously, GraphiQL
does not seem to understand that yet. In any case, the runtime behavior did
not change.

⚠  AuthorizedPlaylist object implements Node interface
⚠  Series object implements Node interface

`Series` and `AuthorizedPlaylist` implement `Node` now. This was just an
oversight before and caused compiler errors after the update.

⚠  Default value [] was added to argument newRealms on field Mutation.mountSeries

The runtime behavior was like this already, but now it's properly
specified in the schema.

✔  Description A string in different languages on type TranslatedString has changed to A string in different languages.

Added a '.' at the end.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems semver::breaking Breaking change in terms of SemVer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants