Skip to content

MCR-3258 Enable PMD rule ExceptionAsFlowControl #2480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: 2024.06.x
Choose a base branch
from
Draft

MCR-3258 Enable PMD rule ExceptionAsFlowControl #2480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion mycore-base/src/main/java/org/mycore/access/facts/MCRFactsAccessSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,9 @@ private MCRCondition buildRulesFromXML() {
if (configFile != null) {
content.sendTo(configFile);
} else {
throw new IOException("MCRConfigurationDir is not available!");
LOGGER.error(() -> "MCRConfigurationDir is not availabel to write file + " + RESOLVED_RULES_FILE_NAME);
LOGGER.info(() -> "Rules file is: \n"
+ new XMLOutputter(Format.getPrettyFormat()).outputString(eRules));
}
} catch (IOException e) {
LOGGER.error(() -> "Could not write file '" + RESOLVED_RULES_FILE_NAME + "' to config directory", e);
Expand Down
7 changes: 3 additions & 4 deletions mycore-base/src/main/java/org/mycore/common/xml/MCRURIResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ public void reinitialize() {
LOGGER.error("Unable to initialize MCRURIResolver", exc);
}
}

/**
* @deprecated Use {@link MCRURIResolver#reinitialize()} instead
*/
Expand Down Expand Up @@ -198,7 +198,7 @@ public static MCRURIResolver instance() {
public static MCRURIResolver obtainInstance() {
return SHARED_INSTANCE;
}

/**
* Initializes the MCRURIResolver for servlet applications.
*
Expand Down Expand Up @@ -579,12 +579,11 @@ public Source resolve(String href, String base) throws TransformerException {
URL resource = MCRResourceHelper.getWebResourceUrl(path);
if (resource != null) {
return new StreamSource(resource.toURI().toASCIIString());
} else {
throw new TransformerException("Could not find web resource: " + path);
}
} catch (Exception ex) {
throw new TransformerException("Could not load web resource: " + path, ex);
}
throw new TransformerException("Could not find web resource: " + path);
}
}

Expand Down
1 change: 1 addition & 0 deletions mycore-base/src/main/java/org/mycore/common/xsl/MCRXSLInfoServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ private void findInConfigResourcesDir(File configDir) throws IOException {
configDir.toPath().relativize(file).toString()));
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }

private void findInConfigLibDir(File configDir) throws IOException {
final Path lib = configDir.toPath().resolve("lib");
try {
Expand Down
25 changes: 15 additions & 10 deletions mycore-base/src/main/java/org/mycore/datamodel/common/MCRDefaultXMLMetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.mycore.datamodel.common;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -221,14 +220,15 @@ private synchronized void closeCreatedStores() {
private void checkPath(Path path, String type) {
if (!Files.exists(path)) {
try {
if (!Files.exists(Files.createDirectories(path))) {
throw new MCRConfigurationException(
"The metadata store " + type + " directory " + path.toAbsolutePath() + " does not exist.");
}
Files.createDirectories(path);
} catch (Exception ex) {
String msg = "Exception while creating metadata store " + type + " directory " + path.toAbsolutePath();
throw new MCRConfigurationException(msg, ex);
}
if (!Files.exists(path)) {
throw new MCRConfigurationException(
"The metadata store " + type + " directory " + path.toAbsolutePath() + " does not exist.");
}
} else {
if (!Files.isDirectory(path)) {
throw new MCRConfigurationException(
Expand Down Expand Up @@ -358,8 +358,8 @@ private void setupStore(String project, String objectType, String configPrefix,
}
MCRConfiguration2.set(configPrefix + "BaseDir", typePath.toAbsolutePath().toString());
MCRConfiguration2.set(configPrefix + "ForceXML", String.valueOf(true));
String value = Objects.equals(objectType, MCRDerivate.OBJECT_TYPE) ?
MCRDerivate.ROOT_NAME : MCRObject.ROOT_NAME;
String value =
Objects.equals(objectType, MCRDerivate.OBJECT_TYPE) ? MCRDerivate.ROOT_NAME : MCRObject.ROOT_NAME;
MCRConfiguration2.set(configPrefix + "ForceDocType", value);
createdStores.add(baseID);
MCRStoreManager.createStore(baseID, clazz);
Expand All @@ -377,15 +377,20 @@ private void checkAndCreateDirectory(Path path, String project, String objectTyp
path.toAbsolutePath(), project, objectType, configPrefix));
}
try {
if (!Files.exists(Files.createDirectories(path))) {
throw new FileNotFoundException(path.toAbsolutePath() + " does not exists.");
}
Files.createDirectories(path);
} catch (Exception e) {
throw new MCRPersistenceException(String.format(Locale.ENGLISH,
"Couldn'e create directory ''%s'' to set up store for project ''%s'' and objectType ''%s'' "
+ "and config prefix ''%s''",
path.toAbsolutePath(), project, objectType, configPrefix), e);
}
if (!Files.exists(path)) {
throw new MCRPersistenceException(String.format(Locale.ENGLISH,
"Path does not exists ''%s'' to set up store for project ''%s'' and objectType ''%s'' "
+ "and config prefix ''%s''",
path.toAbsolutePath(), project, objectType, configPrefix));
}

}

private String getStoryKey(String project, String objectType) {
Expand Down
2 changes: 1 addition & 1 deletion mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRDirectory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ protected MCRStoredNode buildChildNode(Path fo) {
* Repairs additional metadata of this directory and all its children
*/
@Override
@SuppressWarnings("unchecked")
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
                }
            }
        }

void repairMetadata() throws IOException {
writeData(e -> {
e.setName("dir");
Expand Down
1 change: 1 addition & 0 deletions mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRFileCollection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ private void readAdditionalData() throws IOException {
}
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

protected void saveAdditionalData() throws IOException {
Path target = path.resolve(DATA_FILE);
try {
Expand Down
2 changes: 2 additions & 0 deletions mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRIFS2Commands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ public static List<String> writeMD5SumsToDirectory(String directory) throws NotD
.collect(Collectors.toList());
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }));
            }
        }
    }

@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 {
Expand Down Expand Up @@ -167,6 +168,7 @@ public static void writeMD5SumFile(String targetFile, String ifsStoreId) throws
}
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")));
            });
    }

private static Stream<FileInfo> getAllFiles(Path nodePath, Element node)
throws IOException {
Function<Element, String> getName = e -> e.getAttributeValue("name");
Expand Down
20 changes: 10 additions & 10 deletions mycore-base/src/main/java/org/mycore/datamodel/metadata/MCRMetaDerivateLink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,20 +167,20 @@ public MCRPath getLinkedFile() throws URISyntaxException {
@Override
public void validate() throws MCRException {
super.validate();
try {
MCRPath linkedFile = getLinkedFile();

if (linkedFile == null) {
throw new MCRException(getSubTag() + ": linked file is null");
}

if (!Files.exists(linkedFile)) {
LOGGER.warn("{}: File not found: {}", this::getSubTag, () -> super.href);
}

MCRPath linkedFile;
try {
linkedFile = getLinkedFile();
} catch (Exception exc) {
throw new MCRException(getSubTag() + ": Error while getting linked file " + super.href, exc);
}

if (linkedFile == null) {
throw new MCRException(getSubTag() + ": linked file is null");
}
if (!Files.exists(linkedFile)) {
LOGGER.warn("{}: File not found: {}", this::getSubTag, () -> super.href);
}
}

@Override
Expand Down
1 change: 1 addition & 0 deletions mycore-ifs/src/main/java/org/mycore/datamodel/niofs/ifs2/MCRFileSystemUtils.java
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
  1. 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)) {
  1. 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;
    }

Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
/**
* @author Thomas Scheffler
*/
@SuppressWarnings("PMD.ExceptionAsFlowControl")
final class MCRFileSystemUtils {

private static final Logger LOGGER = LogManager.getLogger();
Expand Down
1 change: 1 addition & 0 deletions mycore-iview2/src/main/java/org/mycore/iview2/services/MCRIView2Tools.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ public static boolean isDerivateSupported(String derivateID) {
* @return if content type is in property <code>MCR.Module-iview2.SupportedContentTypes</code>
* @see MCRContentTypes#probeContentType(Path)
*/
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
    }

public static boolean isFileSupported(Path file) throws IOException {
try {
return Optional.ofNullable(file)
Expand Down
1 change: 1 addition & 0 deletions mycore-iview2/src/main/java/org/mycore/iview2/services/MCRTilingAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public MCRTilingAction(MCRTileJob image) {
* Also this updates tileJob properties of {@link MCRTileJob} in the database.
*/
@Override
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

public void run() {
tileJob.setStart(new Date());
MCRImage image;
Expand Down
1 change: 1 addition & 0 deletions mycore-ocfl/src/main/java/org/mycore/ocfl/metadata/MCROCFLXMLMetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ public Collection<String> getObjectBaseIds() {
}

@Override
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
    }

public List<MCRObjectIDDate> retrieveObjectDates(List<String> ids) throws IOException {
try {
return ids.stream()
Expand Down
1 change: 1 addition & 0 deletions mycore-orcid/src/main/java/org/mycore/orcid/resources/MCRORCIDResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public String getPublicationStatus(@PathParam("objectID") String objectID)
@GET
@Path("publish/{objectID}")
@Produces(MediaType.APPLICATION_JSON)
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} catch (WebApplicationException webApplicationException) {
            throw webApplicationException;
        }

public String publish(@PathParam("objectID") String objectID) {
MCRObjectID oid = checkID(objectID);
MCRORCIDUser user = MCRORCIDSession.getCurrentUser();
Expand Down
1 change: 1 addition & 0 deletions mycore-orcid2/src/main/java/org/mycore/orcid2/v3/work/MCRORCIDWorkService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public MCRORCIDWorkService(String orcid, MCRORCIDCredential credential) {
* @param object the MCRObject
* @throws MCRORCIDException if cannot create work, transformation fails or cannot update flags
*/
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here would rethrow and maybe update the exception a bit:

    ```

} catch (MCRORCIDWorkAlreadyExistsException workAlreadyExistsException) {
throw workAlreadyExistsException;
} catch (Exception e) {
throw new MCRORCIDException("Cannot create work for object " + object.getId(), e);
}

public void createWork(MCRObject object) {
if (!MCRORCIDUtils.checkPublishState(object)) {
throw new MCRORCIDException("Object has wrong state");
Expand Down
1 change: 1 addition & 0 deletions mycore-pandoc/src/main/java/org/mycore/pandoc/MCRPandocAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ private static String convertFormatToPath(Action action, String format) {
}
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just throw this

 throw new MCRPandocException("Pandoc process " + String.join(" ", args) +
                    " timed out after " + TIMEOUT + " seconds");

instead of the InterruptedException

private static byte[] callPandoc(String[] args, byte[] input) {
class ThreadWrapper implements Runnable {

Expand Down
2 changes: 2 additions & 0 deletions mycore-pi/src/main/java/org/mycore/pi/MCRPIXPathMetadataService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@
import org.mycore.pi.exceptions.MCRPersistentIdentifierException;

public class MCRPIXPathMetadataService extends MCRPIMetadataService<MCRPersistentIdentifier> {

@Override
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the "check" is done way to late. It can be done at the start of the method. Also the method should throw an MCRPersistentIdentifierException.

@Override
    public void insertIdentifier(MCRPersistentIdentifier identifier, MCRBase obj, String additional) throws MCRPersistentIdentifierException {
        if (!(obj instanceof MCRObject object)) {
            throw new MCRPersistentIdentifierException(obj.getId() + " is no MCRObject!",
                new OperationNotSupportedException(getClass().getName() + " only supports "
                    + MCRObject.class.getName() + "!"));
        }
        String xpath = getProperties().get("Xpath");
        Document xml = obj.createXML();
        MCRNodeBuilder nb = new MCRNodeBuilder();
        try {
            nb.buildElement(xpath, identifier.asString(), xml);
            final Element metadata = xml.getRootElement().getChild("metadata");
            object.getMetadata().setFromDOM(metadata);
        } catch (Exception e) {
            throw new MCRException("Error while inscribing PI to " + obj.getId(), e);
        }
    }

public void insertIdentifier(MCRPersistentIdentifier identifier, MCRBase obj, String additional) {
String xpath = getProperties().get("Xpath");
Document xml = obj.createXML();
Expand Down
1 change: 1 addition & 0 deletions mycore-restapi/src/main/java/org/mycore/restapi/v2/MCRRestDerivateContents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ private boolean isFile() {
&& (headers.containsKey(HttpHeaders.CONTENT_LENGTH) || headers.containsKey("Transfer-Encoding"));
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what todo here, this method does to much. @yagee-de can you look at this?

private Response serveDirectory(MCRPath mcrPath, MCRFileAttributes dirAttrs) {
Directory dir = new Directory(mcrPath, dirAttrs);
try (DirectoryStream ds = Files.newDirectoryStream(mcrPath)) {
Expand Down
1 change: 1 addition & 0 deletions mycore-restapi/src/main/java/org/mycore/restapi/v2/MCRRestObjects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ public Response getThumbnail(@PathParam(PARAM_MCRID) String id, @PathParam("ext"
return getThumbnail(id, defaultSize, ext);
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private Response getThumbnail(String id, int size, String ext) {
List<MCRPath> mainDocs = MCRMetadataManager.getDerivateIds(MCRObjectID.getInstance(id), 1, TimeUnit.MINUTES)
.stream()
Expand Down
1 change: 1 addition & 0 deletions mycore-sass/src/main/java/org/mycore/sass/MCRSassCompiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ private void handleCanonicalizeRequest(int compilationId, CanonicalizeRequest ca
connection.sendMessage(compilationId, ProtocolUtil.inboundMessage(canonicalizeResponse.build()));
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));
   }

private void handleFunctionCallRequest(int compilationId, FunctionCallRequest functionCallRequest)
throws IOException {
FunctionCallResponse.Builder response = FunctionCallResponse.newBuilder()
Expand Down
13 changes: 7 additions & 6 deletions mycore-solr/src/main/java/org/mycore/solr/cloud/configsets/MCRSolrConfigSetHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,17 @@ public static Map<String, MCRSolrConfigSetProvider> getLocalConfigSets() {
Map<String, MCRSolrConfigSetProvider> configSets = new HashMap<>(instances.size());

instances.forEach((name, supplier) -> {
Object cs;
try {
Object cs = supplier.call();
if (cs instanceof MCRSolrConfigSetProvider configSet) {
configSets.put(name, configSet);
} else {
throw new MCRConfigurationException("Invalid config set instance " + name);
}
cs = supplier.call();
} catch (Exception e) {
throw new MCRConfigurationException("Error while initializing config set " + name, e);
}
if (cs instanceof MCRSolrConfigSetProvider configSet) {
configSets.put(name, configSet);
} else {
throw new MCRConfigurationException("Invalid config set instance " + name);
}
});

return configSets;
Expand Down
1 change: 1 addition & 0 deletions mycore-sword/src/main/java/org/mycore/sword/MCRSwordContainerHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public DepositReceipt getMetadata(String collectionString, MCRObject object, Opt
return MCRSword.getCollection(collectionString).getMetadataProvider().provideMetadata(object);
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
        }
    }

public void deleteObject(MCRObject object) throws SwordServerException {
try {
object
Expand Down
1 change: 1 addition & 0 deletions mycore-webcli/src/main/java/org/mycore/webcli/container/MCRWebCLIContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ public Boolean call() throws Exception {
*
* @return true if command processed successfully
*/
@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
            }
        }

private boolean processCommand(String command) {
if (command.trim().startsWith("#")) {
// ignore comment
Expand Down
1 change: 1 addition & 0 deletions mycore-webtools/src/main/java/org/mycore/webtools/upload/MCRFileUploadBucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public static MCRFileUploadBucket getBucket(String bucketID) {
return BUCKET_MAP.get(bucketID);
}

@SuppressWarnings("PMD.ExceptionAsFlowControl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }

public static synchronized MCRFileUploadBucket createBucket(String bucketID,
Map<String, List<String>> parameters,
MCRUploadHandler uploadHandler) throws MCRUploadServerException {
Expand Down
3 changes: 0 additions & 3 deletions ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,7 @@
</rule>
-->
<rule ref="category/java/design.xml/DoNotExtendJavaLangError"/>
<!-- TODO: Create PR to fix this rule -->
<!--
<rule ref="category/java/design.xml/ExceptionAsFlowControl"/>
-->
<rule ref="category/java/design.xml/ExcessiveParameterList"/>
<rule ref="category/java/design.xml/FinalFieldCouldBeStatic"/>
<rule ref="category/java/design.xml/InvalidJavaBean"/>
Expand Down
Loading