Lenient parsing: ignore faulty fields?

[chibenwa]

Describe the bug

An invalid TRIGGER field cause the entire parsing to be aborted despite trials to configure parsing in a lenient way.

To Reproduce

The following code throws:

        CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_RELAXED_PARSING, true);
        CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_RELAXED_UNFOLDING, true);
        CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_RELAXED_VALIDATION, true);
        CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_OUTLOOK_COMPATIBILITY, true);
        CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_NOTES_COMPATIBILITY, true);
        CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_VCARD_COMPATIBILITY, true);

        new CalendarBuilder()
            .build(new ByteArrayInputStream(
                ("BEGIN:VCALENDAR\n" +
                    "METHOD:CANCEL\n" +
                    "BEGIN:VEVENT\n" +
                    "BEGIN:VALARM\n" +
                    "TRIGGER:P1800S\n" +
                    "END:VALARM\n" +
                    "END:VCALENDAR").getBytes(StandardCharsets.UTF_8)));

Here is the resulting exception :

net.fortuna.ical4j.data.ParserException: Error at line 5:Text cannot be parsed to a Duration
    at net.fortuna.ical4j.data.CalendarParserImpl.parse(CalendarParserImpl.java:158)
    at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:183)
    at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:171)
    at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:158)
[... junit stacktrace ]
Caused by: java.time.format.DateTimeParseException: Text cannot be parsed to a Duration
    at java.base/java.time.Duration.parse(Duration.java:417)
    at net.fortuna.ical4j.model.TemporalAmountAdapter.parse(TemporalAmountAdapter.java:125)
    at net.fortuna.ical4j.model.property.Trigger.setValue(Trigger.java:270)
    at net.fortuna.ical4j.model.property.Trigger.<init>(Trigger.java:165)
    at net.fortuna.ical4j.model.property.Trigger$Factory.createProperty(Trigger.java:315)
    at net.fortuna.ical4j.model.PropertyBuilder.build(PropertyBuilder.java:48)
    at net.fortuna.ical4j.data.DefaultContentHandler.endProperty(DefaultContentHandler.java:123)
    at net.fortuna.ical4j.data.CalendarParserImpl$PropertyParser.parse(CalendarParserImpl.java:292)
    at net.fortuna.ical4j.data.CalendarParserImpl$PropertyParser.access$1100(CalendarParserImpl.java:224)
    at net.fortuna.ical4j.data.CalendarParserImpl$PropertyListParser.parse(CalendarParserImpl.java:211)
    at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.parse(CalendarParserImpl.java:427)
    at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.access$900(CalendarParserImpl.java:404)
    at net.fortuna.ical4j.data.CalendarParserImpl$PropertyListParser.parse(CalendarParserImpl.java:209)
    at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.parse(CalendarParserImpl.java:427)
    at net.fortuna.ical4j.data.CalendarParserImpl$ComponentParser.access$900(CalendarParserImpl.java:404)
    at net.fortuna.ical4j.data.CalendarParserImpl$PropertyListParser.parse(CalendarParserImpl.java:209)
    at net.fortuna.ical4j.data.CalendarParserImpl.parseCalendar(CalendarParserImpl.java:115)
    at net.fortuna.ical4j.data.CalendarParserImpl.parseCalendarList(CalendarParserImpl.java:180)
    at net.fortuna.ical4j.data.CalendarParserImpl.parse(CalendarParserImpl.java:149)
    ... 67 more

Expected behavior

I would expect some lenient parsing allowing to ignore the mal-formatted TRIGGER but would still allow me to parse the rest of the file.

Environment (please complete the following information):

• ical4j versions: 3.0.9 -> 3.0.19. I also reproduced it on top of 4.0.0-alpha6.
• OS: Linux
• Java Version: 11

Additional context

We are using ICAL4J as part of the Apache James project to provide a connector for calendar application:

The Icalendar / VCards attached to mails are extractes and sent to calendar applications to display the given events in the user calendar.

The following parsing bugs prevents us to display slightly invalid VCARDs sent by buggy third party applications.

We of course would love to have a way to not loose such third party events.

Note that the correct version of the exemple is:

        new CalendarBuilder()
            .build(new ByteArrayInputStream(
                ("BEGIN:VCALENDAR\n" +
                    "METHOD:CANCEL\n" +
                    "BEGIN:VEVENT\n" +
                    "BEGIN:VALARM\n" +
                    "TRIGGER:PT1800S\n" +
                    "END:VALARM\n" +
                    "END:VCALENDAR").getBytes(StandardCharsets.UTF_8)));

