Skip to content

GH-5314: add validation check for construct query template #5321

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

GH-5314: add validation check for construct query template #5321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2bf139
GH-5314: Add property paths validation check for construct query
linnaung Apr 25, 2025
53fda35
GH-5314: rephrase exception message
linnaung Apr 26, 2025
cad419c
GH-5314: revert license header
linnaung May 4, 2025
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
55 changes: 47 additions & 8 deletions ...ryparser/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/TupleExprBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTPathAlternative;
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTPathElt;
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTPathMod;
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTPathNegatedPropertySet;
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTPathOneInPropertySet;
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTPathSequence;
import org.eclipse.rdf4j.query.parser.sparql.ast.ASTProjectionElem;
Expand Down Expand Up @@ -251,9 +252,6 @@ public class TupleExprBuilder extends AbstractASTVisitor {

GraphPattern graphPattern = new GraphPattern();

// private Map<ValueConstant, Var> mappedValueConstants = new
// HashMap<ValueConstant, Var>();

/*--------------*
* Constructors *
*--------------*/
Expand Down Expand Up @@ -820,6 +818,12 @@ public TupleExpr visit(ASTConstructQuery node, Object data) throws VisitorExcept

@Override
public TupleExpr visit(ASTConstruct node, Object data) throws VisitorException {

// check if the construct template contains any invalid nodes
if (isInvalidConstructQueryTemplate(node, true)) {
throw new MalformedQueryException("Invalid construct clause.");
}

TupleExpr result = (TupleExpr) data;

// Collect construct triples
Expand Down Expand Up @@ -872,7 +876,7 @@ public TupleExpr visit(ASTConstruct node, Object data) throws VisitorException {
// assign non-anonymous vars not present in where clause as
// extension elements. This is necessary to make external
// binding
// assingnment possible (see SES-996)
// assignment possible (see SES-996)
extElemMap.put(var, new ExtensionElem(var.clone(), var.getName()));
}
}
Expand Down Expand Up @@ -910,6 +914,41 @@ public TupleExpr visit(ASTConstruct node, Object data) throws VisitorException {
return new Reduced(result);
}

/**
* Checks if the given node is invalid in a CONSTRUCT template.
*
* @param node The node to check.
* @param isInConstructTemplate Indicates if the check is being performed within a CONSTRUCT clause.
* @return true if the node is invalid, false otherwise.
*/
private static boolean isInvalidConstructQueryTemplate(Node node, boolean isInConstructTemplate) {
if (isInConstructTemplate && isInvalidConstructNode(node)) {
return true;
}

// recursively check child nodes
for (int i = 0; i < node.jjtGetNumChildren(); i++) {
if (isInvalidConstructQueryTemplate(node.jjtGetChild(i), isInConstructTemplate)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an example query that recurses several levels deep?

Copy link
Contributor Author

@linnaung linnaung May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yes, it's provided as an invalid test case.

@Test
public void testInvalidConstructQueryWithZeroOrMorePathInPredicate() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
		"CONSTRUCT {\n" +
		"  ?s  foaf:knows* ?o . " +
		"}\n" +
		"WHERE {\n" +
		"  ?s  foaf:knows* ?o  .\n" +
		"} ";
assertThrows(MalformedQueryException.class, () -> {
	parser.parseQuery(invalidSparqlQuery, null);
});
}

@Test
public void testInvalidConstructQueryWithZeroOrMorePathInPredicates() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
		"CONSTRUCT {\n" +
		"  ?s  foaf:knows ?o . " +
		"  ?s  foaf:name+ ?o . " +
		"}\n" +
		"WHERE {\n" +
		"  ?s  foaf:knows* ?o  .\n" +
		"  ?s foaf:name ?o .\n" +
		"} ";
assertThrows(MalformedQueryException.class, () -> {
	parser.parseQuery(invalidSparqlQuery, null);
});
}

return true;
}
}

return false;
}

/**
* Checks if the given node is an invalid construct node.
*
* @param node The node to check.
* @return true if the node is invalid, false otherwise.
*/
private static boolean isInvalidConstructNode(Node node) {
return node instanceof ASTPathMod
|| node instanceof ASTPathNegatedPropertySet
|| node instanceof ASTPathOneInPropertySet
|| (node instanceof ASTPathAlternative && node.jjtGetNumChildren() > 1);
}

