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 #1393 - Parse datetime to spec without panic #1550

Merged
merged 7 commits into from Jan 15, 2023

Conversation

AL1L
Copy link
Contributor

@AL1L AL1L commented Dec 25, 2022

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

What is the motivation?

See #1393 - Illegal datetimes panic, such as November 31st (2022-11-31T12:00:00.000Z)

What does this change do?

Unifies the parsing of datetimes into one function that errors when invalid dates are entered. The errors comply with how nom expects it to and will try a new branch when one does occur; this results in nom parsing the invalid datetime as a string.

Additionally, I have changed the ranges for seconds and hours to 0..=60 and 0..=23 respectively. The 60 for seconds is for leap seconds, and 24 is an invalid hour in the ISO 8601 spec (and RFC 3339)

What is your testing strategy?

Created a new test, date_time_illegal_date, in lib/src/sql/datetime.rs

Is this related to any issues?

Fixes #1393

Have you read the Contributing Guidelines?

.from_local_date(&n)
.unwrap()
.and_hms_nano_opt(hour, min, sec, nano)
.ok_or(Err::Error(error_position!(i, ErrorKind::Verify)))?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the correct way to send errors, tbh GitHub CoPilot wrote it. It works in my testing though.

@AL1L AL1L changed the title Fix #1393 - Parse datetime with proper errors Fix #1393 - Better datetime parsing Dec 25, 2022
}

fn minute(i: &str) -> IResult<&str, u32> {
take_digits_range(i, 2, 0..=59)
}

fn second(i: &str) -> IResult<&str, u32> {
take_digits_range(i, 2, 0..=59)
// 60 is for leap seconds
take_digits_range(i, 2, 0..=60)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See RFC 3339 §5.6, I don't have access to ISO 8601, so this is the closest thing.

@tobiemh you should buy ISO 8601-1 and ISO 8601-2 for me and I'll redo all this parsing to spec.

@AL1L AL1L changed the title Fix #1393 - Better datetime parsing Fix #1393 - Parse datetime to spec without panic Dec 25, 2022
@tobiemh tobiemh added this to the v1.0.0-beta.9 milestone Jan 15, 2023
@tobiemh tobiemh merged commit 07a2e5f into surrealdb:main Jan 15, 2023
@AL1L
Copy link
Contributor Author

AL1L commented Jan 16, 2023

@tobiemh can I ask, why did you change the hours to 0..=24 when the spec (RFC 3339 § 5.6) says it's 0..=23 and even the documentation says that method will return None if it's >= 24. You can see the check in the native time code here

Also, while looking at the code I noticed leap seconds are actually handled by passing greater than 1,000,000,000 nanoseconds and not setting the second to 60.

You can test these both by running this query

CREATE test:invalid_hour SET date = "2012-12-30T24:59:59.5631Z";
SELECT date, type::int(date), date - 1h FROM test:invalid_hour;

CREATE test:invalid_seconds SET date = "2012-12-31T0:59:60.5631Z";
SELECT date, type::int(date), date - 1h FROM test:invalid_seconds;

Notice these don't give you a datietime, but a string

@tobiemh
Copy link
Member

tobiemh commented Jan 16, 2023

Hey @AL1L yeah that might not parse as a datetime just yet, because of the Chrono library, but in October 22 there was an amendment to the ISO8601 specification...

As of ISO 8601-1:2019/Amd 1:2022, midnight may be referred to as "00:00:00", corresponding to the instant at the beginning of a calendar day; or "24:00:00", corresponding to the instant at the end of a calendar day. ISO 8601-1:2019 as originally published removed "24:00" as a representation for the end of day although it was permitted in earlier versions of the standard.

@tobiemh
Copy link
Member

tobiemh commented Jan 16, 2023

Hmmm @AL1L maybe we should just put it back to 23 (not that it affects the output or anything)...

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.

Bug: Trying to add invalid dates to the database causes it to crash
2 participants