From 6823523433636af0faaebc16f71a5108a1317d19 Mon Sep 17 00:00:00 2001 From: Jeen Broekstra Date: Sat, 9 Apr 2022 15:03:31 +1200 Subject: [PATCH] GH-3782 clean up handling of GROUP BY projection element verification --- .../query/parser/sparql/TupleExprBuilder.java | 48 +++++----- .../query/parser/sparql/SPARQLParserTest.java | 93 +++++++++++++++---- 2 files changed, 103 insertions(+), 38 deletions(-) diff --git a/core/queryparser/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/TupleExprBuilder.java b/core/queryparser/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/TupleExprBuilder.java index f9de930eb72..fa5ae222cfc 100644 --- a/core/queryparser/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/TupleExprBuilder.java +++ b/core/queryparser/sparql/src/main/java/org/eclipse/rdf4j/query/parser/sparql/TupleExprBuilder.java @@ -635,32 +635,38 @@ public TupleExpr visit(ASTSelect node, Object data) throws VisitorException { result = new Projection(result, projElemList); if (group != null) { - for (ProjectionElem elem : projElemList.getElements()) { - if (!elem.hasAggregateOperatorInExpression()) { - Set groupNames = group.getBindingNames(); + boolean projectionHasAggregate = projElemList.getElements() + .stream() + .anyMatch(elem -> elem.hasAggregateOperatorInExpression()); - ExtensionElem extElem = elem.getSourceExpression(); - if (extElem != null) { - ValueExpr expr = extElem.getExpr(); + if (projectionHasAggregate) { + for (ProjectionElem elem : projElemList.getElements()) { + if (!elem.hasAggregateOperatorInExpression()) { - VarCollector collector = new VarCollector(); - expr.visit(collector); + Set groupNames = group.getBindingNames(); - for (Var var : collector.getCollectedVars()) { - if (!groupNames.contains(var.getName())) { - throw new VisitorException( - "variable '" + var.getName() + "' in projection not present in GROUP BY."); - - } - } - } else { - if (!groupNames.contains(elem.getTargetName())) { - throw new VisitorException( - "variable '" + elem.getTargetName() + "' in projection not present in GROUP BY."); - } else if (!groupNames.contains(elem.getSourceName())) { + // elem is only allowed to be a simple var or constant (see + // https://www.w3.org/TR/sparql11-query/#aggregateRestrictions) + ExtensionElem extElem = elem.getSourceExpression(); + if (extElem != null) { + ValueExpr expr = extElem.getExpr(); throw new VisitorException( - "variable '" + elem.getSourceName() + "' in projection not present in GROUP BY."); + "non-aggregate expression '" + expr + + "' not allowed in projection when using GROUP BY."); + } else { + if (!elem.getSourceName().equals(elem.getTargetName())) { + throw new VisitorException( + "non-aggregate expression '(?" + elem.getSourceName() + " AS ?" + + elem.getTargetName() + + ")' not allowed in projection when using GROUP BY."); + } + // projection element is simple var or constant. Must be present in GROUP BY. + if (!groupNames.contains(elem.getTargetName())) { + throw new VisitorException( + "variable '" + elem.getTargetName() + + "' in projection not present in GROUP BY."); + } } } } diff --git a/core/queryparser/sparql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/SPARQLParserTest.java b/core/queryparser/sparql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/SPARQLParserTest.java index aecb19d210b..a85666e7474 100644 --- a/core/queryparser/sparql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/SPARQLParserTest.java +++ b/core/queryparser/sparql/src/test/java/org/eclipse/rdf4j/query/parser/sparql/SPARQLParserTest.java @@ -8,13 +8,13 @@ package org.eclipse.rdf4j.query.parser.sparql; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; import java.util.List; @@ -41,9 +41,10 @@ import org.eclipse.rdf4j.query.parser.ParsedQuery; import org.eclipse.rdf4j.query.parser.ParsedTupleQuery; import org.eclipse.rdf4j.query.parser.ParsedUpdate; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import org.eclipse.rdf4j.query.parser.sparql.ast.VisitorException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; /** * @author jeen @@ -55,7 +56,7 @@ public class SPARQLParserTest { /** * @throws java.lang.Exception */ - @Before + @BeforeEach public void setUp() throws Exception { parser = new SPARQLParser(); } @@ -63,7 +64,7 @@ public void setUp() throws Exception { /** * @throws java.lang.Exception */ - @After + @AfterEach public void tearDown() throws Exception { parser = null; } @@ -78,8 +79,7 @@ public void testSourceStringAssignment() throws Exception { ParsedQuery q = parser.parseQuery(simpleSparqlQuery, null); - assertNotNull(q); - assertEquals(simpleSparqlQuery, q.getSourceString()); + assertThat(q.getSourceString()).isEqualTo(simpleSparqlQuery); } @Test @@ -90,7 +90,7 @@ public void testInsertDataLineNumberReporting() throws Exception { ParsedUpdate u = parser.parseUpdate(insertDataString, null); fail("should have resulted in parse exception"); } catch (MalformedQueryException e) { - assertTrue(e.getMessage().contains("line 2,")); + assertThat(e.getMessage()).contains("line 2,"); } } @@ -103,7 +103,7 @@ public void testDeleteDataLineNumberReporting() throws Exception { ParsedUpdate u = parser.parseUpdate(deleteDataString, null); fail("should have resulted in parse exception"); } catch (MalformedQueryException e) { - assertTrue(e.getMessage().contains("line 2,")); + assertThat(e.getMessage()).contains("line 2,"); } } @@ -171,7 +171,7 @@ public void testParseWildcardSubselectInUpdate() throws Exception { List elements = projectionElemList.getElements(); assertNotNull(elements); - assertEquals("projection should contain all three variables", 3, elements.size()); + assertThat(elements).hasSize(3); } @Test @@ -381,4 +381,63 @@ public void testWildCardPathComplexSubjectHandling() { assertThat(subClassOfObjectVar).isEqualTo(commentObjectVar); } + + @Test + public void testGroupByProjectionHandling_NoAggregate() { + String query = "SELECT DISTINCT ?s (?o AS ?o1) \n" + + "WHERE {\n" + + " ?s ?p ?o \n" + + "} GROUP BY ?o"; + + // should parse without error + parser.parseQuery(query, null); + } + + @Test + public void testGroupByProjectionHandling_Aggregate_NonSimpleExpr() { + String query = "SELECT (COUNT(?s) as ?count) (?o + ?s AS ?o1) \n" + + "WHERE {\n" + + " ?s ?p ?o \n" + + "} GROUP BY ?o"; + + assertThatExceptionOfType(MalformedQueryException.class).isThrownBy(() -> parser.parseQuery(query, null)) + .withMessageStartingWith("non-aggregate expression 'MathExpr (+)"); + + } + + @Test + public void testGroupByProjectionHandling_Aggregate_Alias() { + String query = "SELECT (COUNT(?s) as ?count) (?o AS ?o1) \n" + + "WHERE {\n" + + " ?s ?p ?o \n" + + "} GROUP BY ?o"; + + assertThatExceptionOfType(MalformedQueryException.class).isThrownBy(() -> parser.parseQuery(query, null)) + .withMessageStartingWith("non-aggregate expression '(?o AS ?o1)"); + + } + + @Test + public void testGroupByProjectionHandling_Aggregate_SimpleExpr() { + String query = "SELECT (COUNT(?s) as ?count) ?o \n" + + "WHERE {\n" + + " ?s ?p ?o \n" + + "} GROUP BY ?p"; + + assertThatExceptionOfType(MalformedQueryException.class).isThrownBy(() -> parser.parseQuery(query, null)) + .withMessageStartingWith("variable 'o' in projection not present in GROUP BY."); + + } + + @Test + public void testGroupByProjectionHandling_Aggregate_SimpleExpr2() { + String query = "SELECT (COUNT(?s) as ?count) ?o \n" + + "WHERE {\n" + + " ?s ?p ?o \n" + + "} GROUP BY ?o"; + + // should parse without error + parser.parseQuery(query, null); + + } }