eclipse-rdf4jGH-3782 clean up handling of GROUP BY projection element…

… verification

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<String> 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<String> 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.");
+ }
  }
  }
  }

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,15 +56,15 @@ public class SPARQLParserTest {
  /**
  * @throws java.lang.Exception
  */
- @Before
+ @BeforeEach
  public void setUp() throws Exception {
  parser = new SPARQLParser();
  }
 
  /**
  * @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<ProjectionElem> 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);
+
+ }
 }