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

[APE] Fix DATETIME format using serialize of DateTime #38

Merged
merged 2 commits into from
May 22, 2024

Conversation

JMGDoctolib
Copy link
Member

@JMGDoctolib JMGDoctolib commented Apr 19, 2024

The format was not well manage for the DateTime and Time.

We now apply the iso8601 format to DateTime and Time and the caller is not obliged to format the dates when using CB ORM like that:

 Meet.where(started_at: @period_start.iso8601..@period_end.iso8601)

Why ?

When building a select query containing a data param with time, example

Meet.where(started_at: @period_start..@period_end)

When, the @period_end is an ActiveSupport::TimeWithZone, on interpollation time, it gives 2024-01-31 23:59:59 +0100

Then, the query above yields a n1ql wih the following clause :

AND started_at >= '2024-01-01 00:00:00 +0100' AND started_at <= '2024-01-31 23:59:59 +0100'

Note the date formats in that clause.
However, the started_at MIGHT/MUST/SHOULD be formated differently, like this : 2024-01-31T13:00:00Z

Because of this format miss-match, all documents having a started_at at the last date of the period are missed by the query.

It seems that CB compares dates as strings, When I run this query:
select '2024-01-31T13:00:00Z' < '2024-01-31 23:59:59 +0100'
I get "$1": false Because the char T is grater than the char space

@JMGDoctolib JMGDoctolib self-assigned this Apr 19, 2024
@JMGDoctolib JMGDoctolib requested a review from a team as a code owner April 19, 2024 14:53
Copy link
Member

@pimpin pimpin left a comment

Choose a reason for hiding this comment

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

Before merging change on date management, I guess we should think again.
I'd love to have a meet on that.
IMHO The main question to get answered before merging is :
Do we have some guarantee that stored date and or data time attributes have always the same format, or at least a format that is compatible with that change ?
I guess we should do a large review of the spec files to ensure it.

@pimpin
Copy link
Member

pimpin commented May 21, 2024

I will merge it as soon as we ran all e2e tests in our repo using CBOrm. See https://github.com/doctolib/doctolib/pull/164446

@pimpin pimpin merged commit 6bd5178 into master May 22, 2024
4 checks passed
@pimpin pimpin deleted the APE-FIX-DATETIME branch May 22, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants