MCR-3258 Enable PMD rule ExceptionAsFlowControl

[rsteph-de]

Link to jira.

[rsteph-de]

Alles "schwierig":

Ein paar einfache Fälle konnte ich lösen.
Offen bleiben:

• Fälle von UncheckedExceptions aus anonymen Funktionen (z.B. durch Stream-Verarbeitung) die nach außen gereicht werden, diese finde ich gerechtfertigt.
• andere Fälle, wenn z.B. ein globales Transaktion-Rollback für alle Exceptions notwendig wird.
• Fälle, deren Behebung fehleranfällig und vermutlich zu komplex für 2024.06.LTS sind (schwierg Merges).

Ich tendiere die einfachen Fälle zu mergen, aber dann die Regel ggf. sogar ganz zu deaktivieren.
Es macht vermutlich wenig Sinn, die ca. 20 @SupressWarnings so drin zu behalten.

[Mewel]

I didn't finish with all of them will continue later.

[Mewel]

You can try:

Simplify findInConfigLibDir.

    private void findInConfigLibDir(File configDir) throws IOException {
        final Path lib = configDir.toPath().resolve("lib");
        doForFilesRecursive(lib, n -> n.endsWith(".jar"),
            file -> {
                try (InputStream in = Files.newInputStream(file)) {
                    findInJarInputStream(configDir.toPath().relativize(file).toString(), in);
                }
            });
    }

Change method signature to:

private void doForFilesRecursive(Path baseDir, Predicate<String> lowerCaseCheck,
        PathConsumerWithIOException pathConsumer)

Add functional interface:

    @FunctionalInterface
    interface PathConsumerWithIOException {
        void accept(Path path) throws IOException;
    }

[Mewel]

Maybe don't use a stream, this actually looks simpler:

        try (Stream<MCRNode> streamMCRNode = getChildren()) {
            for (Iterator<MCRNode> iterator = streamMCRNode.iterator(); iterator.hasNext(); ) {
                MCRNode node = iterator.next();
                if (node instanceof MCRStoredNode storedNode) {
                    storedNode.repairMetadata();
                }
            }
        }

[Mewel]

I don't understand this code. It looks way to complicated. @yagee-de can you fix this?

[Mewel]

Use iterator again:

@MCRCommand(syntax = "generate md5sum file {0} for ifs2 file store {1}",
        help = "writes md5sum file {0} for every file in MCRFileStore with ID {1}")
    public static void writeMD5SumFile(String targetFile, String ifsStoreId) throws IOException {
        initFileStores();
        final MCRStore store = MCRStoreCenter.getInstance().getStore(ifsStoreId);
        if (!(store instanceof MCRFileStore)) {
            throw new MCRException("Store " + ifsStoreId + " is not found or is not a file store.");
        }
        Path targetPath = Paths.get(targetFile);
        if (!Files.isDirectory(targetPath.getParent())) {
            throw new NotDirectoryException(targetPath.getParent().toString());
        }
        try (BufferedWriter bufferedWriter = Files.newBufferedWriter(targetPath, StandardCharsets.UTF_8,
            StandardOpenOption.CREATE)) {
            final MessageFormat md5FileFormat = new MessageFormat("{0}  {1}\n", Locale.ROOT);
            MCRFileStore fileStore = (MCRFileStore) store;
            Stream<MCRFileCollection> fileCollectionsStream = fileStore.getStoredIDs()
                .sorted()
                .mapToObj(fileStore::retrieve);
            for (Iterator<MCRFileCollection> fcIterator = fileCollectionsStream.iterator(); fcIterator.hasNext();) {
                MCRFileCollection fileCollection = fcIterator.next();
                writeMd5EntriesForCollection(fileCollection, bufferedWriter, md5FileFormat);
            }
        }
    }

    private static void writeMd5EntriesForCollection(MCRFileCollection fileCollection, BufferedWriter bufferedWriter,
        MessageFormat md5FileFormat) throws IOException {
        try (Stream<FileInfo> filesStream =
            getAllFiles(fileCollection.getLocalPath(), fileCollection.getMetadata().getRootElement())) {
            for (Iterator<FileInfo> fileIterator = filesStream.iterator();
                fileIterator.hasNext();) {
                FileInfo fileInfo = fileIterator.next();
                bufferedWriter.write(md5FileFormat.format(new Object[] { fileInfo.md5, fileInfo.localPath }));
            }
        }
    }

[Mewel]

getAllFiles() doesnt even throw an IOException :)

    private static Stream<FileInfo> getAllFiles(Path nodePath, Element node) {
        Function<Element, String> getName = e -> e.getAttributeValue("name");
        return node.getChildren().stream()
            .sorted(Comparator.comparing(getName))
            .flatMap(n -> {
                final String fileName = getName.apply(n);
                if ("dir".equals(n.getName())) {
                    return getAllFiles(nodePath.resolve(fileName), n);
                }
                return Stream.of(new FileInfo(nodePath.resolve(fileName), n.getAttributeValue("md5")));
            });
    }

[Mewel]

It's better to move the annotation to the method instead of the whole class.

We have 3 problems here:

1. getMCRFile for a MCRPath
   I think we can just move the inner try out of it and it should be fixed.

static MCRFile getMCRFile(MCRPath ifsPath, boolean create, boolean createNew, boolean fireCreateEvent)
        throws IOException {
        if (!ifsPath.isAbsolute()) {
            throw new IllegalArgumentException("'path' needs to be absolute.");
        }
        MCRFile file;
        MCRDirectory root;
        boolean rootCreated = false;
        try {
            root = getFileCollection(ifsPath.getOwner());
        } catch (NoSuchFileException e) {
            if (create || createNew) {
                MCRObjectID derId = MCRObjectID.getInstance(ifsPath.getOwner());
                root = getStore(derId.getBase()).create(derId.getNumberAsInteger());
                rootCreated = true;
            } else {
                throw e;
            }
        }
        try {
            MCRPath relativePath = toPath(root).relativize(ifsPath);
            file = getMCRFile(root, relativePath, create, createNew, fireCreateEvent);
        } catch (Exception e) {
            if (rootCreated) {
                LOGGER.error("Exception while getting MCRFile {}. Removing created filesystem nodes.", ifsPath);
                try {
                    root.delete();
                } catch (Exception de) {
                    LOGGER.fatal("Error while deleting file system node: {}", root.getName(), de);
                }
            }
            throw e;
        }
        return file;
    }