[rfc2822]

Is it possible that properties which don't break the whole iCalendar syntax but just have invalid values (for instance: GEO:0) can just be ignored instead of stopping the whole iCalendar processing? I imagine that when the Factory for that property throws an Exception that it could be catched in relaxed mode, but then parsing continues or something like that.

[benfortuna]

So I've just added an option to the ContextHandlerContext to support suppression of invalid properties:

https://github.com/ical4j/ical4j/blob/develop/src/main/java/net/fortuna/ical4j/data/DefaultContentHandler.java#L163

When enabled it should catch any checked exceptions and ignore the invalid property. I think this should work for both the original Trigger issue and the Geo. (apologies for delay on this issue)

[chibenwa]

context.isSupressInvalidProperties is not available on 3.2.8 (I just tried...)

• 1. Could you please confirm that the change you mention would be available for 4.0.0
• 2. Could you give an ETA for 4.0.0 ?

[benfortuna]

I only just committed the change, but it will be in the next release of both 3.x and 4.x versions.

I try to release at least once a month, but if you want to try a snapshot build they should be available immediately.

[ArnyminerZ]

I have been trying the new property on ical4j 3.2.9. Trying to parse the following calendar:

BEGIN:VCALENDAR
BEGIN:VEVENT
LAST-MODIFIED:20230108T011226
DTSTAMP:20230108T011226Z
DTSTART:20230101T015100Z
DTEND:20230101T020600Z
SUMMARY:This is a test event
DESCRIPTION:Example description
UID:63b0e389453c5d000e1161ae
GEO:0
END:VEVENT
END:VCALENDAR

With this pseudocode:

calendar = CalendarBuilder(
    CalendarParserFactory.getInstance().get(),
    ContentHandlerContext().withSupressInvalidProperties(true),
    TimeZoneRegistryFactory.getInstance().createRegistry(),
).build(reader)

This is thrown:

...
Caused by: net.fortuna.ical4j.data.ParserException: Error at line 10:Invalid GEO string
at net.fortuna.ical4j.data.CalendarParserImpl.parse(CalendarParserImpl.java:172)
at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:197)
at net.fortuna.ical4j.data.CalendarBuilder.build(CalendarBuilder.java:185)
at at.bitfire.ical4android.ICalendar$Companion.fromReader(ICalendar.kt:91)
... 29 more
Caused by: java.lang.IllegalArgumentException: Invalid GEO string
...

I have seen that the fix doesn't cover ParserException but ParseException:

https://github.com/ical4j/ical4j/blob/b0257526796e83022708deb3eef56189632da5bf/src/main/java/net/fortuna/ical4j/data/DefaultContentHandler.java#L162-L169

I don't know if this is intentional or a typo.

[rfc2822]

I can confirm that invalid GEO strings still don't work and cause the while iCalendar to be ignored (with v3.2.9).

[benfortuna]

Apologies I couldn't look into this sooner. Just looking at the code I think the issue is we are only catching checked exceptions in the change to suppress invalid properties.

For the case of invalid GEO it is throwing an IllegalArgumentException (unchecked), which is why it isn't working.

I'll review all the unchecked exceptions and provide a fix shortly.

[benfortuna]

Fix included in 3.2.10 (I don't think v4.x is affected by this bug)

[bubbatls]

Hello, it is still happening in the 3.2.12 with bad value like TRIGGER:-P2DT

here is the stack:

Caused by: java.time.format.DateTimeParseException: Text cannot be parsed to a Duration
    at java.time.Duration.parse(Duration.java:419) ~[?:?]
    at net.fortuna.ical4j.model.TemporalAmountAdapter.parse(TemporalAmountAdapter.java:146) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.model.TemporalAmountAdapter.parse(TemporalAmountAdapter.java:133) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.model.property.Trigger.setValue(Trigger.java:253) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.model.property.Trigger.<init>(Trigger.java:166) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.model.property.Trigger$Factory.createProperty(Trigger.java:301) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.model.property.Trigger$Factory.createProperty(Trigger.java:291) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.model.PropertyBuilder.build(PropertyBuilder.java:76) ~[ical4j-3.2.12.jar:3.2.12]
    at net.fortuna.ical4j.data.DefaultContentHandler.endProperty(DefaultContentHandler.java:161) ~[ical4j-3.2.12.jar:3.2.12]

because is not catch by } catch (URISyntaxException | ParseException | IOException | IllegalArgumentException e) {

I think that TemporalAmountAdapter.parse should wrap this DateTimeParseException in a java.text.ParseException