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

TBD: Make parsing of CalDateTime strings more restrictive #734

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

minichma
Copy link
Collaborator

TBD: This PR makes the parsing of DATE/DATE-TIME values more restrictive, i.e. rejects if

  • TZID and a Z suffix are present
  • TZID is present and the value type is DATE, i.e. date-only

This needs to be discussed as lib users may run into issues when parsing non-compliant calendars. On the other hand users might not notice that TZIDs have been specified that are being ignored because of the value type DATE

Copy link

codecov bot commented Feb 15, 2025

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
....Net/Serialization/DataTypes/DateTimeSerializer.cs 43% 2 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (43%) is below the target coverage (80%). You can increase the patch coverage or adjust the target coverage.
❌ Your project check has failed because the head coverage (66%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage.

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #734   +/-   ##
===================================
- Coverage    66%    66%   -0%     
===================================
  Files       103    103           
  Lines      4644   4650    +6     
  Branches   1152   1155    +3     
===================================
+ Hits       3064   3066    +2     
- Misses     1144   1146    +2     
- Partials    436    438    +2     
Files with missing lines Coverage Δ
....Net/Serialization/DataTypes/DateTimeSerializer.cs 79% <43%> (-6%) ⬇️

@axunonb
Copy link
Collaborator

axunonb commented Feb 16, 2025

First of all: These changes are useful and should get merged.

Deserialization of non-standard input:
All kind of "silent corrections", even when they make perfect sense, may cause users submitting issues.
Didn't think this till the end yet: Could a Validate or DeserializeWithValidation method, that returned ambiguities or simple errors to users be a way out? Or even a user callback containing issues as an argument, so they could decide how to deal with it? E.g. for "A TZID mustn't be specified for a date with the 'Z'", we submit the original value, and users can return the value they think is appropriate.
Very roughly expressed... Maybe something to work out in another task.

@minichma
Copy link
Collaborator Author

First of all: These changes are useful and should get merged.

Deserialization of non-standard input: All kind of "silent corrections", even when they make perfect sense, may cause users submitting issues. Didn't think this till the end yet: Could a Validate or DeserializeWithValidation method, that returned ambiguities or simple errors to users be a way out? Or even a user callback containing issues as an argument, so they could decide how to deal with it? E.g. for "A TZID mustn't be specified for a date with the 'Z'", we submit the original value, and users can return the value they think is appropriate. Very roughly expressed... Maybe something to work out in another task.

A Validate method would come with quite some overhead, because it would validate a string, not a Calendar instance. So if validation would be desired, the user would effectively have to deserialize twice, once implicitly when calling Validate and then for the actual deserialization. Allowing to parameterize the deserializer would probably be a good solution but today the architecture of the whole deserialization process is not that flexible. I don't know of an easy way to provide parameters. So maybe not something for v5. A static configuration like the TimeZoneProviders would be a workaround but in this case I think its really not the right thing to do. The TZ-DB to use is really something more global, but this kind of configuration is not suitable for global configuration.

So yes, I think, being more strict would probably be the right way to go but we must be aware that there will be people complaining.

Generally speaking, a desirable solution would always be to parse the user content as long as it is syntactically correct and only fail later on during an explicit Validation or during the Evaluation step. But this doesn't work with our new approach to CalDateTime, which doesn't allow illegal input. Would be good to have the objects used for evaluation (i.e. CalDateTime) and those representing the model in parallel. Refers to our discussion on CalDateTime in #705.

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.

2 participants