2. getMCRFile for MCRDirectory
   Here I would just move the declaration of MCRFile file and the FileAlreadyExistException out of the try block

        Deque<MCRStoredNode> created = new ArrayDeque<>();
        MCRFile file = (MCRFile) baseDir.getNodeByPath(ifsPath.toString());
        if (file != null && createNew) {
            throw new FileAlreadyExistsException(toPath(baseDir).resolve(ifsPath).toString());
        }
        try {
            if (file == null & (create || createNew)) {

3. getObjectBaseIds
   Maybe just do it in two steps. Also looks more readable:

static Collection<String> getObjectBaseIds() throws IOException {
        final Path baseDir = Paths.get(getBaseDir());
        if (!Files.isDirectory(baseDir)) {
            return List.of();
        }
        // Get the list of subdirectories in baseDir.
        List<Path> baseDirDirs;
        try (Stream<Path> baseDirEntries = Files.list(baseDir)) {
            baseDirDirs = baseDirEntries
                .filter(Files::isDirectory)
                .sorted(Comparator.comparing(Path::getFileName))
                .toList();
        }
        // For each directory, list its entries and process them.
        List<String> result = new ArrayList<>();
        for (Path dir : baseDirDirs) {
            try (Stream<Path> subDirStream = Files.list(dir)) {
                subDirStream
                    .filter(path -> MCRObjectID.isValidType(path.getFileName().toString()))
                    .filter(Files::isDirectory)
                    .sorted(Comparator.comparing(Path::getFileName))
                    .map(p -> p.getParent().getFileName().toString() + "_" + p.getFileName())
                    .forEach(result::add);
            }
        }
        return result;
    }

[Mewel]

I would extract it into two methods:

/**
     * @param file
     *            image file
     * @return if content type is in property <code>MCR.Module-iview2.SupportedContentTypes</code>
     * @see MCRContentTypes#probeContentType(Path)
     */
    public static boolean isFileSupported(Path file) throws IOException {
        if(file == null) {
            return isContentTypeSupported(null);
        }
        String contentType = MCRContentTypes.probeContentType(file);
        return isContentTypeSupported(contentType);
    }

    /**
     * @param contentType
     *            content type
     * @return if content type is in property <code>MCR.Module-iview2.SupportedContentTypes</code>
     * @see MCRContentTypes#probeContentType(Path)
     */
    public static boolean isContentTypeSupported(String contentType) {
        if(contentType == null) {
            contentType = "application/octet-stream";
        }
        return SUPPORTED_CONTENT_TYPE.contains(contentType);
    }

[Mewel]

To me it looks like the inner try catch is unecessary because it's just there to add an extra log message.

I would remove the inner try catch. And also update the log message in the "real" catch message:
LOGGER.error("Error while executing tile action.", e);

[Mewel]

Here the API is a bit "stupid" cause there is no "real" IOException. OCFL throws a NotFoundException which is already a runtime exception. To fix it I would suggest:

@Override
    public List<MCRObjectIDDate> retrieveObjectDates(List<String> ids) throws IOException {
        try {
            return ids.stream()
                .map(this::getOCFLObjectID)
                .filter(this::isMetadata)
                .filter(this::isNotDeleted)
                .map(ocflId -> {
                    long lastModified = getLastModifiedInternal(ocflId);
                    return new MCRObjectIDDateImpl(new Date(lastModified), removePrefix(ocflId));
                })
                .collect(Collectors.toList());
        } catch (Exception e) {
            throw new IOException("Unable to retrieve object dates for ids " + ids, e);
        }
    }

    @Override
    public long getLastModified(MCRObjectID id) throws IOException {
        return getLastModified(getOCFLObjectID(id));
    }

    public long getLastModified(String ocflObjectId) throws IOException {
        try {
            return getLastModifiedInternal(ocflObjectId);
        } catch (NotFoundException e) {
            throw new IOException(e);
        }
    }

    private long getLastModifiedInternal(String ocflObjectId) {
        return Date.from(getRepository()
            .getObject(ObjectVersionId.head(ocflObjectId))
            .getVersionInfo()
            .getCreated()
            .toInstant())
            .getTime();
    }

[Mewel]

Here I would catch the WebApplicationException and rethrow it. This is not an internal server error in my opinion.

} catch (WebApplicationException webApplicationException) {
            throw webApplicationException;
        }

[Mewel]

Same here, method is just too big and does to much. Can you take a look @yagee-de?

[Mewel]

would refactor to this:

private void handleFunctionCallRequest(int compilationId, FunctionCallRequest functionCallRequest)
       throws IOException {
       FunctionCallResponse.Builder response = FunctionCallResponse.newBuilder().setId(functionCallRequest.getId());

       Exception exception = switch (functionCallRequest.getIdentifierCase()) {
           case NAME -> new UnsupportedOperationException(
               "Calling function " + functionCallRequest.getName() + " is not supported");
           case FUNCTION_ID -> new UnsupportedOperationException("Calling functions by ID is not supported");
           case IDENTIFIER_NOT_SET -> new IllegalArgumentException("FunctionCallRequest has no identifier");
       };

       LOGGER.debug("Failed to handle FunctionCallRequest", exception);

       response.setError(getErrorMessage(exception));
       connection.sendMessage(compilationId, ProtocolUtil.inboundMessage(response.build()));
   }

[Mewel]

streams abused...

public void deleteObject(MCRObject object) throws SwordServerException {
        try {
            for (MCRMetaLinkID metaLink : object.getStructure().getDerivates()) {
                MCRObjectID id = metaLink.getXLinkHrefID();
                MCRMetadataManager.deleteMCRDerivate(id);
            }
            MCRMetadataManager.delete(object);
        } catch (MCRActiveLinkException | MCRAccessException | MCRPersistenceException e) {
            throw new SwordServerException("Error while deleting Object.", e);
        }
    }

[Mewel]

would change to:

private boolean processCommand(String command) {
            if (command.trim().startsWith("#")) {
                // ignore comment
                return true;
            }
            LOGGER.info("Processing command:'{}' ({} left)", () -> command, commands::size);
            setCurrentCommand(command);
            long start = System.currentTimeMillis();
            MCRTransactionManager.beginTransactions();
            List<String> commandsReturned = null;
            try {
                for (List<MCRCommand> cmds : knownCommands.values()) {
                    commandsReturned = runCommand(command, cmds);
                    if (commandsReturned != null) {
                        break;
                    }
                }
                if (commandsReturned == null) {
                    LOGGER.warn("Command not understood: {}", command);
                    rollbackAndContinue(command);
                    return false;
                }
                updateKnownCommandsIfNeeded();
                MCRTransactionManager.commitTransactions();
                LOGGER.info("Command processed ({} ms)", () -> System.currentTimeMillis() - start);
            } catch (Exception ex) {
                LOGGER.error("Command '{}' failed. Performing transaction rollback...", command, ex);
                rollbackAndContinue(command);
                return false;
            } finally {
                // Resetting the entity manager
                MCRTransactionManager.beginTransactions();
                MCREntityManagerProvider.getCurrentEntityManager().clear();
                MCRTransactionManager.commitTransactions();
            }
            return true;
        }

        private void rollbackAndContinue(String command) {
            try {
                MCRTransactionManager.rollbackTransactions();
            } catch (Exception exception) {
                LOGGER.error("Error while performing rollback for command '{}'!", command, exception);
            }
            if (!continueIfOneFails) {
                saveQueue(command, null);
            }
        }

[Mewel]

simplified:

public static synchronized MCRFileUploadBucket createBucket(String bucketID,
        Map<String, List<String>> parameters,
        MCRUploadHandler uploadHandler) throws MCRUploadServerException {
        MCRFileUploadBucket bucket = BUCKET_MAP.get(bucketID);
        if (bucket == null) {
            bucket = new MCRFileUploadBucket(bucketID, parameters, uploadHandler);
            BUCKET_MAP.put(bucketID, bucket);
        }
        return bucket;
    }

[kkrebs]

Due to the high merge effort in current feature branches, we have decided to defer the PMD-PullRequests until the feature branches have been merged.
In addition, all PMD-PRs that are still open will go against the main branch. Branch 2024.06.x is only to be stabilised. So could you please rebase this branch to main?! Thanks!