Crash when using `SMH` suffix with durations
ArnyminerZ opened this issue · 4 comments
We have FixInvalidDayOffsetPreprocessor in ical4android, which handles cases when PT is not properly formatted for triggers and durations, however, it doesn't contemplate the S suffix, as stated in the Java Documentation for "seconds", so for example:
BEGIN:VALARM
TRIGGER:-P5S
ACTION:DISPLAY
END:VALARM
crashes the app.
Internal DAVx5 reference: https://github.com/bitfireAT/davx5/issues/482
Ticket reference: https://bitfire-at.zammad.com/#ticket/zoom/3057
The issue here is that we are specifying seconds (S suffix), and as stated in the documentation:
The ASCII letter "T" must occur before the first occurrence, if any, of an hour, minute or second section.
So that is clearly missing the T, so the correct duration would be -PT5S, which if we try to parse it, doesn't give any issues:
Duration.parse("-PT5S")However, I've been taking a look at FixInvalidDayOffsetPreprocessor and it isn't exactly this case. So I don't know if we should extend the regex and functions, which would make them much more complicated, or just create a new StreamProcessor for cases like this.
This processor would have to detect cases where seconds, minutes or hours are specified without the T, and add it, so it would be a pretty simple one.
@sunkup what do you think? I believe that even though it's quite more code, it would be more readable and maintainable if we create a new StreamProcessor for this, since the cases handled by FixInvalidDayOffsetPreprocessor as stated in #77, even though they are related to the T prefix, it's not really the same case.
How often does this occur? Meaning how many providers/servers do this wrong? I am asking because, even if it seems like a small fix, we can not possibly repair all existing wrongdoings of all providers.
I only found one ticket in zammad mentioning one problematic server: "calcurse/baikal" (calcurse.org). I might be wrong, but I believe not too many people are using this server and it seems they have already fixed the time formatting issue? At least I can't find the file which the user in the ticket mentions: src/local.c in their repository.
I think we should consider adding this in case it pops up again with a different provider or if more calcurse users report it.
I've only seen that case, and I agree that we shouldn't cover all the formatting errors for all the server providers. So either we consider this case, or we can stale this until it happens again, or directly tell the user that the server must be updated or something
Addressed on server side here: lfos/calcurse#480
I wouldn't add another stream preprecessor for that, because it's a really rare problem and operating with regex on incoming iCalendars is really some "last resort" thing that I'd only do for problems that occur often and/or are very important.