You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by GitBox <gi...@apache.org> on 2020/11/24 19:00:57 UTC
[GitHub] [jena] fkleedorfer opened a new pull request #870: Add public getters to SHACL constraint implementations
fkleedorfer opened a new pull request #870:
URL: https://github.com/apache/jena/pull/870
As documented in https://issues.apache.org/jira/browse/JENA-2002, this commit adds public getters for accessing internal data in SHACL constraints.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r534730915
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/QualifiedValueShape.java
##########
@@ -50,6 +50,22 @@ public QualifiedValueShape(Shape sub, int qMin, int qMax, boolean qDisjoint) {
this.qDisjoint = qDisjoint;
}
+ public Shape getSub() {
+ return sub;
+ }
+
+ public int getqMin() {
Review comment:
ok (`getQMin()` has its merits, but your call)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #870:
URL: https://github.com/apache/jena/pull/870#issuecomment-736733369
Sorry for the delay - I was seeing the release through.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r534717575
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/CardinalityConstraint.java
##########
@@ -38,7 +38,15 @@ public void validateNodeShape(ValidationContext vCxt, Graph data, Shape shape, N
// Node shape with cardinality. Can this usefully be checked for in the parser?
throw new ShaclParseException("Cardinality constraint on a node shape");
}
-
+
+ public int getMinCount(){
Review comment:
ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r530253747
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/ConstraintComponentSPARQL.java
##########
@@ -59,6 +59,18 @@ public ConstraintComponentSPARQL(SparqlComponent sparqlConstraintComponent,
}
}
+ public SparqlComponent getSparqlConstraintComponent() {
+ return sparqlConstraintComponent;
+ }
+
+ public Multimap<Parameter, Node> getParameterMap() {
+ return parameterMap;
Review comment:
Sorry, good catch. I think wrapping with unmodifiable collections or cloning in the getter should do. I'll add another commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] kinow commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r530269016
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/ConstraintComponentSPARQL.java
##########
@@ -59,6 +59,18 @@ public ConstraintComponentSPARQL(SparqlComponent sparqlConstraintComponent,
}
}
+ public SparqlComponent getSparqlConstraintComponent() {
+ return sparqlConstraintComponent;
+ }
+
+ public Multimap<Parameter, Node> getParameterMap() {
+ return parameterMap;
Review comment:
>For the Multimap in ConstraintComponentSPARQL I am not sure how to proceed, I think it's best to revert the change on that one.
If that's OK for you for now, then all good :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #870: JENA-2002: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #870:
URL: https://github.com/apache/jena/pull/870#issuecomment-739490079
Seeing a PR for the visitor pattern would be interesting.
Sometimes it take time to get round to PRs when there are several in-progress. This week has been getting in the ones that are best done at the beginning of a release cycle.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on pull request #870: JENA-2002: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on pull request #870:
URL: https://github.com/apache/jena/pull/870#issuecomment-739526037
ValueRangeConstraint and ConstraintPairwise - have two getters for the same thing. `Constraint` already has `getComponent`.
I've removed the stray.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r534722919
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/JViolationConstraint.java
##########
@@ -35,6 +35,10 @@ public JViolationConstraint(boolean generateViolation) {
this.generateViolation = generateViolation;
}
+ public boolean isGenerateViolation() {
Review comment:
going with `generatesViolation()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r529848215
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/ConstraintComponentSPARQL.java
##########
@@ -59,6 +59,18 @@ public ConstraintComponentSPARQL(SparqlComponent sparqlConstraintComponent,
}
}
+ public SparqlComponent getSparqlConstraintComponent() {
+ return sparqlConstraintComponent;
+ }
+
+ public Multimap<Parameter, Node> getParameterMap() {
+ return parameterMap;
Review comment:
Yes, it does matter. `.equals` relies on all parts of the shape and shapes go into datastructures so `hashCode` and `equals` must be stable. A `Collections.unmodifiable` (on "get" or in the constructor, not sure which is better) or a structure copy is needed. The current code probably isn't perfect.
Or documentation!
Another to look at is whether internal calculated members should be made public.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r533625107
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/PatternConstraint.java
##########
@@ -53,6 +53,18 @@ public PatternConstraint(String pattern, String flagsStr) {
this.pattern = Pattern.compile(pattern, flags);
}
+ public Pattern getPattern() {
Review comment:
Minor: I'd prefer not the Pattern because it is an internal term, and then have the pattern string as `getPattern()`.
It is exposing the information once, not in two different ways.
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/CardinalityConstraint.java
##########
@@ -38,7 +38,15 @@ public void validateNodeShape(ValidationContext vCxt, Graph data, Shape shape, N
// Node shape with cardinality. Can this usefully be checked for in the parser?
throw new ShaclParseException("Cardinality constraint on a node shape");
}
-
+
+ public int getMinCount(){
Review comment:
Not sure if this the best thing to do. The subclass for max and min already have accessors.
Being an abstract class, CardinalityConstraint can't be created directly.
"MaxCount" have a "getMinCount" function is a bit strange. If there were a general min and max at the same time constraint, then on the superclass would make more sense but there isn't.
Please remove theses and use overrides in the subclasses.
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/QualifiedValueShape.java
##########
@@ -50,6 +50,22 @@ public QualifiedValueShape(Shape sub, int qMin, int qMax, boolean qDisjoint) {
this.qDisjoint = qDisjoint;
}
+ public Shape getSub() {
+ return sub;
+ }
+
+ public int getqMin() {
Review comment:
AutoCase does not work out! And "Q" does work that well either! How about `public int qMin()`?
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/JViolationConstraint.java
##########
@@ -35,6 +35,10 @@ public JViolationConstraint(boolean generateViolation) {
this.generateViolation = generateViolation;
}
+ public boolean isGenerateViolation() {
Review comment:
Minor: "generateViolation" read better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on pull request #870:
URL: https://github.com/apache/jena/pull/870#issuecomment-739467109
Additionally, I've had to implement a kind of non-invasive visitor pattern to process constraint structures (such as sh:or/sh:node):
https://github.com/researchstudio-sat/webofneeds/blob/feat-acl-impl/webofneeds/won-utils/won-utils-shacl2java/src/main/java/won/shacl2java/constraints/ConstraintVisitor.java
Of course, I would have preferred to use visitor methods offered by the Constraint classes. If you are interested, I can modify my implementation and add the visitor methods and visitor interface to the jena classes in a separate PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r534729902
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/PatternConstraint.java
##########
@@ -53,6 +53,18 @@ public PatternConstraint(String pattern, String flagsStr) {
this.pattern = Pattern.compile(pattern, flags);
}
+ public Pattern getPattern() {
Review comment:
ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on pull request #870:
URL: https://github.com/apache/jena/pull/870#issuecomment-739464769
If I am not mistaken, we're done. Please let me know if this is not the case. Cheers!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] kinow commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r529840930
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/ConstraintComponentSPARQL.java
##########
@@ -59,6 +59,18 @@ public ConstraintComponentSPARQL(SparqlComponent sparqlConstraintComponent,
}
}
+ public SparqlComponent getSparqlConstraintComponent() {
+ return sparqlConstraintComponent;
+ }
+
+ public Multimap<Parameter, Node> getParameterMap() {
+ return parameterMap;
Review comment:
Not sure if there's any risk here (or in the other getters that return objects and lists) to have concurrency issues. If so, we could copy the underlying object/collection.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] fkleedorfer commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
fkleedorfer commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r530256689
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/ConstraintComponentSPARQL.java
##########
@@ -59,6 +59,18 @@ public ConstraintComponentSPARQL(SparqlComponent sparqlConstraintComponent,
}
}
+ public SparqlComponent getSparqlConstraintComponent() {
+ return sparqlConstraintComponent;
+ }
+
+ public Multimap<Parameter, Node> getParameterMap() {
+ return parameterMap;
Review comment:
For the `Multimap` in `ConstraintComponentSPARQL` I am not sure how to proceed, I think it's best to revert the change on that one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r535445659
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/JViolationConstraint.java
##########
@@ -35,6 +35,10 @@ public JViolationConstraint(boolean generateViolation) {
this.generateViolation = generateViolation;
}
+ public boolean isGenerateViolation() {
Review comment:
Yes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs commented on a change in pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r535445930
##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/QualifiedValueShape.java
##########
@@ -50,6 +50,22 @@ public QualifiedValueShape(Shape sub, int qMin, int qMax, boolean qDisjoint) {
this.qDisjoint = qDisjoint;
}
+ public Shape getSub() {
+ return sub;
+ }
+
+ public int getqMin() {
Review comment:
Ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
[GitHub] [jena] afs merged pull request #870: Add public getters to SHACL constraint implementations
Posted by GitBox <gi...@apache.org>.
afs merged pull request #870:
URL: https://github.com/apache/jena/pull/870
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org