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(core): deal with unqualified date times in local offsets #553

Merged
merged 6 commits into from
Jan 6, 2023

Conversation

mttrbrts
Copy link
Member

@mttrbrts mttrbrts commented Nov 29, 2022

This PR adds a new option to the JSONGenerator and JSONPopulator classes (and the Serializer) called strictQualifiedDateTimes. This allows client applications to drop support for unqualified, and ambiguous dates/datetimes.

This solves #552 in a backward-compatible way by not-allowing dates and date-times that don't have an offset. This also has the side-effect of making the utcOffset parameter only relevant to serialization; i.e. it becomes a rendering option. We always trust the offset provided when strictQualifiedDateTimes is true

When strictQualifiedDateTimes is true, only the following date-time formats are accepted.

  • YYYY-MM-DDTHH:mm:ssZ
  • YYYY-MM-DDTHH:mm:ss.SZ
  • YYYY-MM-DDTHH:mm:ss.SSZ
  • YYYY-MM-DDTHH:mm:ss.SSSZ
  • YYYY-MM-DDTHH:mm:ss+HH:mm
  • YYYY-MM-DDTHH:mm:ss.S+HH:mm
  • YYYY-MM-DDTHH:mm:ss.SS+HH:mm
  • YYYY-MM-DDTHH:mm:ss.SSS+HH:mm
  • YYYY-MM-DDTHH:mm:ss-HH:mm
  • YYYY-MM-DDTHH:mm:ss.S-HH:mm
  • YYYY-MM-DDTHH:mm:ss.SS-HH:mm
  • YYYY-MM-DDTHH:mm:ss.SSS-HH:mm

There are lots of formats that are no-longer accepted, for example (see the test for the full set):

  • YYYY-MM-DD
  • YYYY-MM-DDTHH:mm:ss
  • YYYY-MM-DDTHH:mm:ss.S
  • YYYY-MM-DDTHH:mm:ss.SS
  • YYYY-MM-DDTHH:mm:ss.SSS

Changes

  • Adds lots of format tests for the serialization/deserialization logic.
  • Adds a new constructor argument called strictQualifiedDateTimes to JSONGenerator and JSONPopulator.

Flags

  • This is a candidate for becoming the default behaviour in future.
  • Currently, the existing behaviour is retained for backwards compatibility. This does permit some unexpected behaviour as described in Concerto assumes UTC for unqualified "Local Time" DateTime values #552.
  • When using the strictQualifiedDateTimes flag, this has the effect of removing support dates (without explicit times).
  • Client applications such as Cicero, should take responsibility for (de)normalizing to this format.

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

Signed-off-by: Matt Roberts <code@rbrts.uk>
Signed-off-by: Matt Roberts <code@rbrts.uk>
Signed-off-by: Matt Roberts <code@rbrts.uk>
Signed-off-by: Matt Roberts <code@rbrts.uk>
Signed-off-by: Matt Roberts <code@rbrts.uk>
Signed-off-by: Matt Roberts <code@rbrts.uk>
@mttrbrts mttrbrts marked this pull request as ready for review January 3, 2023 12:57
@mttrbrts mttrbrts requested review from a team and stefanblaginov January 3, 2023 12:57
Copy link
Sponsor Contributor

@dselman dselman left a comment

Choose a reason for hiding this comment

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

Great!

@mttrbrts mttrbrts merged commit 20315ea into accordproject:main Jan 6, 2023
@mttrbrts mttrbrts deleted the mr-unqualified-dates branch January 6, 2023 13:00
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 this pull request may close these issues.

None yet

2 participants