/**
* Gets the set of variables that are relevant for the constructor. This method accumulates all subject, predicate
* and object variables from the supplied statement patterns, but ignores any context variables.
Expand Down Expand Up @@ -1359,7 +1398,7 @@ public TupleExpr visit(ASTPathAlternative pathAltNode, Object data) throws Visit
}
}

// when using union to execute path expressions, the scope does not not change
// when using union to execute path expressions, the scope does not change
union.setVariableScopeChange(false);
return union;
}
Expand Down Expand Up @@ -1548,7 +1587,7 @@ private TupleExpr createTupleExprForNegatedPropertySets(List<PropertySetElem> np

TupleExpr patternMatchInverse = null;

// build a inverse statement pattern if needed
// build an inverse statement pattern if needed
if (filterConditionInverse != null) {
patternMatchInverse = new StatementPattern(pathSequenceContext.scope, endVar.clone(), predVar.clone(),
subjVar.clone(),
Expand Down Expand Up @@ -2362,7 +2401,7 @@ public Object visit(ASTBind node, Object data) throws VisitorException {
ValueExpr ve = child0 instanceof ValueExpr ? (ValueExpr) child0
: (child0 instanceof TripleRef) ? ((TripleRef) child0).getExprVar() : null;
if (ve == null) {
throw new IllegalArgumentException("Unexpected expressin on bind");
throw new IllegalArgumentException("Unexpected expression on bind");
}

// name to bind the expression outcome to
Expand All @@ -2381,7 +2420,7 @@ public Object visit(ASTBind node, Object data) throws VisitorException {

// check if alias is not previously used in the BGP
if (arg.getBindingNames().contains(alias)) {
// SES-2314 we need to doublecheck that the reused varname is not just
// SES-2314 we need to double-check that the reused varname is not just
// for an anonymous var or a constant.
VarCollector collector = new VarCollector();
arg.visit(collector);
Expand Down
155 changes: 155 additions & 0 deletions ...ryparser/sparql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/SPARQLParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.eclipse.rdf4j.model.util.Values;
import org.eclipse.rdf4j.query.BindingSet;
import org.eclipse.rdf4j.query.MalformedQueryException;
import org.eclipse.rdf4j.query.QueryLanguage;
import org.eclipse.rdf4j.query.algebra.AggregateFunctionCall;
import org.eclipse.rdf4j.query.algebra.ArbitraryLengthPath;
import org.eclipse.rdf4j.query.algebra.DeleteData;
Expand Down Expand Up @@ -70,6 +71,8 @@
import org.eclipse.rdf4j.query.parser.ParsedQuery;
import org.eclipse.rdf4j.query.parser.ParsedTupleQuery;
import org.eclipse.rdf4j.query.parser.ParsedUpdate;
import org.eclipse.rdf4j.query.parser.QueryParserUtil;
import org.eclipse.rdf4j.query.parser.sparql.SPARQLParser;
import org.eclipse.rdf4j.query.parser.sparql.aggregate.AggregateCollector;
import org.eclipse.rdf4j.query.parser.sparql.aggregate.AggregateFunction;
import org.eclipse.rdf4j.query.parser.sparql.aggregate.AggregateFunctionFactory;
Expand Down Expand Up @@ -1002,6 +1005,158 @@ public void testApostropheInsertData() {
parser.parseUpdate(query, null);
}

@Test
public void testInvalidConstructQueryWithPropertyPathInConstructClause() {
String invalidSparqlQuery = " CONSTRUCT {\n" +
" ?s (!a)* ?p .\n" +
" }\n" +
" WHERE {\n" +
" ?s (!a)* ?p .\n" +
" }";

assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});

}

@Test
public void testValidConstructQueryWithPropertyPathInWhereClause() {
String validSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?person foaf:knows ?friend .\n" +
"}\n" +
"WHERE {\n" +
" ?person foaf:knows+ ?friend .\n" +
"}";

ParsedQuery parsedQuery = parser.parseQuery(validSparqlQuery, null);
assertThat(parsedQuery.getSourceString()).isEqualTo(validSparqlQuery);

}

@Test
public void testValidConstructQueryWithBlankNodeAsSubject() {
String validSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/>\n"
+ "PREFIX site: <http://example.org/stats#>\n"
+ "\n"
+ "CONSTRUCT { [] foaf:name ?name }\n"
+ "WHERE\n"
+ "{ [] foaf:name ?name ;\n"
+ " site:hits ?hits .\n"
+ "}\n"
+ "ORDER BY desc(?hits)\n"
+ "LIMIT 2";

ParsedQuery parsedQuery = parser.parseQuery(validSparqlQuery, null);
assertThat(parsedQuery.getSourceString()).isEqualTo(validSparqlQuery);

}

@Test
public void testInvalidConstructQueryWithPropertyPathInPredicatePosition() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?person foaf:knows+ ?friend . " +
"}\n" +
"WHERE {\n" +
" ?person foaf:knows+ ?friend .\n" +
"}";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});

}

@Test
public void testInvalidConstructQueryWithPropertyPathAlternationInPredicate() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?x (foaf:knows|foaf:friendOf) ?y . " +
"}\n" +
"WHERE {\n" +
" ?x (foaf:knows|foaf:friendOf) ?y .\n" +
"} ";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});

}

@Test
public void testInvalidConstructQueryWithPathSequenceInPredicate() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?a foaf:knows/foaf:knows ?c . " +
"}\n" +
"WHERE {\n" +
" ?a foaf:knows/foaf:knows ?c . .\n" +
"} ";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});

}

@Test
public void testInvalidConstructQueryWithNegatedPropertySet() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?s !foaf:knows ?o . " +
"}\n" +
"WHERE {\n" +
" ?s !foaf:knows ?o .\n" +
"} ";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});

}

@Test
public void testInvalidConstructQueryWithZeroOrMorePathInPredicate() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?s foaf:knows* ?o . " +
"}\n" +
"WHERE {\n" +
" ?s foaf:knows* ?o .\n" +
"} ";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});
}

@Test
public void testInvalidConstructQueryWithZeroOrMorePathInPredicates() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?s foaf:knows ?o . " +
" ?s foaf:name+ ?o . " +
"}\n" +
"WHERE {\n" +
" ?s foaf:knows* ?o .\n" +
" ?s foaf:name ?o .\n" +
"} ";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});
}

@Test
public void testInvalidConstructQueryWithMalformedTriple() {
String invalidSparqlQuery = "PREFIX foaf: <http://xmlns.com/foaf/0.1/> " +
"CONSTRUCT {\n" +
" ?person foaf:name \n" +
"}\n" +
"WHERE {\n" +
" ?person foaf:name ?name .\n" +
"} ";
assertThrows(MalformedQueryException.class, () -> {
parser.parseQuery(invalidSparqlQuery, null);
});
}

private AggregateFunctionFactory buildDummyFactory() {
return new AggregateFunctionFactory() {
@Override
Expand Down
Loading