diff --git a/api/src/main/java/org/itsallcode/openfasttrace/api/importer/ImporterFactory.java b/api/src/main/java/org/itsallcode/openfasttrace/api/importer/ImporterFactory.java index 943e9fc29..7740bba8c 100644 --- a/api/src/main/java/org/itsallcode/openfasttrace/api/importer/ImporterFactory.java +++ b/api/src/main/java/org/itsallcode/openfasttrace/api/importer/ImporterFactory.java @@ -20,6 +20,18 @@ protected ImporterFactory() // empty by intention } + /** + * Get the priority of this {@link ImporterFactory}. + *

+ * The priority is used to determine the order in which importer factories + * are tried when importing a file. Lower priority values indicate higher + * precedence. + *

+ * + * @return priority of this importer factory + */ + public abstract int getPriority(); + /** * Returns {@code true} if this {@link ImporterFactory} supports * importing the given file based on its file extension. diff --git a/core/src/main/java/org/itsallcode/openfasttrace/core/importer/ImporterFactoryLoader.java b/core/src/main/java/org/itsallcode/openfasttrace/core/importer/ImporterFactoryLoader.java index a22589e41..dda9cbc37 100644 --- a/core/src/main/java/org/itsallcode/openfasttrace/core/importer/ImporterFactoryLoader.java +++ b/core/src/main/java/org/itsallcode/openfasttrace/core/importer/ImporterFactoryLoader.java @@ -1,6 +1,7 @@ package org.itsallcode.openfasttrace.core.importer; import java.nio.file.Path; +import java.util.Comparator; import java.util.List; import java.util.Optional; import java.util.logging.Logger; @@ -17,7 +18,6 @@ public class ImporterFactoryLoader { private static final Logger LOG = Logger.getLogger(ImporterFactoryLoader.class.getName()); - private final Loader serviceLoader; /** @@ -45,29 +45,27 @@ public ImporterFactoryLoader(final ImporterContext context) /** * Finds a matching {@link ImporterFactory} that can handle the given - * {@link Path}. If no or more than one {@link ImporterFactory} is found, - * this throws an {@link ImporterException}. + * {@link Path}. If no {@link ImporterFactory} is found, this throws an + * {@link ImporterException}. * * @param file * the file for which to get a {@link ImporterFactory}. * @return a matching {@link ImporterFactory} that can handle the given * {@link Path} * @throws ImporterException - * when no or more than one {@link ImporterFactory} is found. + * when no {@link ImporterFactory} is found. */ public Optional getImporterFactory(final InputFile file) { - final List matchingImporters = getMatchingFactories(file); - switch (matchingImporters.size()) + final List matchingImporters = getMatchingFactoriesInOrderOfPriority(file); + if (matchingImporters.isEmpty()) { - case 0: LOG.info(() -> "Found no matching importer for file '" + file + "'"); return Optional.empty(); - case 1: + } + else + { return Optional.of(matchingImporters.get(0)); - default: - LOG.info(() -> "Found more than one matching importer for file '" + file + "'"); - return Optional.empty(); } } @@ -81,12 +79,12 @@ public Optional getImporterFactory(final InputFile file) */ public boolean supportsFile(final InputFile file) { - final boolean supported = !getMatchingFactories(file).isEmpty(); + final boolean supported = !getMatchingFactoriesInOrderOfPriority(file).isEmpty(); LOG.finest(() -> "File " + file + " is supported = " + supported); return supported; } - private List getMatchingFactories(final InputFile file) + private List getMatchingFactoriesInOrderOfPriority(final InputFile file) { final List allFactories = this.serviceLoader.load().toList(); if (allFactories.isEmpty()) @@ -96,6 +94,7 @@ private List getMatchingFactories(final InputFile file) } return allFactories.stream() .filter(f -> f.supportsFile(file)) + .sorted(Comparator.comparingInt(ImporterFactory::getPriority)) .toList(); } } diff --git a/core/src/test/java/org/itsallcode/openfasttrace/core/importer/TestImporterFactoryLoader.java b/core/src/test/java/org/itsallcode/openfasttrace/core/importer/TestImporterFactoryLoader.java index ac4fb741e..6a4f21365 100644 --- a/core/src/test/java/org/itsallcode/openfasttrace/core/importer/TestImporterFactoryLoader.java +++ b/core/src/test/java/org/itsallcode/openfasttrace/core/importer/TestImporterFactoryLoader.java @@ -1,15 +1,15 @@ package org.itsallcode.openfasttrace.core.importer; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.same; -import static org.mockito.Mockito.lenient; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import java.nio.file.Paths; import java.util.Arrays; +import java.util.Optional; import org.itsallcode.openfasttrace.api.importer.ImporterException; import org.itsallcode.openfasttrace.api.importer.ImporterFactory; @@ -26,8 +26,7 @@ * Test for {@link ImporterFactoryLoader} */ @ExtendWith(MockitoExtension.class) -class TestImporterFactoryLoader -{ +class TestImporterFactoryLoader { @Mock private Loader serviceLoaderMock; @Mock @@ -41,8 +40,7 @@ class TestImporterFactoryLoader private InputFile file; @BeforeEach - void beforeEach() - { + void beforeEach() { this.loader = new ImporterFactoryLoader(this.serviceLoaderMock); this.file = RealFileInput.forPath(Paths.get("dir", "name")); @@ -52,49 +50,62 @@ void beforeEach() } @Test - void testNoFactoryRegisteredThrowsException() - { + void testNoFactoryRegisteredThrowsException() { simulateFactories(); assertThrows(ImporterException.class, () -> this.loader.getImporterFactory(this.file)); } @Test - void testSupportsFileWhenNoFactoryRegisteredThrowsException() - { + void testSupportsFileWhenNoFactoryRegisteredThrowsException() { simulateFactories(); assertThrows(ImporterException.class, () -> this.loader.supportsFile(this.file)); } @Test - void testMatchingFactoryFoundOnlyOneAvailable() - { + void testMatchingFactoryFoundOnlyOneAvailable() { simulateFactories(this.supportedFactory1); assertFactoryFound(this.supportedFactory1); } @Test - void testMatchingFactoryFoundTwoAvailable() - { + void testMatchingFactoryFoundTwoAvailable() { simulateFactories(this.supportedFactory1, this.unsupportedFactory); assertFactoryFound(this.supportedFactory1); } @Test - void testMultipleMatchingFactoriesFoundReturnNoImporter() - { - simulateFactories(this.supportedFactory1, this.supportedFactory1); - assertTrue(this.loader.getImporterFactory(this.file).isEmpty()); + void testMultipleMatchingFactoriesReturnsTopPriority(@Mock ImporterFactory priority1Factory, + @Mock ImporterFactory priority2Factory, + @Mock ImporterFactory priority3Factory) { + when(priority1Factory.supportsFile(same(this.file))).thenReturn(true); + when(priority2Factory.supportsFile(same(this.file))).thenReturn(true); + when(priority3Factory.supportsFile(same(this.file))).thenReturn(true); + when(priority1Factory.getPriority()).thenReturn(1); + when(priority2Factory.getPriority()).thenReturn(2); + when(priority3Factory.getPriority()).thenReturn(3); + simulateFactories(priority2Factory, priority1Factory, priority3Factory); + assertThat(this.loader.getImporterFactory(this.file).orElseThrow().getPriority(), equalTo(1)); } - private void assertFactoryFound(final ImporterFactory expectedFactory) - { - assertThat(this.loader.getImporterFactory(this.file).orElseThrow(() -> - new AssertionError("Unable to find ImporterFactory.") - ), sameInstance(expectedFactory)); + @Test + void testNoMatchingFactoriesReturnsEmpty(@Mock final ImporterFactory unused) { + when(unused.supportsFile(same(this.file))).thenReturn(false); + simulateFactories(unused); + assertThat(this.loader.getImporterFactory(this.file), equalTo(Optional.empty())); + } + + @Test + void testSupportsFile() { + simulateFactories(this.supportedFactory1); + assertThat(this.loader.supportsFile(this.file), equalTo(true)); + } + + private void assertFactoryFound(final ImporterFactory expectedFactory) { + assertThat(this.loader.getImporterFactory(this.file).orElseThrow( + () -> new AssertionError("Unable to find ImporterFactory.")), sameInstance(expectedFactory)); } - private void simulateFactories(final ImporterFactory... factories) - { + private void simulateFactories(final ImporterFactory... factories) { when(this.serviceLoaderMock.load()).thenReturn(Arrays.stream(factories)); } } diff --git a/doc/changes/changes_4.5.0.md b/doc/changes/changes_4.5.0.md index adea60b84..e3f447286 100644 --- a/doc/changes/changes_4.5.0.md +++ b/doc/changes/changes_4.5.0.md @@ -12,10 +12,13 @@ We also refactored the tests around the CLI starter to improve readability and m We added FXML to the list of supported file formats for the tag importer. +File extensions can now be handled by multiple importers. Importers have a priority order, the first importer that can handle a file extension will be used. Since the XML importer has a peek function to detect SpecObject files, it can decide not to handle an XML file, in which case the tag importer can take over. + When no importer factory is found for a given file format, the importer factory loader will now throw an exception that explains the likely cause. This is helpful in case OFT is used as a library with a custom classloader and thus does not have the right context. ## Features +* #352: Tag parsing in XML documents * #503: Added `-h` / `--help` to the command line. * #506: Exception when no importer was found. * #524: Added version number to help text. diff --git a/doc/spec/design.md b/doc/spec/design.md index 2a4e0104f..7a88a4af4 100644 --- a/doc/spec/design.md +++ b/doc/spec/design.md @@ -178,7 +178,51 @@ Depending on the source format, a variety of [importers](#importers) takes care The listener handles Common parts of the import like filtering out unnecessary items or attributes. -A factory for importers decides which importer to use. Usually, by file extension. +The following sequence diagram illustrates the interaction between `MultiFileImporter`, `ImporterFactoryLoader`, `ImporterFactory`, and `Importer` during the import process: + +```puml +@startuml +participant MultiFileImporterImpl as MFI +participant ImporterFactoryLoader as IFL +participant ImporterFactory as IF +participant Importer as I +participant SpecificationListBuilder as SLB + +[-> MFI : importFile(file) +activate MFI +MFI -> IFL : getImporterFactory(file) +activate IFL +IFL -> IF : supportsFile(file) +activate IF +return true +IFL -> IF : getPriority() +activate IF +return priority +note over IFL: Choose factory with\nlowest priority value +return factory +MFI -> IF : createImporter(file, specItemBuilder) +activate IF +create I +IF -> I : new +return importer +MFI -> I : runImport() +activate I +I -> SLB : event(item) +activate SLB +return +return +return +@enduml +``` + +A factory for importers decides which importer to use. When multiple importers support the same file, the one with the lowest priority value (highest precedence) is chosen. Usually, importers are selected based on file extension, but some importers may peek into the file content to determine compatibility. + +The default priorities for standard importers are: +1. Markdown Importer: 1000 +2. reStructuredText Importer: 2000 +3. Specobject (ReqM2) Importer: 3000 +4. Tag Importer: 10000 +5. Zip Importer: 20000 ### ReqM2 File Detection `dsn~import.reqm2-file-detection~1` @@ -188,6 +232,8 @@ The `SpecobjectImporterFactory` detects ReqM2 files either 1. via the file extension `.oreqm` or 2. via the file extension `.xml` and the presence of the string ` getSupportedFilenames() { diff --git a/importer/restructuredtext/src/main/java/org/itsallcode/openfasttrace/importer/restructuredtext/RestructuredTextImporterFactory.java b/importer/restructuredtext/src/main/java/org/itsallcode/openfasttrace/importer/restructuredtext/RestructuredTextImporterFactory.java index cd9b77e21..c5d33d152 100644 --- a/importer/restructuredtext/src/main/java/org/itsallcode/openfasttrace/importer/restructuredtext/RestructuredTextImporterFactory.java +++ b/importer/restructuredtext/src/main/java/org/itsallcode/openfasttrace/importer/restructuredtext/RestructuredTextImporterFactory.java @@ -14,6 +14,11 @@ public RestructuredTextImporterFactory() super("(?i).*\\.rst"); } + @Override + public int getPriority() { + return 2000; + } + @Override public Importer createImporter(final InputFile fileName, final ImportEventListener listener) { diff --git a/importer/restructuredtext/src/test/java/org/itsallcode/openfasttrace/importer/restructuredtext/TestRestructuredTextImporterFactory.java b/importer/restructuredtext/src/test/java/org/itsallcode/openfasttrace/importer/restructuredtext/TestRestructuredTextImporterFactory.java index d42528ae0..accf2fafd 100644 --- a/importer/restructuredtext/src/test/java/org/itsallcode/openfasttrace/importer/restructuredtext/TestRestructuredTextImporterFactory.java +++ b/importer/restructuredtext/src/test/java/org/itsallcode/openfasttrace/importer/restructuredtext/TestRestructuredTextImporterFactory.java @@ -17,6 +17,11 @@ protected RestructuredTextImporterFactory createFactory() return new RestructuredTextImporterFactory(); } + @Override + protected int getExpectedPriority() { + return 2000; + } + @Override protected List getSupportedFilenames() { diff --git a/importer/specobject/src/main/java/org/itsallcode/openfasttrace/importer/specobject/SpecobjectImporterFactory.java b/importer/specobject/src/main/java/org/itsallcode/openfasttrace/importer/specobject/SpecobjectImporterFactory.java index 5e14e1c44..d4137d163 100644 --- a/importer/specobject/src/main/java/org/itsallcode/openfasttrace/importer/specobject/SpecobjectImporterFactory.java +++ b/importer/specobject/src/main/java/org/itsallcode/openfasttrace/importer/specobject/SpecobjectImporterFactory.java @@ -27,6 +27,11 @@ public SpecobjectImporterFactory() this.xmlParserFactory = new XmlParserFactory(); } + @Override + public int getPriority() { + return 3000; + } + // [impl -> dsn~import.reqm2-file-detection~1] @Override public boolean supportsFile(final InputFile file) diff --git a/importer/specobject/src/test/java/org/itsallcode/openfasttrace/importer/specobject/TestSpecobjectImporterFactory.java b/importer/specobject/src/test/java/org/itsallcode/openfasttrace/importer/specobject/TestSpecobjectImporterFactory.java index 7bd8661c3..fccb70bcc 100644 --- a/importer/specobject/src/test/java/org/itsallcode/openfasttrace/importer/specobject/TestSpecobjectImporterFactory.java +++ b/importer/specobject/src/test/java/org/itsallcode/openfasttrace/importer/specobject/TestSpecobjectImporterFactory.java @@ -23,13 +23,17 @@ class TestSpecobjectImporterFactory extends ImporterFactoryTestBase { - @Override protected SpecobjectImporterFactory createFactory() { return new SpecobjectImporterFactory(); } + @Override + protected int getExpectedPriority() { + return 3000; + } + /** * Only the {@code .oreqm} extension is always supported. That is why we * can't simply list {@code .xml} here. Whether {@code .xml} is supported diff --git a/importer/tag/src/main/java/org/itsallcode/openfasttrace/importer/tag/TagImporterFactory.java b/importer/tag/src/main/java/org/itsallcode/openfasttrace/importer/tag/TagImporterFactory.java index 20033a8f5..b3c705002 100644 --- a/importer/tag/src/main/java/org/itsallcode/openfasttrace/importer/tag/TagImporterFactory.java +++ b/importer/tag/src/main/java/org/itsallcode/openfasttrace/importer/tag/TagImporterFactory.java @@ -23,7 +23,7 @@ public class TagImporterFactory extends ImporterFactory "feature", // Gherkin feature files "go", // Go "groovy", // Groovy - "json", "htm", "html", "xhtml", "yaml", "yml", // markup languages + "json", "htm", "html", "xhtml", "xml", "yaml", "yml", // markup languages "fxml", "java", // Java "clj", "kt", "kts", "scala", // JVM languages "js", "mjs", "cjs", "ejs", // JavaScript @@ -55,6 +55,12 @@ public TagImporterFactory() // empty by intention } + @Override + public int getPriority() { + return 10000; + } + + @Override public boolean supportsFile(final InputFile path) { diff --git a/importer/tag/src/test/java/org/itsallcode/openfasttrace/importer/tag/TestTagImporterFactory.java b/importer/tag/src/test/java/org/itsallcode/openfasttrace/importer/tag/TestTagImporterFactory.java index 8d1b09c66..50a71b4b5 100644 --- a/importer/tag/src/test/java/org/itsallcode/openfasttrace/importer/tag/TestTagImporterFactory.java +++ b/importer/tag/src/test/java/org/itsallcode/openfasttrace/importer/tag/TestTagImporterFactory.java @@ -32,7 +32,8 @@ protected List getSupportedFilenames() "foo.ts", "foo.json", "foo.lua", "foo.m", "foo.mm", "foo.php", "foo.pl", "foo.pls", "foo.pm", "foo.proto", "foo.py", "foo.sql", "foo.r", - "foo.rs", "foo.sh", "foo.sv", "foo.v", "foo.inc", "foo.yaml", "foo.yml", "foo.xhtml", "foo.zsh", + "foo.rs", "foo.sh", "foo.sv", "foo.v", "foo.inc", "foo.yaml", "foo.yml", "foo.xhtml", "foo.xml", + "foo.zsh", "foo.clj", "foo.kt", "foo.scala", "foo.pu", "foo.puml", "foo.plantuml", "foo.go", "foo.robot", "foo.tf", "foo.tfvars", "foo.toml"); } @@ -43,4 +44,10 @@ protected List getUnsupportedFilenames() return asList("file.md", "file.jav", "file.ml", "file.1java", "file.java1", "file.java.md", "file_java", "filejava"); } + + @Override + protected int getExpectedPriority() + { + return 10000; + } } diff --git a/importer/zip/src/main/java/org/itsallcode/openfasttrace/importer/zip/ZipFileImporterFactory.java b/importer/zip/src/main/java/org/itsallcode/openfasttrace/importer/zip/ZipFileImporterFactory.java index f322c8b5d..f94d6b9e7 100644 --- a/importer/zip/src/main/java/org/itsallcode/openfasttrace/importer/zip/ZipFileImporterFactory.java +++ b/importer/zip/src/main/java/org/itsallcode/openfasttrace/importer/zip/ZipFileImporterFactory.java @@ -18,6 +18,11 @@ public ZipFileImporterFactory() super("(?i).*\\.(zip)"); } + @Override + public int getPriority() { + return 20000; + } + @Override public Importer createImporter(final InputFile file, final ImportEventListener listener) { diff --git a/importer/zip/src/test/java/org/itsallcode/openfasttrace/importer/zip/TestZipFileImporterFactory.java b/importer/zip/src/test/java/org/itsallcode/openfasttrace/importer/zip/TestZipFileImporterFactory.java index e2609afbd..c91f18313 100644 --- a/importer/zip/src/test/java/org/itsallcode/openfasttrace/importer/zip/TestZipFileImporterFactory.java +++ b/importer/zip/src/test/java/org/itsallcode/openfasttrace/importer/zip/TestZipFileImporterFactory.java @@ -20,7 +20,7 @@ class TestZipFileImporterFactory extends ImporterFactoryTestBase + + + foobar + 1 + + + """; + Files.write(tempDir.resolve("specobject.xml"), specObjectContent.getBytes()); + final String xmlContent = ""; + Files.write(tempDir.resolve("non_specobject.xml"), xmlContent.getBytes()); + final ImportSettings settings = ImportSettings.builder() + .addInputs(tempDir) + .filter(FilterSettings.builder().build()) + .build(); + final List items = oft.importItems(settings); + assertThat(items, containsInAnyOrder( + hasProperty("id", hasToString("dsn~foobar~1")), + hasProperty("id", hasToString(startsWith("impl~foobar"))) + )); + } +} diff --git a/testutil/src/main/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBase.java b/testutil/src/main/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBase.java index f76f9ce99..c6400c422 100644 --- a/testutil/src/main/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBase.java +++ b/testutil/src/main/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBase.java @@ -93,6 +93,18 @@ void testMissingContextThrowsException() "Context was not initialized"); } + @Test + void testGetPriority() { + assertThat(createFactory().getPriority(), equalTo(getExpectedPriority())); + } + + /** + * Get the priority the importer factory should report. + * + * @return expected priority + */ + protected abstract int getExpectedPriority(); + private void assertSupported(final List filenames, final boolean expectedResult) { final T factory = createFactory(); diff --git a/testutil/src/test/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBaseTest.java b/testutil/src/test/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBaseTest.java index a4011ec6c..67470c659 100644 --- a/testutil/src/test/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBaseTest.java +++ b/testutil/src/test/java/org/itsallcode/openfasttrace/testutil/importer/ImporterFactoryTestBaseTest.java @@ -25,6 +25,11 @@ protected DummyImporterFactory createFactory() throw new UnsupportedOperationException("Unimplemented method 'createFactory'"); } + @Override + protected int getExpectedPriority() { + return 0; + } + @Override protected List getSupportedFilenames() { @@ -40,6 +45,11 @@ protected List getUnsupportedFilenames() private static class DummyImporterFactory extends ImporterFactory { + @Override + public int getPriority() { + return 99999999; + } + @Override public boolean supportsFile(final InputFile file) {