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

No validation on date parsing #188

Closed
epage opened this issue Sep 9, 2021 · 5 comments · Fixed by #631
Closed

No validation on date parsing #188

epage opened this issue Sep 9, 2021 · 5 comments · Fixed by #631
Labels
A-parse Area: Parsing TOML C-bug Category: Things not working as expected

Comments

@epage
Copy link
Member

epage commented Sep 9, 2021

With #187, we dropped most of our date validation, like 2021-02-31 now works.

Looks like chrono has some pretty complex lookup logic to prevent that

Playground of it erroring

@epage epage added the C-bug Category: Things not working as expected label Sep 9, 2021
@epage
Copy link
Member Author

epage commented Sep 9, 2021

Options

  • Revert the change
  • Add a feature flag for full date validation, which pulls in chrono
  • Do nothing (like toml-rs)

@ordian
Copy link
Member

ordian commented Sep 10, 2021

Curious how other parsers handle that (e.g. in other languages). It seems toml spec specifies the date should follow the https://datatracker.ietf.org/doc/html/rfc3339. And if chrono panics on construction instead of returning an error, that's not good either, but I would lean towards Do nothing and documenting this behavior.

@epage
Copy link
Member Author

epage commented Sep 10, 2021

Chrono offers panicing and Option variants of its constructors

Looking at the RFC, it seems like it shouldn't be too bad to address days of month. I assume the complexity in chrono is coming from other functionality.

Leap seconds is an interesting one. If you don't bother with a lookup table, it simplifies things but knowing whether you can have a leap second or not is dependent on the month. So a date-less time can't do validation and combining a date and time can error.

@epage
Copy link
Member Author

epage commented Sep 10, 2021

As for other languages, toml-test validates that toml parsers reject months with 50 days but doesn't validate further. So parsers using that test (like go) at least validate that much.

Python's toml loads into datetime which handles leap years. I did not verify how it handles leap seconds

@parasyte
Copy link
Contributor

parasyte commented Nov 3, 2021

If my preference is at all useful here, I would like chrono behind a feature flag if you intend to add it again.

The project where I use toml_edit already has a ton of dependencies. And while chonro is not large compared to everything else I depend on, I do try to keep the tree as thin as possible by removing unnecessary features. A feature flag would just be really nice to have.

@epage epage added the A-parse Area: Parsing TOML label Sep 23, 2022
@epage epage changed the title No validation on date No validation on date parsing Jan 27, 2023
epage added a commit to epage/toml_edit that referenced this issue Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parse Area: Parsing TOML C-bug Category: Things not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants