From f0bd89d77fdd724b0ffe934ba7188ad90d6189a9 Mon Sep 17 00:00:00 2001
From: Tpt <thomaspt@hotmail.fr>
Date: Fri, 13 Aug 2021 21:08:54 +0200
Subject: [PATCH] Fixes BIND variables validation

---
 spargebra/src/algebra.rs  | 30 ++++++++++++++++++++++++------
 spargebra/src/parser.rs   | 17 ++++++++---------
 testsuite/tests/sparql.rs |  4 ----
 3 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/spargebra/src/algebra.rs b/spargebra/src/algebra.rs
index 98e1eab1..e2adffaa 100644
--- a/spargebra/src/algebra.rs
+++ b/spargebra/src/algebra.rs
@@ -739,12 +739,30 @@ impl<'a> fmt::Display for SparqlGraphPattern<'a> {
                 path,
                 object,
             } => write!(f, "{} {} {} .", subject, SparqlPropertyPath(path), object),
-            GraphPattern::Join { left, right } => write!(
-                f,
-                "{} {}",
-                SparqlGraphPattern(&*left),
-                SparqlGraphPattern(&*right)
-            ),
+            GraphPattern::Join { left, right } => {
+                if matches!(
+                    right.as_ref(),
+                    GraphPattern::LeftJoin { .. }
+                        | GraphPattern::Minus { .. }
+                        | GraphPattern::Extend { .. }
+                        | GraphPattern::Filter { .. }
+                ) {
+                    // The second block might be considered as a modification of the first one.
+                    write!(
+                        f,
+                        "{} {{ {} }}",
+                        SparqlGraphPattern(&*left),
+                        SparqlGraphPattern(&*right)
+                    )
+                } else {
+                    write!(
+                        f,
+                        "{} {}",
+                        SparqlGraphPattern(&*left),
+                        SparqlGraphPattern(&*right)
+                    )
+                }
+            }
             GraphPattern::LeftJoin { left, right, expr } => {
                 if let Some(expr) = expr {
                     write!(
diff --git a/spargebra/src/parser.rs b/spargebra/src/parser.rs
index 3e3c0048..8376056a 100644
--- a/spargebra/src/parser.rs
+++ b/spargebra/src/parser.rs
@@ -1352,14 +1352,10 @@ parser! {
         }
 
         //[54]
-        rule GroupGraphPatternSub() -> GraphPattern = a:TriplesBlock()? _ b:GroupGraphPatternSub_item()* {
-            let mut p = a.map_or_else(Vec::default, |v| vec![PartialGraphPattern::Other(build_bgp(v))]);
-            for v in b {
-                p.extend(v)
-            }
+        rule GroupGraphPatternSub() -> GraphPattern = a:TriplesBlock()? _ b:GroupGraphPatternSub_item()* {?
             let mut filter: Option<Expression> = None;
-            let mut g = GraphPattern::default();
-            for e in p {
+            let mut g = a.map_or_else(GraphPattern::default, build_bgp);
+            for e in b.into_iter().flatten() {
                 match e {
                     PartialGraphPattern::Optional(p, f) => {
                         g = GraphPattern::LeftJoin { left: Box::new(g), right: Box::new(p), expr: f }
@@ -1368,6 +1364,9 @@ parser! {
                         g = GraphPattern::Minus { left: Box::new(g), right: Box::new(p) }
                     }
                     PartialGraphPattern::Bind(expr, var) => {
+                        if g.visible_variables().contains(&var) {
+                            return Err("BIND is overriding an existing variable")
+                        }
                         g = GraphPattern::Extend { inner: Box::new(g), var, expr }
                     }
                     PartialGraphPattern::Filter(expr) => filter = Some(if let Some(f) = filter {
@@ -1379,11 +1378,11 @@ parser! {
                 }
             }
 
-            if let Some(expr) = filter {
+            Ok(if let Some(expr) = filter {
                 GraphPattern::Filter { expr, inner: Box::new(g) }
             } else {
                 g
-            }
+            })
         }
         rule GroupGraphPatternSub_item() -> Vec<PartialGraphPattern> = a:GraphPatternNotTriples() _ ("." _)? b:TriplesBlock()? _ {
             let mut result = vec![a];
diff --git a/testsuite/tests/sparql.rs b/testsuite/tests/sparql.rs
index 2055f10e..db8b337e 100644
--- a/testsuite/tests/sparql.rs
+++ b/testsuite/tests/sparql.rs
@@ -69,10 +69,6 @@ fn sparql11_query_w3c_evaluation_testsuite() -> Result<()> {
     run_testsuite(
         "http://www.w3.org/2009/sparql/docs/tests/data-sparql11/manifest-sparql11-query.ttl",
         vec![
-            //Bad SPARQL query that should be rejected by the parser
-            "http://www.w3.org/2009/sparql/docs/tests/data-sparql11/syntax-query/manifest#test_60",
-            "http://www.w3.org/2009/sparql/docs/tests/data-sparql11/syntax-query/manifest#test_61a",
-            "http://www.w3.org/2009/sparql/docs/tests/data-sparql11/syntax-query/manifest#test_62a",
             //BNODE() scope is currently wrong
             "http://www.w3.org/2009/sparql/docs/tests/data-sparql11/functions/manifest#bnode01",
             //Property path with unbound graph name are not supported yet