From e8e7ec74106f67d50555ff54ab44b94dcb1642b0 Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Tue, 2 Jul 2024 18:24:31 +0200 Subject: [PATCH 1/2] #4921 - Constraints do not work properly when rules are distributed over multiple rulesets - Fixed merging - Added test --- .../constraints/ConstraintsServiceImpl.java | 89 ++++++++++--------- .../constraints/model/ParsedConstraints.java | 45 ++++++---- .../webanno/constraints/model/Scope.java | 8 +- .../ConstraintsServiceImplTest.java | 73 +++++++++++++++ 4 files changed, 154 insertions(+), 61 deletions(-) create mode 100644 inception/inception-constraints/src/test/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImplTest.java diff --git a/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImpl.java b/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImpl.java index 7d4c04420a5..895040ef4bb 100644 --- a/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImpl.java +++ b/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImpl.java @@ -28,6 +28,8 @@ import java.io.InputStream; import java.io.StringReader; import java.lang.invoke.MethodHandles; +import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; import java.util.Objects; @@ -52,6 +54,7 @@ import de.tudarmstadt.ukp.clarin.webanno.constraints.grammar.ConstraintsParser; import de.tudarmstadt.ukp.clarin.webanno.constraints.grammar.ParseException; import de.tudarmstadt.ukp.clarin.webanno.constraints.model.ParsedConstraints; +import de.tudarmstadt.ukp.clarin.webanno.constraints.model.Rule; import de.tudarmstadt.ukp.clarin.webanno.model.ConstraintSet; import de.tudarmstadt.ukp.clarin.webanno.model.Project; import de.tudarmstadt.ukp.inception.documents.api.RepositoryProperties; @@ -241,52 +244,56 @@ public ParsedConstraints getMergedConstraints(Project aProject) private ParsedConstraints loadConstraints(Project aProject) throws IOException, ParseException { try (var logCtx = withProjectLogger(aProject)) { - ParsedConstraints merged = null; - + var sets = new ArrayList(); for (var set : listConstraintSets(aProject)) { - var script = readConstrainSet(set); - var parser = new ConstraintsParser(new StringReader(script)); - var astConstraintsSet = parser.constraintsSet(); - var constraints = new ParsedConstraints(astConstraintsSet); + sets.add(loadConstraints(set)); + } - if (merged == null) { - merged = constraints; - } - else { - // Merge imports - for (var e : constraints.getImports().entrySet()) { - // Check if the value already points to some other feature in previous - // constraint file(s). - if (merged.getImports().containsKey(e.getKey()) && !e.getValue() - .equalsIgnoreCase(merged.getImports().get(e.getKey()))) { - // If detected, notify user with proper message and abort merging - var errorMessage = "Conflict detected in imports for key \"" - + e.getKey() + "\", conflicting values are \"" + e.getValue() - + "\" & \"" + merged.getImports().get(e.getKey()) - + "\". Please contact Project Admin for correcting this." - + "Constraints feature may not work." - + "\nAborting Constraint rules merge!"; - throw new ParseException(errorMessage); - } - } - merged.getImports().putAll(constraints.getImports()); - - // Merge scopes - for (var scope : constraints.getScopes()) { - var target = merged.getScopeByName(scope.getScopeName()); - if (target == null) { - // Scope does not exist yet - merged.getScopes().add(scope); - } - else { - // Scope already exists - target.getRules().addAll(scope.getRules()); - } - } + return mergeConstraintSets(sets); + } + } + + static ParsedConstraints mergeConstraintSets(List sets) throws ParseException + { + var allImports = new LinkedHashMap(); + var allScopes = new LinkedHashMap>(); + + for (var constraints : sets) { + + // Merge imports + for (var e : constraints.getImports().entrySet()) { + // Check if the value already points to some other feature in previous + // constraint file(s). + if (allImports.containsKey(e.getKey()) + && !e.getValue().equalsIgnoreCase(allImports.get(e.getKey()))) { + // If detected, notify user with proper message and abort merging + var errorMessage = "Conflict detected in imports for key \"" + e.getKey() + + "\", conflicting values are \"" + e.getValue() + "\" & \"" + + allImports.get(e.getKey()) + + "\". Please contact a project manager to correct this." + + "Constraint may not work." + "\nAborting Constraint rules merge!"; + throw new ParseException(errorMessage); } } + allImports.putAll(constraints.getImports()); - return merged; + // Merge scopes + for (var scope : constraints.getScopes()) { + var target = allScopes.computeIfAbsent(scope.getScopeName(), + $ -> new ArrayList()); + target.addAll(scope.getRules()); + } } + + return new ParsedConstraints(allImports, allScopes); + } + + private ParsedConstraints loadConstraints(ConstraintSet set) throws IOException, ParseException + { + var script = readConstrainSet(set); + var parser = new ConstraintsParser(new StringReader(script)); + var astConstraintsSet = parser.constraintsSet(); + var constraints = new ParsedConstraints(astConstraintsSet); + return constraints; } } diff --git a/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/ParsedConstraints.java b/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/ParsedConstraints.java index 98616a79516..07b2f0e53fb 100644 --- a/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/ParsedConstraints.java +++ b/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/ParsedConstraints.java @@ -24,13 +24,14 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; import de.tudarmstadt.ukp.clarin.webanno.constraints.grammar.ASTConstraintsSet; -/*** +/** * Serialized Class containing objects after parsing and creating objects based on rules file. */ public class ParsedConstraints @@ -38,24 +39,36 @@ public class ParsedConstraints { private static final long serialVersionUID = -2401965871743170805L; - private final Map imports; - private final List scopes; + private final Map imports = new LinkedHashMap<>(); + private final List scopes = new ArrayList<>(); private final Map scopeMap; // Contains possible scenarios for which rules are available. private final Set rulesSet; + public ParsedConstraints(Map aAliases, Map> aScopes) + { + imports.putAll(aAliases); + + for (var e : aScopes.entrySet()) { + var scope = new Scope(e.getKey(), e.getValue()); + scopes.add(scope); + } + + rulesSet = buildRulesSet(); + scopeMap = buildScopeMap(); + } + public ParsedConstraints(Map aAliases, List aScopes) { - imports = aAliases; - scopes = aScopes; + imports.putAll(aAliases); + scopes.addAll(aScopes); rulesSet = buildRulesSet(); scopeMap = buildScopeMap(); } public ParsedConstraints(ASTConstraintsSet astConstraintsSet) { - imports = astConstraintsSet.getImports(); - scopes = new ArrayList<>(); + imports.putAll(astConstraintsSet.getImports()); for (var ruleGroup : astConstraintsSet.getScopes().entrySet()) { var rules = new ArrayList(); @@ -105,16 +118,16 @@ public boolean areThereRules(String aFS, String aFeature) buildRulesSet(); } - if (getShortName(aFS) == null) { + var shortName = getShortName(aFS); + if (shortName == null) { return false; } - if (getScopeByName(getShortName(aFS)) == null) { + if (getScopeByName(shortName) == null) { return false; } - var _tempFsfPair = new FSFPair(getShortName(aFS), aFeature); - if (rulesSet.contains(_tempFsfPair)) { + if (rulesSet.contains(new FSFPair(shortName, aFeature))) { // If it has rules satisfying with proper input FS and affecting feature return true; } @@ -125,7 +138,7 @@ public boolean areThereRules(String aFS, String aFeature) private Map buildScopeMap() { var scopeMap = new HashMap(); - for (Scope scope : scopes) { + for (var scope : scopes) { scopeMap.put(scope.getScopeName(), scope); } return unmodifiableMap(scopeMap); @@ -136,19 +149,19 @@ private Map buildScopeMap() */ private Set buildRulesSet() { - var rulesSet = new HashSet(); + var rs = new HashSet(); FSFPair _temp; for (var scope : scopes) { for (var rule : scope.getRules()) { for (var restriction : rule.getRestrictions()) { _temp = new FSFPair(scope.getScopeName(), restriction.getPath()); - if (!rulesSet.contains(_temp)) { - rulesSet.add(_temp); + if (!rs.contains(_temp)) { + rs.add(_temp); } } } } - return unmodifiableSet(rulesSet); + return unmodifiableSet(rs); } private String printImports() diff --git a/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/Scope.java b/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/Scope.java index 1d05bf8ea87..c8ee316e1c2 100644 --- a/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/Scope.java +++ b/inception/inception-constraints/src/main/java/de/tudarmstadt/ukp/clarin/webanno/constraints/model/Scope.java @@ -27,13 +27,14 @@ public class Scope implements Serializable { private static final long serialVersionUID = 226908916809455385L; + private final String scopeName; private final List rules; - public Scope(String scopeName, List rules) + public Scope(String aScopeName, List aRules) { - this.scopeName = scopeName; - this.rules = rules; + scopeName = aScopeName; + rules = aRules; } public String getScopeName() @@ -51,5 +52,4 @@ public String toString() { return "Scope [" + scopeName + "]\nRules\n" + rules.toString() + "]"; } - } diff --git a/inception/inception-constraints/src/test/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImplTest.java b/inception/inception-constraints/src/test/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImplTest.java new file mode 100644 index 00000000000..4b0b2521f13 --- /dev/null +++ b/inception/inception-constraints/src/test/java/de/tudarmstadt/ukp/clarin/webanno/constraints/ConstraintsServiceImplTest.java @@ -0,0 +1,73 @@ +/* + * Licensed to the Technische Universität Darmstadt under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The Technische Universität Darmstadt + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package de.tudarmstadt.ukp.clarin.webanno.constraints; + +import static de.tudarmstadt.ukp.clarin.webanno.constraints.ConstraintsServiceImpl.mergeConstraintSets; +import static java.util.Arrays.asList; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import de.tudarmstadt.ukp.clarin.webanno.constraints.model.Condition; +import de.tudarmstadt.ukp.clarin.webanno.constraints.model.ParsedConstraints; +import de.tudarmstadt.ukp.clarin.webanno.constraints.model.Restriction; +import de.tudarmstadt.ukp.clarin.webanno.constraints.model.Rule; +import de.tudarmstadt.ukp.clarin.webanno.constraints.model.Scope; + +class ConstraintsServiceImplTest +{ + @Test + void testMergeConstraintSets() throws Exception + { + var rule1 = new Rule(new Condition("foo", "lala"), new Restriction("fee", "lili")); + var set1 = new ParsedConstraints(Map.of("Foo", "some.Foo"), + asList(new Scope("Foo", asList(rule1)))); + + var rule2 = new Rule(new Condition("bar", "lolo"), new Restriction("faa", "lulu")); + var set2 = new ParsedConstraints(Map.of("Bar", "some.Bar"), + asList(new Scope("Bar", asList(rule2)))); + + var merged = mergeConstraintSets(asList(set1, set2)); + + assertThat(merged.getImports()) // + .hasSize(2) // + .containsEntry("Foo", "some.Foo") // + .containsEntry("Bar", "some.Bar"); + + assertThat(merged.getShortName("some.Foo")).isEqualTo("Foo"); + assertThat(merged.getShortName("some.Bar")).isEqualTo("Bar"); + + assertThat(merged.getScopes()) // + .extracting(Scope::getScopeName) // + .containsExactlyInAnyOrder("Foo", "Bar"); + + assertThat(merged.getScopeByName("Foo")) // + .extracting(Scope::getScopeName) // + .isEqualTo("Foo"); + assertThat(merged.getScopeByName("Foo").getRules()) // + .containsExactly(rule1); + + assertThat(merged.getScopeByName("Bar")) // + .extracting(Scope::getScopeName) // + .isEqualTo("Bar"); + assertThat(merged.getScopeByName("Bar").getRules()) // + .containsExactly(rule2); + } +} From 3aa7a2374f39da8186fa9a3f8dae240f0334d223 Mon Sep 17 00:00:00 2001 From: Richard Eckart de Castilho Date: Tue, 2 Jul 2024 17:33:50 +0200 Subject: [PATCH 2/2] #4919 - Unable to import project with knowledge-based exported by INCEpTION 33.0 (again) - Fixed issue and added test --- .../webanno/api/export/ProjectExporter.java | 24 ++-- .../api/export/ProjectExporterTest.java | 116 ++++++++++++++++++ 2 files changed, 132 insertions(+), 8 deletions(-) create mode 100644 inception/inception-export-api/src/test/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporterTest.java diff --git a/inception/inception-export-api/src/main/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporter.java b/inception/inception-export-api/src/main/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporter.java index 64dea6f1a87..a885cf2b0e4 100644 --- a/inception/inception-export-api/src/main/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporter.java +++ b/inception/inception-export-api/src/main/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporter.java @@ -18,6 +18,7 @@ package de.tudarmstadt.ukp.clarin.webanno.api.export; import static org.apache.commons.io.FilenameUtils.normalize; +import static org.apache.commons.lang3.StringUtils.isEmpty; import static org.apache.commons.lang3.StringUtils.removeStart; import java.io.IOException; @@ -56,23 +57,30 @@ void importData(ProjectImportRequest aRequest, Project aProject, ExportedProject static String normalizeEntryName(ZipEntry aEntry) { - // Strip leading "/" that we had in ZIP files prior to 2.0.8 (bug #985) - var entryName = aEntry.toString(); - if (entryName.startsWith("/")) { - entryName = entryName.substring(1); + var name = removeStart(normalize(aEntry.getName(), true), "/"); + + if (isEmpty(name)) { + return null; } - return entryName; + return name; } static ZipEntry getEntry(ZipFile aFile, String aEntryName) { - var entryName = new ZipEntry(removeStart(normalize(aEntryName, true), "/")); - var entry = aFile.getEntry(aEntryName); + var normalizedEntryName = normalize(aEntryName, true); + + var entry = aFile.getEntry(normalizedEntryName); if (entry != null) { return entry; } - return aFile.getEntry("/" + entryName); + + if (normalizedEntryName.startsWith("/")) { + return aFile.getEntry(removeStart(normalizedEntryName, "/")); + } + else { + return aFile.getEntry("/" + normalizedEntryName); + } } static void writeEntry(ZipOutputStream aStage, String aEntryName, diff --git a/inception/inception-export-api/src/test/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporterTest.java b/inception/inception-export-api/src/test/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporterTest.java new file mode 100644 index 00000000000..e0052c03d1b --- /dev/null +++ b/inception/inception-export-api/src/test/java/de/tudarmstadt/ukp/clarin/webanno/api/export/ProjectExporterTest.java @@ -0,0 +1,116 @@ +/* + * Licensed to the Technische Universität Darmstadt under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The Technische Universität Darmstadt + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package de.tudarmstadt.ukp.clarin.webanno.api.export; + +import static de.tudarmstadt.ukp.clarin.webanno.api.export.ProjectExporter.getEntry; +import static de.tudarmstadt.ukp.clarin.webanno.api.export.ProjectExporter.normalizeEntryName; +import static de.tudarmstadt.ukp.clarin.webanno.api.export.ProjectExporter.writeEntry; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.FileOutputStream; +import java.nio.file.Path; +import java.util.zip.ZipEntry; +import java.util.zip.ZipFile; +import java.util.zip.ZipOutputStream; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class ProjectExporterTest +{ + + @Test + void testGetEntry(@TempDir Path tempDir) throws Exception + { + var zipFile = tempDir.resolve("test.zip").toFile(); + + try (var fos = new FileOutputStream(zipFile); var zos = new ZipOutputStream(fos)) { + zos.putNextEntry(new ZipEntry("/foo.txt")); + zos.write("foo".getBytes(UTF_8)); + zos.closeEntry(); + + zos.putNextEntry(new ZipEntry("bar.txt")); + zos.write("bar".getBytes(UTF_8)); + zos.closeEntry(); + } + + try (var zip = new ZipFile(zipFile)) { + assertThat(getEntry(zip, "/foo.txt")) // + .isNotNull() // + .extracting(ZipEntry::getName) // + .isEqualTo("/foo.txt"); + + assertThat(getEntry(zip, "foo.txt")) // + .isNotNull() // + .extracting(ZipEntry::getName) // + .isEqualTo("/foo.txt"); + + assertThat(getEntry(zip, "/bar.txt")) // + .isNotNull() // + .extracting(ZipEntry::getName) // + .isEqualTo("bar.txt"); + + assertThat(getEntry(zip, "bar.txt")) // + .isNotNull() // + .extracting(ZipEntry::getName) // + .isEqualTo("bar.txt"); + + assertThat(getEntry(zip, "nonexistent.txt")) // + .isNull(); + } + } + + @Test + void testWriteEntry(@TempDir Path tempDir) throws Exception + { + var zipFile = tempDir.resolve("test.zip").toFile(); + + try (var fos = new FileOutputStream(zipFile); var zos = new ZipOutputStream(fos)) { + writeEntry(zos, "/foo.txt", os -> os.write("foo".getBytes(UTF_8))); + writeEntry(zos, "bar.txt", os -> os.write("bar".getBytes(UTF_8))); + } + + try (var zf = new ZipFile(zipFile)) { + assertThat(zf.getEntry("foo.txt")) // + .isNotNull() // + .extracting(ZipEntry::getName) // + .isEqualTo("foo.txt"); + assertThat(zf.getEntry("bar.txt")) // + .isNotNull() // + .extracting(ZipEntry::getName) // + .isEqualTo("bar.txt"); + } + } + + @Test + void testNormalizeEntryName() + { + // Test entry with leading slash + assertThat(normalizeEntryName(new ZipEntry("/foo.txt"))).isEqualTo("foo.txt"); + + // Test entry without leading slash + assertThat(normalizeEntryName(new ZipEntry("bar.txt"))).isEqualTo("bar.txt"); + + // Test entry with multiple leading slashes + assertThat(normalizeEntryName(new ZipEntry("///baz.txt"))).isNull(); + + // Test entry with no name + assertThat(normalizeEntryName(new ZipEntry(""))).isNull(); + } +}