Skip to content
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

Cleanup for FindBugs and PMD warnings in -simple and -jena #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cleanup for FindBugs and PMD warnings in -simple and -jena #49

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: 2 additions & 2 deletions commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/JenaGraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public interface JenaGraph extends org.apache.commons.rdf.api.Graph {
*
* @return A Jena {@link org.apache.jena.graph.Graph}
*/
public org.apache.jena.graph.Graph asJenaGraph();
org.apache.jena.graph.Graph asJenaGraph();

/**
* Return the graph as a Jena {@link Model}.
Expand All @@ -52,5 +52,5 @@ public interface JenaGraph extends org.apache.commons.rdf.api.Graph {
*
* @return A Jena {@link Model}
*/
public Model asJenaModel();
Model asJenaModel();
}
2 changes: 1 addition & 1 deletion commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/JenaQuadLike.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,5 @@ public interface JenaQuadLike<G extends RDFTerm> extends JenaTripleLike, QuadLik
*
* @return Adapted Jena {@link Quad}.
*/
public Quad asJenaQuad();
Quad asJenaQuad();
}
2 changes: 1 addition & 1 deletion commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/JenaRDFTerm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,5 @@
* @see JenaBlankNode
*/
public interface JenaRDFTerm extends RDFTerm {
public Node asJenaNode();
Node asJenaNode();
}
2 changes: 1 addition & 1 deletion commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/JenaTripleLike.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* @see JenaGeneralizedQuadLike
*
*/
public interface JenaTripleLike extends org.apache.commons.rdf.api.TripleLike {
public interface JenaTripleLike extends TripleLike {

/**
* Return the adapted Jena triple
Expand Down
1 change: 1 addition & 0 deletions commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/impl/AbstractQuadLike.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
* RDFTerm)
*
*/
@SuppressWarnings("PMD.UnnecessaryFullyQualifiedName") // we use fully-qualified names for clarity
abstract class AbstractQuadLike<S extends RDFTerm, P extends RDFTerm, O extends RDFTerm, G extends RDFTerm>
implements JenaQuadLike<G> {

Expand Down
6 changes: 4 additions & 2 deletions commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/impl/InternalJenaFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,13 @@ public JenaRDFTerm createRDFTerm(final Node node, final UUID salt) throws Conver
}
if (node.equals(Node.ANY)) {
// NOTE: JenaAny no longer supported by Commons RDF
// return JenaAnyImpl.Singleton.instance;
//TODO remove this check or handle JenaAny
throw new ConversionException("Unrecognized node type: " + node);
}
if (node.isVariable()) {
// NOTE: JenaVariable no longer supported by Commons RDF
// return new JenaVariableImpl(node);
//TODO remove this check or handle JenaVariable
throw new ConversionException("Unrecognized node type: " + node);
}
throw new ConversionException("Unrecognized node type: " + node);
}
Expand Down
18 changes: 10 additions & 8 deletions commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/impl/JenaGraphImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class JenaGraphImpl implements JenaGraph {
private final org.apache.jena.graph.Graph graph;
private final UUID salt;
private final transient JenaRDF factory;
private Model model;
private volatile Model model;

JenaGraphImpl(final org.apache.jena.graph.Graph graph, final UUID salt) {
this.graph = graph;
Expand Down Expand Up @@ -107,7 +107,7 @@ private Node toJenaPattern(final RDFTerm pattern) {

@Override
public void remove(final Triple triple) {
if ((triple.getObject() instanceof Literal) &&
if (triple.getObject() instanceof Literal &&
((Literal) triple.getObject()).getLanguageTag().isPresent()) {
// COMMONSRDF-51: graph.delete(Triple) would be too restrictive
// as it won't delete triples with different lang tag - so
Expand Down Expand Up @@ -154,17 +154,19 @@ public String toString() {

@Override
public Model asJenaModel() {
if (model == null) {
// use a local holder to avoid accessing the volatile field model more than needed
Model check = model;
if (check == null) {
synchronized (this) {
// As Model can be used for locks, we should make sure we don't
// make
// more than one model
if (model == null) {
model = ModelFactory.createModelForGraph(graph);
// make more than one model
check = model;
if (check == null) {
model = check = ModelFactory.createModelForGraph(graph);
}
}
}
return model;
return check;
}

}
2 changes: 1 addition & 1 deletion commons-rdf-jena/src/main/java/org/apache/commons/rdf/jena/impl/JenaQuadImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class JenaQuadImpl extends AbstractQuadLike<BlankNodeOrIRI, IRI, RDFTerm, BlankN
JenaQuadImpl(final org.apache.jena.sparql.core.Quad quad, final UUID salt) {
super(quad, salt);
// Check the conversion
if ((graphName.isPresent() && !(graphName.get() instanceof BlankNodeOrIRI))
if (graphName.isPresent() && !(graphName.get() instanceof BlankNodeOrIRI)
|| !(subject instanceof BlankNodeOrIRI) || !(predicate instanceof IRI)
|| !(object instanceof RDFTerm)) {
throw new ConversionException("Can't adapt generalized quad: " + quad);
Expand Down
2 changes: 1 addition & 1 deletion commons-rdf-simple/src/main/java/org/apache/commons/rdf/simple/SimpleRDF.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class SimpleRDF implements RDF {
* This method is package protected to avoid any third-party subclasses.
*
*/
static interface SimpleRDFTerm extends RDFTerm {
interface SimpleRDFTerm extends RDFTerm {
}

/**
Expand Down
12 changes: 9 additions & 3 deletions ...df-simple/src/main/java/org/apache/commons/rdf/simple/experimental/AbstractRDFParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@
*/
package org.apache.commons.rdf.simple.experimental;

import static java.util.concurrent.Executors.newCachedThreadPool;

import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Optional;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;

import org.apache.commons.rdf.api.Dataset;
Expand Down Expand Up @@ -58,8 +60,12 @@
*/
public abstract class AbstractRDFParser<T extends AbstractRDFParser<T>> implements RDFParser, Cloneable {

public static final ThreadGroup threadGroup = new ThreadGroup("Commons RDF parsers");
private static final ExecutorService threadpool = Executors.newCachedThreadPool(r -> new Thread(threadGroup, r));
public static final AtomicInteger threadCount = new AtomicInteger();
private static Thread newThread(Runnable r) {
Copy link
Member

Choose a reason for hiding this comment

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

I disagree on this style change, the previous code was much cleaner. Why is a method-reference on 4 lines that is only used once better than a very small lambda?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because either FindBugs or PMD (can't remember) which called out ThreadGroup as not threadsafe, which is correct as far as I know. I am in no way wedded to this change and if we can guarantee thread-safety from outside the class (or simply document it as not threadsafe) I would be happy to pull this change out.

Copy link
Member

Choose a reason for hiding this comment

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

A final ThreadGroup that is not thread safe.. The whole purpose of it is to group threads?

It is true that they should not be used for security purpose, but that is not why it is here. The ThreadGroup is mainly useful for debugging as people might see these "Commons RDF Parser" tree grouped together in the debugger (e.g. in Eclipse) rather than the generic no-name you get from the ExecutorService.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why I put the name "Commons RDF Parser " back in. But like I said, I am fine with whatever- I was just tearing through PMD warnings at speed after the comment (I think to the board report) about there being a lot of them. Shall I take your comment as a direction to remove this change? Happy to...

Copy link
Member

Choose a reason for hiding this comment

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

OK, so I see ThreadGroup is criticized by Joshua Block, but I don't think we would get many of these threads in the pool, so no big need to group them. However we don't need that counter of the threadCount, which also don't need to be public, they can just be called Commons RDF Parser without a number.

return new Thread(r, "Commons RDF Parser " + threadCount.getAndIncrement());
}

private static final ExecutorService threadpool = newCachedThreadPool(AbstractRDFParser::newThread);

// Basically only used for creating IRIs
private static RDF internalRdfTermFactory = new SimpleRDF();
Expand Down