You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jena.apache.org by afs <gi...@git.apache.org> on 2018/08/17 09:32:52 UTC
[GitHub] jena pull request #463: Cleanup
GitHub user afs opened a pull request:
https://github.com/apache/jena/pull/463
Cleanup
This is a collection of small items, accumulated while working with the source code attached to a project.
* Various comments/javadoc added and fixed and other code cleanup.
* make subclassing of the Fuseki query servlet better
* building Fuseki embedded using the command line syntax
* deprecate TDB ops "isBackedBy" in favour of "isTDB" for better naming.
I've left it as each individual commit for the details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/afs/jena cleanup
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/jena/pull/463.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #463
----
commit 8d0c1310a89c115553889208c97cbdfdfb1958b2
Author: Andy Seaborne <an...@...>
Date: 2018-08-14T12:02:38Z
Typos and comments.
commit d061885fae1ca4ba86e0603e7b7a45d21c739802
Author: Andy Seaborne <an...@...>
Date: 2018-08-14T12:12:23Z
Expose the command line argument processing to build a FusekiServer.
commit 0335b26f11825843f17053501a07c185f28571a5
Author: Andy Seaborne <an...@...>
Date: 2018-08-14T12:13:46Z
Deprecate isBackedByTDB in favour of isTDB2.
commit d576e607e6f578a57f7049a4e2dc6ad81d707ebc
Author: Andy Seaborne <an...@...>
Date: 2018-08-15T14:42:48Z
Deprecate isBackedByTDB in favour of isTDB1
commit 833cdb686b6035fd63afbfb0b92bfe7f9c77e493
Author: Andy Seaborne <an...@...>
Date: 2018-08-15T15:54:56Z
Fix javadoc.
commit 8206b7c6cd57868a6aba4021647c10b221986cc1
Author: Andy Seaborne <an...@...>
Date: 2018-08-15T15:55:53Z
Provision for custom parameters so request validation does not WARN.
commit caf572a1081bb4ab84337c7553ff0181a5052c8b
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T08:56:25Z
Convert to single format.
commit cb6d9b5c63e0cfcb5bb600c4e5af9948c77b41ec
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T08:56:48Z
Strings to Node_URIs.
commit 21050fc9300cc29eda22744c35466aae9f5b01c0
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T11:03:08Z
Put attribute names constants in Fuseki.
commit f6bac07b64c500472091efb7a83ca486b89e321e
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T11:03:59Z
Switch naming of setters to non-"set" style.
commit 99180e5831bb8d0ff94c0e98fd3a375f02c20af5
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T15:11:59Z
Support for ServletFilters.
commit ae74347f3607427e88561c854f26903cc880f310
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T15:12:44Z
Comments and internal rename.
commit aa3518ee48d53a4bcd4d63c66b36cb8e0f3a93bd
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T15:13:05Z
Remove unused and out-of-date class.
commit 9dace245d39a7c1a977fa34e423502c5ed39afe9
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T22:21:49Z
Better attribute naming - org.apache.jena.fuseki:<name>
commit 3b153d526d18626e6ad3ea4414d1dc8f138213ba
Author: Andy Seaborne <an...@...>
Date: 2018-08-16T22:22:07Z
Typos
----
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210855919
--- Diff: jena-arq/src/main/java/org/apache/jena/sparql/util/NodeUtils.java ---
@@ -105,15 +102,23 @@ public String accept(Node x) {
Iterator<String> conv = new MapFilterIterator<>(mapper, eIter) ;
return conv ;
}
-
- /** Convert IRI String to Node */
- public static Set<Node> convertToNodes(Collection<String> uris) {
- Set<Node> nodes = new HashSet<>() ;
- for ( String x : uris )
- nodes.add(NodeFactory.createURI(x)) ;
- return nodes ;
+
--- End diff --
Nit-pick! Some spaces were left behind.
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210855762
--- Diff: jena-arq/src/main/java/org/apache/jena/sparql/util/NodeUtils.java ---
@@ -18,12 +18,9 @@
package org.apache.jena.sparql.util;
-import java.util.Collection ;
-import java.util.HashSet ;
-import java.util.Iterator ;
-import java.util.Objects;
-import java.util.Set ;
+import java.util.*;
--- End diff --
Isn't it a bit better to keep what we have in import statements, expanding to what's used instead of importing the whole package?
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210857731
--- Diff: jena-fuseki2/jena-fuseki-embedded/src/main/java/org/apache/jena/fuseki/embedded/FusekiServer.java ---
@@ -20,10 +20,9 @@
import static java.util.Objects.requireNonNull;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Objects;
+import java.util.*;
--- End diff --
Ditto first comment, about maintaining the current standard in the code (I think)
---
[GitHub] jena pull request #463: Cleanup
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210891477
--- Diff: jena-fuseki2/jena-fuseki-embedded/src/main/java/org/apache/jena/fuseki/embedded/FusekiServer.java ---
@@ -455,30 +527,36 @@ private static void setMimeTypes(ServletContextHandler context) {
private void servlets(ServletContextHandler context) {
// Fuseki dataset services filter
+ // This goes as the filter at the end of any filter chaining.
FusekiFilter ff = new FusekiFilter();
- FilterHolder h = new FilterHolder(ff);
- context.addFilter(h, "/*", null);
-
- other.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ addFilter(context, "/*", ff);
if ( withStats )
addServlet(context, "/$/stats", new ActionStats());
if ( withPing )
addServlet(context, "/$/ping", new ActionPing());
+ servlets.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ filters.forEach (p-> addFilter(context, p.getLeft(), p.getRight()));
+
if ( staticContentDir != null ) {
DefaultServlet staticServlet = new DefaultServlet();
ServletHolder staticContent = new ServletHolder(staticServlet);
staticContent.setInitParameter("resourceBase", staticContentDir);
context.addServlet(staticContent, "/");
}
}
-
+
--- End diff --
There is a tradeoff here - simply turning on the save action to remove trailing spaces is likely to cause such formatting changes on files with functional changes. Previously, keeping formatting changes separate from functional changes has been requested. (I'm neutral.)
---
[GitHub] jena pull request #463: Cleanup
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/jena/pull/463
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210857986
--- Diff: jena-fuseki2/jena-fuseki-embedded/src/main/java/org/apache/jena/fuseki/embedded/FusekiServer.java ---
@@ -455,30 +527,36 @@ private static void setMimeTypes(ServletContextHandler context) {
private void servlets(ServletContextHandler context) {
// Fuseki dataset services filter
+ // This goes as the filter at the end of any filter chaining.
FusekiFilter ff = new FusekiFilter();
- FilterHolder h = new FilterHolder(ff);
- context.addFilter(h, "/*", null);
-
- other.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ addFilter(context, "/*", ff);
if ( withStats )
addServlet(context, "/$/stats", new ActionStats());
if ( withPing )
addServlet(context, "/$/ping", new ActionPing());
+ servlets.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ filters.forEach (p-> addFilter(context, p.getLeft(), p.getRight()));
+
if ( staticContentDir != null ) {
DefaultServlet staticServlet = new DefaultServlet();
ServletHolder staticContent = new ServletHolder(staticServlet);
staticContent.setInitParameter("resourceBase", staticContentDir);
context.addServlet(staticContent, "/");
}
}
-
+
--- End diff --
Nit-picking again! Unnecessary spaces...
---
[GitHub] jena pull request #463: Cleanup
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210889580
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/ListUtils.java ---
@@ -34,49 +34,40 @@
{
private ListUtils() {}
- public static <T>
- List<T> unique(List<T> list)
- {
- return toList(list.stream().distinct()) ;
+ public static <T> List<T> unique(List<T> list) {
+ return toList(list.stream().distinct());
}
-
- public static
- List<Integer> asList(int... values)
- {
- List<Integer> x = new ArrayList<>(values.length) ;
+
+ public static List<Integer> asList(int...values) {
--- End diff --
The signature of `Arrays.asList` is `List<T> asList(T... a)` and auto-boxing does not work when the argument is `int[]` or `int ...`).
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210857578
--- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/ServiceRouter.java ---
@@ -120,10 +120,12 @@ protected boolean allowQuads_RW(HttpAction action) {
return isEnabled(action, Operation.Quads_RW);
}
- // Test whether there is a configuration that allows this action as the operation
- // given.
- // Ignores the operation in the action (set due to parsing - it might be "quads"
- // which is the generic operation when just the dataset is specificed.
+ /**
+ * Test whether there is a configuration that allows this action as the operation
+ * given. Ignores the operation in the action which is set due to parsing - it
+ * might be "quads" which is the generic operation when just the dataset is
+ * specificed.
--- End diff --
s/specificed/specified
---
[GitHub] jena pull request #463: Cleanup
Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r211191248
--- Diff: jena-arq/src/main/java/org/apache/jena/sparql/core/DynamicDatasets.java ---
@@ -56,8 +55,8 @@ public static DatasetGraph dynamicDataset(DatasetDescription description, Datase
if (description.isEmpty() )
return dsg;
- Set<Node> defaultGraphs = NodeUtils.convertToNodes(description.getDefaultGraphURIs()) ;
- Set<Node> namedGraphs = NodeUtils.convertToNodes(description.getNamedGraphURIs()) ;
+ Collection<Node> defaultGraphs = NodeUtils.convertToNodes(description.getDefaultGraphURIs()) ;
--- End diff --
Isn't the point of using sets here to remove duplicates or is that now done in the `NodeUtils.convertToNodes()` method?
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210893804
--- Diff: jena-fuseki2/jena-fuseki-embedded/src/main/java/org/apache/jena/fuseki/embedded/FusekiServer.java ---
@@ -455,30 +527,36 @@ private static void setMimeTypes(ServletContextHandler context) {
private void servlets(ServletContextHandler context) {
// Fuseki dataset services filter
+ // This goes as the filter at the end of any filter chaining.
FusekiFilter ff = new FusekiFilter();
- FilterHolder h = new FilterHolder(ff);
- context.addFilter(h, "/*", null);
-
- other.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ addFilter(context, "/*", ff);
if ( withStats )
addServlet(context, "/$/stats", new ActionStats());
if ( withPing )
addServlet(context, "/$/ping", new ActionPing());
+ servlets.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ filters.forEach (p-> addFilter(context, p.getLeft(), p.getRight()));
+
if ( staticContentDir != null ) {
DefaultServlet staticServlet = new DefaultServlet();
ServletHolder staticContent = new ServletHolder(staticServlet);
staticContent.setInitParameter("resourceBase", staticContentDir);
context.addServlet(staticContent, "/");
}
}
-
+
--- End diff --
>simply turning on the save action to remove trailing spaces is likely to cause such formatting changes on files with functional changes.
Agreed, I used the save action in Eclipse for a while and regretted later.
>Previously, keeping formatting changes separate from functional changes has been requested. (I'm neutral.)
I'm ok with either too. But my point here was that this case wasn't like the save action... at least from looking at the github diff, it appears there was no spaces before, and these were added in this commit only. But as I said, nit-pick. And nit-picks like this one can cause distraction, so feel free to ignore or hide :-)
---
[GitHub] jena pull request #463: Cleanup
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r211266636
--- Diff: jena-arq/src/main/java/org/apache/jena/sparql/core/DynamicDatasets.java ---
@@ -56,8 +55,8 @@ public static DatasetGraph dynamicDataset(DatasetDescription description, Datase
if (description.isEmpty() )
return dsg;
- Set<Node> defaultGraphs = NodeUtils.convertToNodes(description.getDefaultGraphURIs()) ;
- Set<Node> namedGraphs = NodeUtils.convertToNodes(description.getNamedGraphURIs()) ;
+ Collection<Node> defaultGraphs = NodeUtils.convertToNodes(description.getDefaultGraphURIs()) ;
--- End diff --
There are various places duplicate removal may happen but it is confusing and with all the routes may be depending on here. Better to maintain compatibility.
PR #464 restores the explicit behaviour by having "to list" and "to set" versions and a deprecated "NodeUtils.convertToNodes" with the Set semantics.
Does that look good?
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210856558
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/ListUtils.java ---
@@ -34,49 +34,40 @@
{
private ListUtils() {}
- public static <T>
- List<T> unique(List<T> list)
- {
- return toList(list.stream().distinct()) ;
+ public static <T> List<T> unique(List<T> list) {
+ return toList(list.stream().distinct());
}
-
- public static
- List<Integer> asList(int... values)
- {
- List<Integer> x = new ArrayList<>(values.length) ;
+
+ public static List<Integer> asList(int...values) {
--- End diff --
Couldn't we replace this method by something like `Arrays.asList(1, 2, 3);` ? Or maybe deprecate it?
---
[GitHub] jena pull request #463: Cleanup
Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210921706
--- Diff: jena-fuseki2/jena-fuseki-embedded/src/main/java/org/apache/jena/fuseki/embedded/FusekiServer.java ---
@@ -455,30 +527,36 @@ private static void setMimeTypes(ServletContextHandler context) {
private void servlets(ServletContextHandler context) {
// Fuseki dataset services filter
+ // This goes as the filter at the end of any filter chaining.
FusekiFilter ff = new FusekiFilter();
- FilterHolder h = new FilterHolder(ff);
- context.addFilter(h, "/*", null);
-
- other.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ addFilter(context, "/*", ff);
if ( withStats )
addServlet(context, "/$/stats", new ActionStats());
if ( withPing )
addServlet(context, "/$/ping", new ActionPing());
+ servlets.forEach(p->addServlet(context, p.getLeft(), p.getRight()));
+ filters.forEach (p-> addFilter(context, p.getLeft(), p.getRight()));
+
if ( staticContentDir != null ) {
DefaultServlet staticServlet = new DefaultServlet();
ServletHolder staticContent = new ServletHolder(staticServlet);
staticContent.setInitParameter("resourceBase", staticContentDir);
context.addServlet(staticContent, "/");
}
}
-
+
--- End diff --
These leading space arise in Eclipse. After a closing "}" the cursor is padded to the same depth as the "}". Is there a way to change that without using a save action?
---
[GitHub] jena pull request #463: Cleanup
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:
https://github.com/apache/jena/pull/463#discussion_r210896418
--- Diff: jena-base/src/main/java/org/apache/jena/atlas/lib/ListUtils.java ---
@@ -34,49 +34,40 @@
{
private ListUtils() {}
- public static <T>
- List<T> unique(List<T> list)
- {
- return toList(list.stream().distinct()) ;
+ public static <T> List<T> unique(List<T> list) {
+ return toList(list.stream().distinct());
}
-
- public static
- List<Integer> asList(int... values)
- {
- List<Integer> x = new ArrayList<>(values.length) ;
+
+ public static List<Integer> asList(int...values) {
--- End diff --
Oh, my mistake :) :+1:
---