Skip to content

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

@sonarqubecloud
Copy link

@codecov
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%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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.

@axunonb
Copy link
Collaborator

axunonb commented Jun 9, 2025

Maybe implement sth. like this draft? The serializer just throws for semantically incorrect input.

using System;
using Ical.Net.DataTypes;

namespace Ical.Net.Serialization.Models;

/// <summary>
/// The model for a date-only or date-time values in iCalendar.
/// When created from the parser, it may contain ambiguities.
/// </summary>
internal struct DateTimeModel
{
    public DateOnly Date;
    public TimeOnly? Time;
    public bool HasTime => Time is not null;
    public bool IsUtc; // Indicates if the time is in UTC (Z suffix)
    public string? TzId;
    public bool HasTzId => !string.IsNullOrEmpty(TzId);
}

internal static class DateTimeModelEvaluator
{
    public static bool ThrowForRfcViolations { get; set; }

    public static CalDateTime ToCalDateTime(DateTimeModel model)
    {
        if (!Validate(model) && ThrowForRfcViolations)
        {
            // or rather a more specific exception type
            throw new InvalidOperationException("Invalid DateTimeModel")
            {
                Data =
                {
                    ["Model"] = model
                }
            };
        }

        if (model.HasTime)
        {
            return new CalDateTime(model.Date, model.Time, ResolveTzId(model));
        }

        return new CalDateTime(model.Date);
    }

    private static string? ResolveTzId(DateTimeModel modelWithTime)
    {
        // do our best to resolve ambiguities
        return modelWithTime switch
        {
            { IsUtc: true } => CalDateTime.UtcTzId,
            _ => modelWithTime.TzId
        };
    }

    private static bool Validate(DateTimeModel model)
    {
        return model switch
        {
            { IsUtc: true, HasTzId: true } => false,
            { HasTime: false, HasTzId: true } => false,
            _ => true
        };
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants