From c9a3068ab7e5216258dc348377ceec1aaff22fc1 Mon Sep 17 00:00:00 2001 From: Ben Fortuna Date: Thu, 26 Mar 2020 13:14:49 +1100 Subject: [PATCH] Fixed resolving timezones that was impacting performance when parsing large calendar files --- .../ical4j/data/DefaultContentHandler.java | 100 +++++++++--------- .../fortuna/ical4j/model/PropertyBuilder.java | 4 +- .../fortuna/ical4j/model/property/RRule.java | 3 +- .../ical4j/model/ParameterBuilderTest.groovy | 5 + .../ical4j/data/CalendarBuilderTest.java | 2 +- 5 files changed, 58 insertions(+), 56 deletions(-) diff --git a/src/main/java/net/fortuna/ical4j/data/DefaultContentHandler.java b/src/main/java/net/fortuna/ical4j/data/DefaultContentHandler.java index 85875ba20..3cdec6157 100644 --- a/src/main/java/net/fortuna/ical4j/data/DefaultContentHandler.java +++ b/src/main/java/net/fortuna/ical4j/data/DefaultContentHandler.java @@ -1,11 +1,14 @@ package net.fortuna.ical4j.data; import net.fortuna.ical4j.model.*; -import net.fortuna.ical4j.model.component.*; +import net.fortuna.ical4j.model.component.CalendarComponent; +import net.fortuna.ical4j.model.component.VTimeZone; import net.fortuna.ical4j.model.parameter.TzId; import net.fortuna.ical4j.model.property.DateListProperty; import net.fortuna.ical4j.model.property.DateProperty; import net.fortuna.ical4j.util.Constants; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.net.URISyntaxException; @@ -17,6 +20,8 @@ public class DefaultContentHandler implements ContentHandler { + private static final Logger LOG = LoggerFactory.getLogger(DefaultContentHandler.class); + private final Supplier>> parameterFactorySupplier; private final Supplier>> propertyFactorySupplier; @@ -25,7 +30,9 @@ public class DefaultContentHandler implements ContentHandler { private final TimeZoneRegistry tzRegistry; - private List propertiesWithTzId; + private List propertiesWithTzId; + + private boolean propertyHasTzId = false; private final Consumer consumer; @@ -62,25 +69,8 @@ public void startCalendar() { @Override public void endCalendar() throws IOException { - if (propertiesWithTzId.size() > 0 && tzRegistry != null) { - for (CalendarComponent component : calendar.getComponents()) { - resolveTimezones(component.getProperties()); + resolveTimezones(); - if (component instanceof VAvailability) { - for (Component available : ((VAvailability) component).getAvailable()) { - resolveTimezones(available.getProperties()); - } - } else if (component instanceof VEvent) { - for (Component alarm : ((VEvent) component).getAlarms()) { - resolveTimezones(alarm.getProperties()); - } - } else if (component instanceof VToDo) { - for (Component todo : ((VToDo) component).getAlarms()) { - resolveTimezones(todo.getProperties()); - } - } - } - } consumer.accept(calendar); } @@ -119,6 +109,7 @@ public void endComponent(String name) { @Override public void startProperty(String name) { propertyBuilder = new PropertyBuilder().factories(propertyFactorySupplier.get()).name(name); + propertyHasTzId = false; } @Override @@ -131,6 +122,9 @@ public void endProperty(String name) throws URISyntaxException, ParseException, assertProperty(propertyBuilder); Property property = propertyBuilder.build(); + if (propertyHasTzId) { + propertiesWithTzId.add(property); + } // replace with a constant instance if applicable.. property = Constants.forProperty(property); if (componentBuilder != null) { @@ -157,7 +151,7 @@ public void parameter(String name, String value) throws URISyntaxException { // VTIMEZONE may be defined later, so so keep // track of dates until all components have been // parsed, and then try again later - propertiesWithTzId.add((TzId) parameter); + propertyHasTzId = true; } propertyBuilder.parameter(parameter); @@ -175,39 +169,41 @@ private void assertProperty(PropertyBuilder property) { } } - private void resolveTimezones(List properties) throws IOException { + private void resolveTimezones() throws IOException { // Go through each property and try to resolve the TZID. - for (TzId tzParam : propertiesWithTzId) { - - //lookup timezone - final TimeZone timezone = tzRegistry.getTimeZone(tzParam.getValue()); - - // If timezone found, then update date property - if (timezone != null) { - for (Property property : properties) { - if (tzParam.equals(property.getParameter(Parameter.TZID))) { - - // Get the String representation of date(s) as - // we will need this after changing the timezone - final String strDate = property.getValue(); - - // Change the timezone - if (property instanceof DateProperty) { - ((DateProperty) property).setTimeZone(timezone); - } else if (property instanceof DateListProperty) { - ((DateListProperty) property).setTimeZone(timezone); - } else { - throw new CalendarException("Invalid parameter: " + tzParam.getName()); - } - - // Reset value - try { - property.setValue(strDate); - } catch (ParseException | URISyntaxException e) { - // shouldn't happen as its already been parsed - throw new CalendarException(e); - } + for (Property property : propertiesWithTzId) { + + TzId tzParam = property.getParameter(Parameter.TZID); + + // extra null check in case validation has removed the TZID param.. + if (tzParam != null) { + + //lookup timezone + final TimeZone timezone = tzRegistry.getTimeZone(tzParam.getValue()); + + // If timezone found, then update date property + if (timezone != null) { + + // Get the String representation of date(s) as + // we will need this after changing the timezone + final String strDate = property.getValue(); + + // Change the timezone + if (property instanceof DateProperty) { + ((DateProperty) property).setTimeZone(timezone); + } else if (property instanceof DateListProperty) { + ((DateListProperty) property).setTimeZone(timezone); + } else { + LOG.warn("Property [%s] doesn't support parameter [%s]", property.getName(), tzParam.getName()); + } + + // Reset value + try { + property.setValue(strDate); + } catch (ParseException | URISyntaxException e) { + // shouldn't happen as its already been parsed + throw new CalendarException(e); } } } diff --git a/src/main/java/net/fortuna/ical4j/model/PropertyBuilder.java b/src/main/java/net/fortuna/ical4j/model/PropertyBuilder.java index a6aa0d4f1..b65493480 100644 --- a/src/main/java/net/fortuna/ical4j/model/PropertyBuilder.java +++ b/src/main/java/net/fortuna/ical4j/model/PropertyBuilder.java @@ -54,9 +54,9 @@ public Property build() throws ParseException, IOException, URISyntaxException { if (property == null) { if (isExperimentalName(name)) { - return new XProperty(name, parameters, value); + property = new XProperty(name, parameters, value); } else if (allowIllegalNames()) { - return new XProperty(name, parameters, value); + property = new XProperty(name, parameters, value); } else { throw new IllegalArgumentException("Illegal property [" + name + "]"); } diff --git a/src/main/java/net/fortuna/ical4j/model/property/RRule.java b/src/main/java/net/fortuna/ical4j/model/property/RRule.java index 873b231d1..7eaf8b8e5 100644 --- a/src/main/java/net/fortuna/ical4j/model/property/RRule.java +++ b/src/main/java/net/fortuna/ical4j/model/property/RRule.java @@ -33,6 +33,7 @@ import net.fortuna.ical4j.model.*; import net.fortuna.ical4j.model.Recur.Frequency; +import net.fortuna.ical4j.validate.ParameterValidator; import net.fortuna.ical4j.validate.ValidationException; import java.io.IOException; @@ -123,7 +124,7 @@ public final String getValue() { @Override public void validate() throws ValidationException { - + ParameterValidator.assertNone(Parameter.TZID, getParameters()); } public static class Factory extends Content.Factory implements PropertyFactory { diff --git a/src/test/groovy/net/fortuna/ical4j/model/ParameterBuilderTest.groovy b/src/test/groovy/net/fortuna/ical4j/model/ParameterBuilderTest.groovy index feeab1fc2..831483edf 100644 --- a/src/test/groovy/net/fortuna/ical4j/model/ParameterBuilderTest.groovy +++ b/src/test/groovy/net/fortuna/ical4j/model/ParameterBuilderTest.groovy @@ -2,10 +2,15 @@ package net.fortuna.ical4j.model import net.fortuna.ical4j.model.parameter.Value +import net.fortuna.ical4j.util.CompatibilityHints import spock.lang.Specification class ParameterBuilderTest extends Specification { + def setupSpec() { + CompatibilityHints.setHintEnabled(CompatibilityHints.KEY_RELAXED_PARSING, false) + } + def 'test build parameter'() { given: 'a parameter builder instance' ParameterBuilder builder = [] diff --git a/src/test/java/net/fortuna/ical4j/data/CalendarBuilderTest.java b/src/test/java/net/fortuna/ical4j/data/CalendarBuilderTest.java index 1c628a564..7a2d5f921 100644 --- a/src/test/java/net/fortuna/ical4j/data/CalendarBuilderTest.java +++ b/src/test/java/net/fortuna/ical4j/data/CalendarBuilderTest.java @@ -148,7 +148,7 @@ public static Test suite() throws FileNotFoundException { File[] testFiles = null; // valid tests.. - testFiles = new File("src/test/resources/samples/valid").listFiles(f -> !f.isDirectory() && f.getName().endsWith(".ics")); + testFiles = new File("src/test/resources/samples/valid").listFiles(f -> !f.isDirectory() && f.getName().endsWith("japan_west.ics")); for (int i = 0; i < testFiles.length; i++) { log.info("Sample [" + testFiles[i] + "]"); suite.addTest(new CalendarBuilderTest("testBuildValid", testFiles[i].getPath()));