FixInvalidDayOffsetPreprocessor: Allow DURATION as value

[sunkup]

Allow DURATION as value in FixInvalidDayOffsetPreprocessor so that it will be found and the actual duration value can be fixed if invalid.

Reported in ICSx5: bitfireAT/icsx5#462

[rfc2822]

I assume this is based on a real issue? Which one?

Can we require a property (or a list of properties which may have a DURATION) or at least ;VALUE=?

This pre-processor is a very risky and off-the-wall way to fix things and should be used in the most minimal way. For instance, I always fear that some plaintext may contain exactly this content and then be modified …

[ArnyminerZ]

Everything looks good, makes sense. I didn't know that there was such a property. Should we somehow use it at some point? I mean, we can only fetch new data if necessary knowing that it only updates once a day, for example.

[sunkup]

I assume this is based on a real issue? Which one?

This one: bitfireAT/icsx5#462
Added to description.

This pre-processor is a very risky and off-the-wall way to fix things and should be used in the most minimal way. For instance, I always fear that some plaintext may contain exactly this content and then be modified …

Yes, that's why I tried to relax the regex as little as possible.

Can we require a property (or a list of properties which may have a DURATION) or at least ;VALUE=?

I will try to narrow it down some more in a sensible way.

[sunkup]

Updated.

I believe DURATION can only occur as value of the following properties.

RFC 5545

• DURATION
• RELATED-TO
• TRIGGER

RFC 7986

• REFRESH-INTERVAL

Since TRIGGER never occurs as a value for any of the properties (like DURATION does), it's a bit weird to have it in the regex as well, but I think separating it might make the regex unnecessarily complex.

[rfc2822]

Working fine.

I have suggestions for method names in tests.

[rfc2822]

I have changed the test so that it asserts what is expected (and not what is not expected, which was confusing for me).

And one question :)

[rfc2822]

Sorry for making this complicated… but I think it's worth to fully understand the details of this regex and class and what they do, otherwise it could make unexpected problems.

[sunkup]

Sorry for making this complicated… but I think it's worth to fully understand the details of this regex and class and what they do, otherwise it could make unexpected problems.

No worries. I don't like working with regex, but you are totally right. We should not slip up here. I did not fully understand how the non-capturing groups work. I expected them to be excluded from the whole match, but alas they are not and we need to use the list of capturing groups like you say.

I changed the regex to capture only the duration in a capturing group, while ignoring the begining of the string, so problems as with the "CAPTION" scenario should not arise:

"(?:^|^(?:DURATION|REFRESH-INTERVAL|RELATED-TO|TRIGGER);VALUE=)(?:DURATION|TRIGGER):(-?P((T-?\\d+D)|(-?\\d+DT)))$"

I also tried to make the code as explicit as possible with many comments. The first capturing group always contains the whole match. The second match group (match.groupValues[1]) now only contains the actual faulty duration, ie "-PT1D", which is fixed first. Then the whole fixed duration string is replacing the faulty duration string in the whole match string and in the end the whole match string is found and replaced in the original ical.

In the best case we would also add another edge case test for the "CAPTION" scenario. If we are worried about unexpected issues, more tests clamping down on the expected behaviour are always nice to have. I don't see how we can do that though, since "CAPTION" is not a relevant property here as it can not have a duration value.

The alternative would be to drop all the non-capturing groups and simply write a comment about replacing "PT" with "T" in the whole capture.

I also just noticed that match has a match.range and we can directly use it to call replace: s.replace(match.range, fixed). If I understand it correctly, this should replace the actually found occurrence and not run a new search.

AFAIK replace() does not take an int range, so I think we would need to use replaceRange() for that. I don't think we need to use the match range at all though.

[rfc2822]

To illustrate what I mean: I have now used replaceRange so that we exactly replace the match, so we don't have to find multiple times (with potential different results).

Tests are OK; please have a final look and then I'll merge if it's OK for you.

[sunkup]

To illustrate what I mean: I have now used replaceRange so that we exactly replace the match, so we don't have to find multiple times (with potential different results).

Tests are OK; please have a final look and then I'll merge if it's OK for you.

Oh, so the match group has a range as well ... Yeah, no. I did not get that from your description. But looks good to me like this. Much shorter 👍