sparesults: return error on duplicated variable declaration

lib/sparesults/src/csv.rs

@@ -204,13 +204,21 @@ impl<R: BufRead> TsvQueryResultsReader<R> {
         if buffer.trim().eq_ignore_ascii_case("false") {
             return Ok(Self::Boolean(false));
         }
-        let variables = buffer
+        let mut variables = Vec::new();
-            .split('\t')
+        for v in buffer.split('\t') {
-            .map(|v| {
+            let v = v.trim();
-                Variable::from_str(v.trim())
+            let variable = Variable::from_str(v).map_err(|e| {
-                    .map_err(|e| SyntaxError::msg(format!("Invalid variable name '{}': {}", v, e)))
+                SyntaxError::msg(format!("Invalid variable declaration '{}': {}", v, e))
-            })
+            })?;
-            .collect::<Result<Vec<_>, _>>()?;
+            if variables.contains(&variable) {
+                return Err(SyntaxError::msg(format!(
+                    "The variable {} is declared twice",
+                    variable
+                ))
+                .into());
+            }
+            variables.push(variable);
+        }
 
         Ok(Self::Solutions {
             variables,

lib/sparesults/src/json.rs

@@ -145,7 +145,9 @@ impl<R: BufRead> JsonQueryResultsReader<R> {
             let event = reader.read_event(&mut buffer)?;
             match event {
                 JsonEvent::ObjectKey(key) => match key {
-                    "head" => variables = Some(read_head(&mut reader, &mut buffer)?),
+                    "head" => {
+                        variables = Some(read_head(&mut reader, &mut buffer)?);
+                    }
                     "results" => {
                         if reader.read_event(&mut buffer)? != JsonEvent::StartObject {
                             return Err(SyntaxError::msg("'results' should be an object").into());
@@ -162,20 +164,10 @@ impl<R: BufRead> JsonQueryResultsReader<R> {
                         return if let Some(variables) = variables {
                             let mut mapping = BTreeMap::default();
                             for (i, var) in variables.iter().enumerate() {
-                                mapping.insert(var.clone(), i);
+                                mapping.insert(var.as_str().to_string(), i);
                             }
                             Ok(Self::Solutions {
-                                variables: variables
+                                variables,
-                                    .into_iter()
-                                    .map(|v| {
-                                        Variable::new(v).map_err(|e| {
-                                            SyntaxError::msg(format!(
-                                                "Invalid variable name: {}",
-                                                e
-                                            ))
-                                        })
-                                    })
-                                    .collect::<Result<Vec<_>, _>>()?,
                                 solutions: JsonSolutionsReader {
                                     reader,
                                     buffer,
@@ -448,45 +440,67 @@ impl<R: BufRead> JsonSolutionsReader<R> {
 fn read_head<R: BufRead>(
     reader: &mut JsonReader<R>,
     buffer: &mut Vec<u8>,
-) -> Result<Vec<String>, ParseError> {
+) -> Result<Vec<Variable>, ParseError> {
     if reader.read_event(buffer)? != JsonEvent::StartObject {
         return Err(SyntaxError::msg("head should be an object").into());
     }
-    let mut variables = None;
+    let mut variables = Vec::new();
     loop {
         match reader.read_event(buffer)? {
             JsonEvent::ObjectKey(key) => match key {
-                "vars" => variables = Some(read_string_array(reader, buffer)?),
+                "vars" => {
-                "link" => {
+                    if reader.read_event(buffer)? != JsonEvent::StartArray {
-                    read_string_array(reader, buffer)?;
+                        return Err(SyntaxError::msg("Variable list should be an array").into());
+                    }
+                    loop {
+                        match reader.read_event(buffer)? {
+                            JsonEvent::String(s) => {
+                                let new_var = Variable::new(s).map_err(|e| {
+                                    SyntaxError::msg(format!(
+                                        "Invalid variable declaration '{}': {}",
+                                        s, e
+                                    ))
+                                })?;
+                                if variables.contains(&new_var) {
+                                    return Err(SyntaxError::msg(format!(
+                                        "The variable {} is declared twice",
+                                        new_var
+                                    ))
+                                    .into());
+                                }
+                                variables.push(new_var);
                             }
+                            JsonEvent::EndArray => break,
                             _ => {
                                 return Err(
-                        SyntaxError::msg(format!("Unexpected key in head: '{}'", key)).into(),
+                                    SyntaxError::msg("Variable names should be strings").into()
                                 )
                             }
-            },
-            JsonEvent::EndObject => return Ok(variables.unwrap_or_else(Vec::new)),
-            _ => return Err(SyntaxError::msg("Invalid head serialization").into()),
                         }
                     }
                 }
-
+                "link" => {
-fn read_string_array<R: BufRead>(
-    reader: &mut JsonReader<R>,
-    buffer: &mut Vec<u8>,
-) -> Result<Vec<String>, ParseError> {
                     if reader.read_event(buffer)? != JsonEvent::StartArray {
                         return Err(SyntaxError::msg("Variable list should be an array").into());
                     }
-    let mut elements = Vec::new();
                     loop {
                         match reader.read_event(buffer)? {
-            JsonEvent::String(s) => {
+                            JsonEvent::String(_) => (),
-                elements.push(s.into());
+                            JsonEvent::EndArray => break,
+                            _ => {
+                                return Err(SyntaxError::msg("Link names should be strings").into())
+                            }
                         }
-            JsonEvent::EndArray => return Ok(elements),
+                    }
-            _ => return Err(SyntaxError::msg("Variable names should be strings").into()),
+                }
+                _ => {
+                    return Err(
+                        SyntaxError::msg(format!("Unexpected key in head: '{}'", key)).into(),
+                    )
+                }
+            },
+            JsonEvent::EndObject => return Ok(variables),
+            _ => return Err(SyntaxError::msg("Invalid head serialization").into()),
         }
     }
 }

lib/sparesults/src/xml.rs

@@ -207,7 +207,15 @@ impl<R: BufRead> XmlQueryResultsReader<R> {
                                 .find(|attr| attr.key == b"name")
                                 .ok_or_else(|| SyntaxError::msg("No name attribute found for the <variable> tag"))?
                                 .unescape_and_decode_value(&reader)?;
-                            variables.push(Variable::new(name).map_err(|e| SyntaxError::msg(format!("Invalid variable name: {}", e)))?);
+                            let variable = Variable::new(name).map_err(|e| SyntaxError::msg(format!("Invalid variable name: {}", e)))?;
+                            if variables.contains(&variable) {
+                                return Err(SyntaxError::msg(format!(
+                                    "The variable {} is declared twice",
+                                    variable
+                                ))
+                                    .into());
+                            }
+                            variables.push(variable);
                         } else if event.name() == b"link" {
                             // no op
                         } else {

testsuite/oxigraph-tests/sparql-results/duplicated_variables.srj

@@ -0,0 +1,15 @@
+{
+  "head": {
+    "vars": ["s", "p", "s"]
+  },
+  "results": {
+    "bindings": [
+      {
+        "s": {
+          "type": "uri",
+          "value": "http://example.org/s1"
+        }
+      }
+    ]
+  }
+}

testsuite/oxigraph-tests/sparql-results/duplicated_variables.srx

@@ -0,0 +1,15 @@
+<?xml version="1.0"?>
+<sparql xmlns="http://www.w3.org/2005/sparql-results#">
+   <head>
+      <variable name="s"/>
+      <variable name="p"/>
+      <variable name="s"/>
+   </head>
+   <results>
+      <result>
+         <binding name="s">
+            <uri>http://example.com/a</uri>
+         </binding>
+      </result>
+   </results>
+</sparql>

testsuite/oxigraph-tests/sparql-results/duplicated_variables.tsv

@@ -0,0 +1,2 @@
+?s  ?p  ?s
+"s" "s" "s"

testsuite/oxigraph-tests/sparql-results/manifest.ttl

@@ -0,0 +1,26 @@
+@prefix rdf:   <http://www.w3.org/1999/02/22-rdf-syntax-ns#> .
+@prefix :      <https://github.com/oxigraph/oxigraph/tests/sparql-results/manifest#> .
+@prefix rdfs:  <http://www.w3.org/2000/01/rdf-schema#> .
+@prefix mf:    <http://www.w3.org/2001/sw/DataAccess/tests/test-manifest#> .
+@prefix ox:    <https://github.com/oxigraph/oxigraph/tests#> .
+
+<>  rdf:type mf:Manifest ;
+    rdfs:label "Oxigraph SPARQL resutls tests" ;
+    mf:entries
+    (
+    :results_json_duplicated_variables
+    :results_xml_duplicated_variables
+    :results_tsv_duplicated_variables
+    ) .
+
+:results_json_duplicated_variables rdf:type ox:NegativeJsonResultsSyntaxTest ;
+    mf:name "Duplicated variables are not allowed" ;
+    mf:action <duplicated_variables.srj> .
+
+:results_xml_duplicated_variables rdf:type ox:NegativeXmlResultsSyntaxTest ;
+    mf:name "Duplicated variables are not allowed" ;
+    mf:action <duplicated_variables.srx> .
+
+:results_xml_duplicated_variables rdf:type ox:NegativeTsvResultsSyntaxTest ;
+    mf:name "Duplicated variables are not allowed" ;
+    mf:action <duplicated_variables.tsv> .

testsuite/src/sparql_evaluator.rs

@@ -9,6 +9,7 @@ use oxigraph::model::*;
 use oxigraph::sparql::*;
 use oxigraph::store::Store;
 use std::collections::HashMap;
+use std::io::Cursor;
 use std::str::FromStr;
 use std::sync::Arc;
 use std::{fmt, io};
@@ -46,6 +47,18 @@ pub fn register_sparql_tests(evaluator: &mut TestEvaluator) {
         "http://www.w3.org/2001/sw/DataAccess/tests/test-manifest#UpdateEvaluationTest",
         evaluate_update_evaluation_test,
     );
+    evaluator.register(
+        "https://github.com/oxigraph/oxigraph/tests#NegativeJsonResultsSyntaxTest",
+        evaluate_negative_json_result_syntax_test,
+    );
+    evaluator.register(
+        "https://github.com/oxigraph/oxigraph/tests#NegativeXmlResultsSyntaxTest",
+        evaluate_negative_xml_result_syntax_test,
+    );
+    evaluator.register(
+        "https://github.com/oxigraph/oxigraph/tests#NegativeTsvResultsSyntaxTest",
+        evaluate_negative_tsv_result_syntax_test,
+    );
 }
 
 fn evaluate_positive_syntax_test(test: &Test) -> Result<()> {
@@ -82,6 +95,51 @@ fn evaluate_negative_syntax_test(test: &Test) -> Result<()> {
     }
 }
 
+fn evaluate_negative_json_result_syntax_test(test: &Test) -> Result<()> {
+    if result_syntax_check(test, QueryResultsFormat::Json).is_ok() {
+        Err(anyhow!("Oxigraph parses even if it should not {}.", test))
+    } else {
+        Ok(())
+    }
+}
+
+fn evaluate_negative_xml_result_syntax_test(test: &Test) -> Result<()> {
+    if result_syntax_check(test, QueryResultsFormat::Xml).is_ok() {
+        Err(anyhow!("Oxigraph parses even if it should not {}.", test))
+    } else {
+        Ok(())
+    }
+}
+
+fn evaluate_negative_tsv_result_syntax_test(test: &Test) -> Result<()> {
+    if result_syntax_check(test, QueryResultsFormat::Tsv).is_ok() {
+        Err(anyhow!("Oxigraph parses even if it should not {}.", test))
+    } else {
+        Ok(())
+    }
+}
+
+fn result_syntax_check(test: &Test, format: QueryResultsFormat) -> Result<()> {
+    let results_file = test
+        .action
+        .as_deref()
+        .ok_or_else(|| anyhow!("No action found for test {}", test))?;
+    match QueryResults::read(Cursor::new(read_file_to_string(results_file)?), format)? {
+        QueryResults::Solutions(solutions) => {
+            for s in solutions {
+                s?;
+            }
+        }
+        QueryResults::Graph(triples) => {
+            for t in triples {
+                t?;
+            }
+        }
+        QueryResults::Boolean(_) => (),
+    }
+    Ok(())
+}
+
 fn evaluate_evaluation_test(test: &Test) -> Result<()> {
     let store = Store::new()?;
     if let Some(data) = &test.data {

testsuite/tests/oxigraph.rs

@@ -26,3 +26,10 @@ fn oxigraph_sparql_testsuite() -> Result<()> {
         "https://github.com/oxigraph/oxigraph/tests/sparql/manifest.ttl",
     ])
 }
+
+#[test]
+fn oxigraph_sparql_results_testsuite() -> Result<()> {
+    run_testsuite(vec![
+        "https://github.com/oxigraph/oxigraph/tests/sparql-results/manifest.ttl",
+    ])
+}