Ability to extend supported extension - file parser mapping #3690 by fractal3000 · Pull Request #3683 · jmix-framework/jmix

fractal3000 · 2024-09-07T19:41:45Z

fractal3000 · 2024-09-09T09:11:09Z

I moved it to "draft" to add specific logging checks in tests.

glebfox · 2024-09-10T12:15:03Z

jmix-search/search/search.gradle

    testImplementation 'org.junit.vintage:junit-vintage-engine'
+    testImplementation 'org.spockframework:spock-core'
    testImplementation 'org.mockito:mockito-core'
+    testImplementation 'org.spockframework:spock-core'


duplicate dependency

Spock, still, added twice

glebfox · 2024-09-10T12:17:05Z

jmix-search/search/src/main/java/io/jmix/search/exception/ParserResolvingException.java

+
+package io.jmix.search.exception;
+
+public abstract class ParserResolvingException extends Exception {


glebfox · 2024-09-10T12:21:30Z

jmix-search/search/src/main/java/io/jmix/search/exception/EmptyFileExtensionException.java

+public class EmptyFileExtensionException extends ParserResolvingException {

-public class UnsupportedFileFormatException extends Exception {
+    public static final String MESSAGE = "Extension of the file %s is empty";


Don't add a public API if it is not intended to be used by users. You don't event use this constant in tests.

glebfox · 2024-09-10T12:21:47Z

...-search/search/src/main/java/io/jmix/search/exception/UnsupportedFileExtensionException.java

+
+public class UnsupportedFileExtensionException extends ParserResolvingException {
+
+    public static final String MESSAGE = "The file %s with '%s' extension is not supported. " +


Don't add a public API if it is not intended to be used by users. You don't event use this constant in tests.

glebfox · 2024-09-10T12:38:56Z

...-search/search/src/main/java/io/jmix/search/exception/UnsupportedFileExtensionException.java

+public class UnsupportedFileExtensionException extends ParserResolvingException {
+
+    public static final String MESSAGE = "The file %s with '%s' extension is not supported. " +
+            "Only following file extensions are supported %s.";


I thought we decided not to specify which extensions are allowed

I left it so because the problem with "fogotten extensions" had been solved.

glebfox · 2024-09-10T12:47:59Z

jmix-search/search/src/main/java/io/jmix/search/utils/SupportedFileExtensions.java

+        this.parserSupplier = parserSupplier;
+    }
+
+    public String getSymbols() {


These symbols are called extension.

I did so to make this field name be different from the class name that countains a "extension" word.

glebfox · 2024-09-10T13:12:59Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileProcessor.java

-import java.util.Optional;

-@Component
+@Component("search_FileProcessor")


We don't change public API. Bean name is public API

fixed. But this package is marked as "internal"

glebfox · 2024-09-10T13:13:11Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileProcessor.java

    private static final Logger log = LoggerFactory.getLogger(FileProcessor.class);

    protected FileStorageLocator fileStorageLocator;
+    private final FileParserResolver fileParserResolver;


glebfox · 2024-09-10T13:14:27Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileProcessor.java

        return stringWriter.toString();
    }
-
-    protected Parser getParser(FileRef fileRef) throws UnsupportedFileFormatException {


Revert both methods and call fileParserResolver from them.

Fixed. But this class is in the package is annotated with an "Internal" annotation.

glebfox · 2024-09-10T13:51:51Z

jmix-search/search/src/main/java/io/jmix/search/utils/SupportedFileExtensions.java

+
+import java.util.function.Supplier;
+
+public enum SupportedFileExtensions {


What is the point of this enum? It still cannot be extended if needed + you've created FileParserResolver bean to iterate over available constants which can be easily done in the enum itself (see any enum for entity attribute).

Remember that the issue that you fix is a bug, not a feature. But all these changes to much for a bug fix.

The main goal of using enums is much more safe using them as string constansts in the "switch" expressions. If the new item is added to a "enum" and the item is not added to the correspondant "swith" constructions the compile time error will occured. Another point is to incapsulate the string constants and the parsers into the "enum" constants. This makes code more clean and reliable.

The FileParserResolver have been created to icapsulate getting a parser into a "enum" counstant. If we should add any new extention we will have to point the correspondant parser in the same time. And there are no an ability to do forget to do it. Such incupsulating makes string link between the string constant of the extension(such as "txt" or "doc") and the parser. If we do so a proggamer can't add any new extension and not add a parser for it. As a result of adding such "enum" item a programmer doesn't need to add any changes to algoritms. The case of using the new file format will be supported automatically.

Except the small change in the io.jmix.search.utils.FileProcessor#getParser method signature there are no any other breaking changes in the public API. Additionaly the changing type of the exception of this method was approved by Ivan Gavrilov and this change is already commited to the release 2.3 and to the "master" branches. The other changes are not breaking.

And the io.jmix.search.utils.FileProcessor class is in the "internal" marked package. And with "The boy scout rule".

Additionally I found at least two places in code with incorrect behaviour(excepting the main problem).

glebfox · 2024-09-16T12:14:36Z

...earch/search/src/main/java/io/jmix/search/index/fileparsing/resolvers/PDFParserResolver.java

+
+@Component("search_PDFParserResolver")
+@Order(100)
+public class PDFParserResolver implements FileParserResolver {


Name and extension do not match

Fixed and covered by the unit test

glebfox · 2024-09-16T12:14:49Z

...earch/search/src/main/java/io/jmix/search/index/fileparsing/resolvers/RTFParserResolver.java

+
+@Component("search_RTFParserResolver")
+@Order(100)
+public class RTFParserResolver implements FileParserResolver {


Name and extension do not match

glebfox · 2024-09-16T12:22:39Z

jmix-search/search/search.gradle

    testImplementation 'org.junit.vintage:junit-vintage-engine'
+    testImplementation 'org.spockframework:spock-core'
    testImplementation 'org.mockito:mockito-core'
+    testImplementation 'org.spockframework:spock-core'


Spock, still, added twice

glebfox · 2024-09-16T12:37:59Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+     * Returns a collection of supported extensions of the supported file type. E.g. ["xlsx", "XLSX", "DOCX", "DOCX"].
+     * @return collection of supported extensions
+     */
+    List<String> getExtension();


getSupportedExtensions()

Usually, when we have a collection of beans that may or may not be applied, they either have a method that returns true/false whether this bean can be used, e.g. boolean supports(...) (ConditionGenerator, AuthenticationLocaleResolver, etc. just try to search for such method). Another option is to have a @Nullable method and the first bean which return a value is used, e.g. ComponentGenerationStrategy.

The point is that the calling code should not check applicability itself. And one of the reasons is that the supports method can be implemented differently for each bean.

In this case, the option with the supports method is better then @Nullable return value.

Ok, you can leave getExtension() method to collect all available extensions for an exception, just rename it to getSupportedExtensions()

The more flexible engine was implemented. This method was moved to the abstract class. The more general method "getCriteriaDescription()" was described instead of it.

glebfox · 2024-09-16T12:42:50Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+public interface FileParserResolver {
+
+    /**
+     * Returns a collection of supported extensions of the supported file type. E.g. ["xlsx", "XLSX", "DOCX", "DOCX"].


None of FileParserResolver support uppercase extensions

It is not an implementation. The interface gives the ability to provide both lowercase and uppercase extensions.

glebfox · 2024-09-16T12:43:27Z

...ch/src/test/groovy/io/jmix/search/utils/parserresolving/FileParserResolverManagerTest.groovy

+        def parserResolver = new FileParserResolverManager(List.of(resolver, resolver2))
+
+        expect:
+        parserResolver.getParser(createFileRefMock("docx")) == parser1


make tests for uppercase extensions

glebfox · 2024-09-23T12:23:04Z

.../groovy/io/jmix/search/utils/parserresolving/FileParserResolverManagerIntegrationTest.groovy

+        "rtf"     | RTFParser
+        "odt"     | OpenDocumentParser
+        "ods"     | OpenDocumentParser
+        "doc"     | OfficeParser


Still no tests for UPPER CASE extensions which are not supported

glebfox · 2024-09-23T12:25:00Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileParserResolverManager.java

+        this.fileParserResolvers = fileParserResolvers;
+    }
+
+    public Parser getParser(FileRef fileRef) throws UnsupportedFileTypeException {


The class is in the internal package. Is it neccessary to add JavaDoc for all its methods?

glebfox · 2024-09-23T12:32:33Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileParserResolverManager.java

+
+    public Parser getParser(FileRef fileRef) throws UnsupportedFileTypeException {
+        if (fileParserResolvers.isEmpty()) {
+            throw new EmptyFileParserResolversList();


Exception classes must be named in form of FooException

You don't need a custom exception class until you explicitly handle it. Throw IllegalStateException

Agree. Fixed

glebfox · 2024-09-23T12:34:49Z

jmix-search/search/src/main/java/io/jmix/search/exception/UnsupportedFileTypeException.java

+ * An exception that is thrown when a user added some file of the type that is not supported
+ * and there are no any known parser for.
+ */
+public class UnsupportedFileTypeException extends Exception {


For 2.3 you have a different exception class name. You cannot rename public API, so either revert prev name or rename it in 2.3 until we published a next bug fix release.

glebfox · 2024-09-23T12:39:11Z

jmix-search/search/src/main/java/io/jmix/search/exception/EmptyFileParserResolversList.java

 package io.jmix.search.exception;

-import org.apache.commons.io.FilenameUtils;
+public class EmptyFileParserResolversList extends RuntimeException {


Exception class must be named in form FooException

You don't need a custom exception class until you explicitly handle it.

This exception was deleted. Fixed.

glebfox · 2024-09-23T12:41:41Z

...main/java/io/jmix/search/index/fileparsing/resolvers/OldMSOfficeDocumentsParserResolver.java

+@Component("search_OldOfficeDocumentsParserResolver")
+@Order(100)
+public class OldMSOfficeDocumentsParserResolver extends AbstractExtensionBasedFileParserResolver {
+    @Override


According to https://youtrack.haulmont.com/articles/PLAT-A-1158/Code-Style#Java

Empty line after class or interface definition

P.S. fix all classes

glebfox · 2024-09-23T12:48:25Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+import java.util.List;
+
+/**
+ * Is a part of the extendable engine the gives an ability to implement custom file parser resolvers and to support


JavaDoc is written for API users, not for developers who will implement it. For example:

Interface to be implemented by a custom file parser resolver

Check https://youtrack.haulmont.com/articles/PLAT-A-1158/Code-Style#Javadocs

glebfox · 2024-09-23T12:49:55Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+public interface FileParserResolver {
+
+    /**
+     * This method should return the description that describes the constraints or the constraint for the files


JavaDoc is written for API users, not for developers who will implement it. Writing style for a methods: starting with the verb defining the main action.

Check https://youtrack.haulmont.com/articles/PLAT-A-1158/Code-Style#Javadocs

glebfox · 2024-09-23T12:50:10Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+    Parser getParser();
+
+    /**
+     * This method should implement the logic for checking


JavaDoc is written for API users, not for developers who will implement it. Writing style for a methods: starting with the verb defining the main action.

Check https://youtrack.haulmont.com/articles/PLAT-A-1158/Code-Style#Javadocs

glebfox · 2024-09-23T12:51:41Z

jmix-search/search/src/main/java/io/jmix/search/exception/UnsupportedFileTypeException.java

+            "Only the following file parsing criteria are supported:\n  -%s";
+
+    /**
+     * @param fileName            - the name of the file which type is not supported


The hyphen is redundant.

Gavrilov-Ivan · 2024-09-24T07:26:03Z

...src/main/java/io/jmix/search/index/fileparsing/AbstractExtensionBasedFileParserResolver.java

+public abstract class AbstractExtensionBasedFileParserResolver implements FileParserResolver {
+
+    /**
+     * Returns a collection of supported extensions of the supported file type. E.g. ["xlsx", "XLSX", "docx", "DOCX"].


Do we have to explicitly specify both lowercase and uppercase extensions now?
Lets require (and mention it in javadoc) lowercase only and convert the actual extension to lowercase within support method.

We discussed with Gleb such option but we desided not to implement it within this issue. It will be implemented in a separate issue. #3696

The JavaDoc was corrected

Gavrilov-Ivan · 2024-09-24T07:26:13Z

...src/main/java/io/jmix/search/index/fileparsing/AbstractExtensionBasedFileParserResolver.java

+     *
+     * @return collection of supported extensions
+     */
+    public abstract List<String> getSupportedExtensions();


Set is better for contains operation than List.

Gavrilov-Ivan · 2024-09-24T07:32:58Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+public interface FileParserResolver {
+
+    /**
+     * This method should return the description that describes the constraints or the constraint for the files


Method name is getCriteriaDescription but javadoc tells about "constraints".

Gavrilov-Ivan · 2024-09-24T07:37:04Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

+     *
+     * @return criteria description
+     */
+    String getCriteriaDescription();


Do we actually need this public API method?
Isn't it better to delegate this logic to the final consumer? The only purpose of this is to generate message like 'The file extension should be one of the following: ...'.

The consumer doesn't know anything about the file checking criteria that are implemented in the resolvers. The aim was to give to the user ability to get comprehensive information what is going wrong. If we remove this method we just could say that "A resolver(and parser) for the file couldn't be found".

The message of the AbstractExtensionBasedFileParserResolver was corrected.

The method was left without changes as it was discussed.

Gavrilov-Ivan · 2024-09-24T07:46:27Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileParserResolverManager.java

+ * A search principle is based on the sequential applying FileParserResolver objects' checks for the given file.
+ */
+@Component("search_FileParserResolverManager")
+public class FileParserResolverManager {


This class consumes existing FileParserResolvers internally, but doesn't provide outside anything about them.
It provides the final Parsers.
So maybe this class should be named as FileParserProvider?

Agree, fixed

Gavrilov-Ivan · 2024-09-24T08:08:32Z

jmix-search/search/src/main/java/io/jmix/search/utils/FileParserResolverManager.java

+            if (resolver.supports(fileRef)) {
+                return resolver.getParser();
+            }
+            messages.add(resolver.getCriteriaDescription());


If we try to process completely unsupported file it will collect "criteria descriptions" from all existing resolvers.
As a result the final exception message will contain multiple "The file extension should be one of the following: ..." expressions.

That is the point in favor of the FileParserResolver#getCriteriaDescription method irrelevance.

This loop should collect extensions supported by unsuitable resolvers and provide them to exception if non of existing resolvers is suitable. And exception generates the final message based on provided extensions.

Discussed. Left without changes.

Gavrilov-Ivan · 2024-09-27T14:49:53Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParserResolver.java

     * @return an instance of a file parser
     */
-    Parser getParser();
+    FileParsingBundle getParsingBundle();


Maybe FileParserKit?

Gavrilov-Ivan · 2024-09-27T14:58:20Z

jmix-search/search/src/main/java/io/jmix/search/index/fileparsing/FileParsingBundle.java

+
+public record FileParsingBundle(
+        @NotNull Parser parser,
+        @NotNull Function<StringWriter, BodyContentHandler> bodyContentHandlerGenerator,


Use ContentHandler.
BodyContentHandler is a specific implementation.

…olver

$@fractal3000$ fractal3000 requested review from Gavrilov-Ivan and glebfox September 7, 2024 19:41

$@fractal3000$ fractal3000 marked this pull request as draft September 9, 2024 09:09

$@fractal3000$ fractal3000 marked this pull request as ready for review September 9, 2024 13:57

glebfox requested changes Sep 10, 2024

View reviewed changes

$@fractal3000$ fractal3000 linked an issue Sep 11, 2024 that may be closed by this pull request

Ability to extend supported extension - file parser mapping #3690

Open

$@fractal3000$ fractal3000 requested a review from glebfox September 13, 2024 14:14

glebfox requested changes Sep 16, 2024

View reviewed changes

$@fractal3000$ fractal3000 changed the title ~~"Search" add-on adds unnecessary stacktrace to log if an unsupported file format is provided #3660~~ Ability to extend supported extension - file parser mapping #3690 Sep 18, 2024

$@fractal3000$ fractal3000 requested a review from glebfox September 18, 2024 21:00

glebfox requested changes Sep 23, 2024

View reviewed changes

Gavrilov-Ivan requested changes Sep 24, 2024

View reviewed changes

$@fractal3000$ fractal3000 requested review from Gavrilov-Ivan and glebfox September 26, 2024 21:54

Gavrilov-Ivan requested changes Sep 27, 2024

View reviewed changes

$@fractal3000$ fractal3000 marked this pull request as draft September 27, 2024 16:01

glebfox removed their request for review September 30, 2024 10:57

fractal3000 added 12 commits September 18, 2025 18:09

$@fractal3000$

Message text correction

62c0383

$@fractal3000$

Parser resolving mechanism refactoring.

56af748

$@fractal3000$

UnsupportedFileExtensionExceptionTest

23a2f66

$@fractal3000$

FileParserResolverTest

e5ed2ce

$@fractal3000$

FileParserResolverTest

0b630ac

$@fractal3000$

File type correction

c51699d

$@fractal3000$

UnsupportedFileExtensionExceptionTest

230e41a

$@fractal3000$

Groovy tests correction

a8d720b

$@fractal3000$

minor change

5f81bf0

$@fractal3000$

minor change

078543b

$@fractal3000$

FilePropertyValueExtractorTest

856366c

$@fractal3000$

FilePropertyValueExtractorTest enhancement

939b560

fractal3000 added 20 commits September 18, 2025 18:11

$@fractal3000$

minor change

2d891cd

$@fractal3000$

code formatting

67753ff

$@fractal3000$

Capital letters checking

4f0d053

$@fractal3000$

Removing not necessary custom exception

545302b

$@fractal3000$

Renaming the exception

c7475c7

$@fractal3000$

Code style changes

d04cf63

$@fractal3000$

JavaDocs correction

11c0b80

$@fractal3000$

JavaDocs correction

0bf2067

$@fractal3000$

Test correction

8292c88

$@fractal3000$

List to Set changing

33fcc94

$@fractal3000$

FileParserResolverManager -> FileParserProvider

6b07864

$@fractal3000$

Message text correction

51ee3fd

$@fractal3000$

OldMSOfficeDocumentsParserResolver > LegacyMSOfficeDocumentsParserRes…

e8016ff

…olver

$@fractal3000$

JavaDoc

b6bf51f

$@fractal3000$

Getting FileParsingBundle with FileParserResolver

6da446f

$@fractal3000$

Comment adding

b744d63

$@fractal3000$

Uppercase extensions' support

1d056e6

$@fractal3000$

FileParsingBundle -> FileParserKit

be96ef6

$@fractal3000$

BodyContentHandler -> ContentHandler

11453d8

$@fractal3000$

MSOfficeDocumentsParserResolver

59c6a88

$@fractal3000$ fractal3000 force-pushed the feature/3660-2-search-add-on-adds-unnecessary-stacktrace-to-log-if-an-unsupported-file-format-is-provided branch from 57e81d9 to 59c6a88 Compare September 18, 2025 14:13

fractal3000 added 4 commits September 18, 2025 18:48

$@fractal3000$

A fixing of the wrong merging

5319a28

$@fractal3000$

Not necessary util method

b24d7d3

$@fractal3000$

Tests correction

6f600d9

$@fractal3000$

Откат некорректного переименования

b563375

$@fractal3000$ fractal3000 marked this pull request as ready for review September 19, 2025 16:38

$@fractal3000$ fractal3000 requested review from Gavrilov-Ivan and glebfox September 19, 2025 16:38

glebfox removed the request for review from Gavrilov-Ivan October 10, 2025 07:13

$@fractal3000$ fractal3000 mentioned this pull request Nov 20, 2025

The vulnerability in the Search Add-on's depency #4898

Open


		package io.jmix.search.exception;

		public abstract class ParserResolvingException extends Exception {


		public class UnsupportedFileExtensionException extends ParserResolvingException {

		public static final String MESSAGE = "The file %s with '%s' extension is not supported. " +


		import java.util.function.Supplier;

		public enum SupportedFileExtensions {

Conversation

fractal3000 commented Sep 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fractal3000 commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fractal3000 Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fractal3000 Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

$@fractal3000$ fractal3000 commented Sep 7, 2024 •

edited

Loading

fractal3000 commented Sep 9, 2024 •

edited

Loading

$@fractal3000$ fractal3000 Sep 10, 2024 •

edited

Loading

$@fractal3000$ fractal3000 Sep 10, 2024 •

edited

Loading

$@fractal3000$ fractal3000 Sep 26, 2024 •

edited

Loading