Repository: RDFLib/rdflib

Issue:
In using SERVICE, "string" variables get retrieved as NULL
I was testing the federated SPARQL queries using "service". There are two issues:
* only low-case "service" is accepted, i.e., SPARQL with "SERVICE" would fail. This was fixed for next release. 
*  "string" variables get retrieved as null. This is new.

Sorry, I do not have time to create a small reproducible example. 
Below, two examples of querying the same SPARQL endpoint, one with   

from pandas import DataFrame
from rdflib.plugins.sparql.processor import SPARQLResult

def sparql_results_to_df(results: SPARQLResult) -> DataFrame:
    return DataFrame(
        data=([None if x is None else x.toPython() for x in row] for row in results),
        columns=[str(x) for x in results.vars],
    )

qf="""
    PREFIX tsmodel: <http://siemens.com/tsmodel/>
    PREFIX vobj: <http://siemens.com/tsmodel/VAROBJECTS#>
                  
     SELECT ?x ?node_name ?idx 
     {      
        service <http://localhost:8080/sparql>
        {
            ?x a tsmodel:VAROBJECTS .
            ?x vobj:idx ?idx .
            ?x vobj:node_name ?node_name .
        }
      }
"""

results=graph.query(qf)
df=sparql_results_to_df(results)
df.head(20)

x	node_name	idx
0	http://siemens.com/tsmodel/VAROBJECTS/idx=1	None	1
1	http://siemens.com/tsmodel/VAROBJECTS/idx=2	None	2
2	http://siemens.com/tsmodel/VAROBJECTS/idx=3	None	3
3	http://siemens.com/tsmodel/VAROBJECTS/idx=4	None	4
4	http://siemens.com/tsmodel/VAROBJECTS/idx=5	None	5

import sparql_dataframe

endpoint = "http://localhost:8080/sparql"

q = """
PREFIX tsmodel: <http://siemens.com/tsmodel/>
PREFIX vobj: <http://siemens.com/tsmodel/VAROBJECTS#>

SELECT ?x ?node_name ?idx { 
        ?x a tsmodel:VAROBJECTS .
        ?x vobj:idx ?idx .
        ?x vobj:node_name ?node_name .
    }
"""
        
df = sparql_dataframe.get(endpoint, q)
df.head()

	x	node_name	idx
0	http://siemens.com/tsmodel/VAROBJECTS/idx=1	ns=3;s="ultrasonicLeft"	1
1	http://siemens.com/tsmodel/VAROBJECTS/idx=2	ns=3;s="voltageL1-N"	2
2	http://siemens.com/tsmodel/VAROBJECTS/idx=3	ns=3;s="voltageL2-N"	3
3	http://siemens.com/tsmodel/VAROBJECTS/idx=4	ns=3;s="voltageL3-N"	4
4	http://siemens.com/tsmodel/VAROBJECTS/idx=5	ns=3;s="currentL1"	5


I do have more examples like this. As I have checked, "int", "float", "datetime" work fine. But, "string" variables are always get "NULL". 


Discussion:
aucampia: Also it seems your test is failing: https://github.com/RDFLib/rdflib/runs/6341689732?check_suite_focus=true#step:9:372

```console
def test():
          """Test service returns simple literals not as NULL.
      
          Issue: https://github.com/RDFLib/rdflib/issues/1278
          """
      
          g = Graph()
          q = """SELECT ?s ?p ?o
      WHERE {
          SERVICE <https://dbpedia.org/sparql> {
              VALUES (?s ?p ?o) {(<http://example.org/a> <http://example.org/b> "c")}
          }
      }"""
  >       assert results.bindings[0].get(Variable("o")) == Literal("c")
  E       NameError: name 'results' is not defined
```
gitmpje: Thanks for your support, the failing test should be fixed now. This test uses a similar approach as the existing SERVICE tests: https://github.com/RDFLib/rdflib/blob/eba13739e1d24b3e52697a1ec9545a361116951f/test/test_sparql/test_service.py. When I have time I will have a look at your suggestions for improving the test(s). 
coveralls: 
[![Coverage Status](https://coveralls.io/builds/48929071/badge)](https://coveralls.io/builds/48929071)

Coverage decreased (-0.003%) to 88.235% when pulling **ccbdbc2d2342d42c09405b934d8a9fe1e2cf3933 on gitmpje:issue1278_null_literals** into **eba13739e1d24b3e52697a1ec9545a361116951f on RDFLib:master**.

aucampia: > Thanks for your support, the failing test should be fixed now. This test uses a similar approach as the existing SERVICE tests: https://github.com/RDFLib/rdflib/blob/eba13739e1d24b3e52697a1ec9545a361116951f/test/test_sparql/test_service.py. When I have time I will have a look at your suggestions for improving the test(s).

Okay in that case I think it is fine then, but maybe it would be good to add this test to `test_service.py`.

gitmpje: I added a more general test for different node types returned by a SERVICE clause to test_service.py.
aucampia: @gitmpje will review tonight, I don't think there are any issues with the code, but I may move tests around slightly, thank you for the PR.
gitmpje: > @gitmpje thanks for the PR. I added more tests to your branch and made some minor mostly cosmetic improvements, let me know if there are any concerns you have with the changes.

No concerns from my side, thank you for your comments and support!
aucampia: @RDFLib/core I will merge this by 2022-05-17 if there is no further feedback.
sonarcloud[bot]: SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=RDFLib_rdflib&pullRequest=1894)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=SECURITY_HOTSPOT) [![E](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E-16px.png 'E')](https://sonarcloud.io/project/security_hotspots?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=SECURITY_HOTSPOT) [5 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=RDFLib_rdflib&pullRequest=1894&resolved=false&types=CODE_SMELL)

[![No Coverage information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo-16px.png 'No Coverage information')](https://sonarcloud.io/component_measures?id=RDFLib_rdflib&pullRequest=1894) No Coverage information  
[![2.8%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.8%')](https://sonarcloud.io/component_measures?id=RDFLib_rdflib&pullRequest=1894&metric=new_duplicated_lines_density&view=list) [2.8% Duplication](https://sonarcloud.io/component_measures?id=RDFLib_rdflib&pullRequest=1894&metric=new_duplicated_lines_density&view=list)



Patch:
diff --git a/rdflib/plugins/sparql/evaluate.py b/rdflib/plugins/sparql/evaluate.py
index edd322e5..49bff943 100644
--- a/rdflib/plugins/sparql/evaluate.py
+++ b/rdflib/plugins/sparql/evaluate.py
@@ -405,18 +405,26 @@ def _yieldBindingsFromServiceCallResult(
     res_dict: Dict[Variable, Identifier] = {}
     for var in variables:
         if var in r and r[var]:
-            if r[var]["type"] == "uri":
-                res_dict[Variable(var)] = URIRef(r[var]["value"])
-            elif r[var]["type"] == "bnode":
-                res_dict[Variable(var)] = BNode(r[var]["value"])
-            elif r[var]["type"] == "literal" and "datatype" in r[var]:
+            var_binding = r[var]
+            var_type = var_binding["type"]
+            if var_type == "uri":
+                res_dict[Variable(var)] = URIRef(var_binding["value"])
+            elif var_type == "literal":
                 res_dict[Variable(var)] = Literal(
-                    r[var]["value"], datatype=r[var]["datatype"]
+                    var_binding["value"],
+                    datatype=var_binding.get("datatype"),
+                    lang=var_binding.get("xml:lang"),
                 )
-            elif r[var]["type"] == "literal" and "xml:lang" in r[var]:
+            # This is here because of
+            # https://www.w3.org/TR/2006/NOTE-rdf-sparql-json-res-20061004/#variable-binding-results
+            elif var_type == "typed-literal":
                 res_dict[Variable(var)] = Literal(
-                    r[var]["value"], lang=r[var]["xml:lang"]
+                    var_binding["value"], datatype=URIRef(var_binding["datatype"])
                 )
+            elif var_type == "bnode":
+                res_dict[Variable(var)] = BNode(var_binding["value"])
+            else:
+                raise ValueError(f"invalid type {var_type!r} for variable {var!r}")
     yield FrozenBindings(ctx, res_dict)
 
 


Test patch:
diff --git a/test/test_sparql/test_service.py b/test/test_sparql/test_service.py
index 9798f3df..31ba6e8a 100644
--- a/test/test_sparql/test_service.py
+++ b/test/test_sparql/test_service.py
@@ -1,8 +1,31 @@
+import json
+from contextlib import ExitStack
 from test.utils import helper
+from test.utils.httpservermock import (
+    MethodName,
+    MockHTTPResponse,
+    ServedBaseHTTPServerMock,
+)
+from typing import (
+    Dict,
+    FrozenSet,
+    Generator,
+    List,
+    Mapping,
+    Optional,
+    Sequence,
+    Tuple,
+    Type,
+    Union,
+)
+
+import pytest
 
 from rdflib import Graph, Literal, URIRef, Variable
 from rdflib.compare import isomorphic
+from rdflib.namespace import XSD
 from rdflib.plugins.sparql import prepareQuery
+from rdflib.term import BNode, Identifier
 
 
 def test_service():
@@ -135,13 +158,171 @@ def test_service_with_implicit_select_and_allcaps():
     assert len(results) == 3
 
 
-# def test_with_fixture(httpserver):
-#    httpserver.expect_request("/sparql/?query=SELECT * WHERE ?s ?p ?o").respond_with_json({"vars": ["s","p","o"], "bindings":[]})
-#    test_server = httpserver.url_for('/sparql')
-#    g = Graph()
-#    q = 'SELECT * WHERE {SERVICE <'+test_server+'>{?s ?p ?o} . ?s ?p ?o .}'
-#    results = g.query(q)
-#   assert len(results) == 0
+def freeze_bindings(
+    bindings: Sequence[Mapping[Variable, Identifier]]
+) -> FrozenSet[FrozenSet[Tuple[Variable, Identifier]]]:
+    result = []
+    for binding in bindings:
+        result.append(frozenset(((key, value)) for key, value in binding.items()))
+    return frozenset(result)
+
+
+def test_simple_not_null():
+    """Test service returns simple literals not as NULL.
+
+    Issue: https://github.com/RDFLib/rdflib/issues/1278
+    """
+
+    g = Graph()
+    q = """SELECT ?s ?p ?o
+WHERE {
+    SERVICE <https://DBpedia.org/sparql> {
+        VALUES (?s ?p ?o) {(<http://example.org/a> <http://example.org/b> "c")}
+    }
+}"""
+    results = helper.query_with_retry(g, q)
+    assert results.bindings[0].get(Variable("o")) == Literal("c")
+
+
+def test_service_node_types():
+    """Test if SERVICE properly returns different types of nodes:
+    - URI;
+    - Simple Literal;
+    - Literal with datatype ;
+    - Literal with language tag .
+    """
+
+    g = Graph()
+    q = """
+SELECT ?o
+WHERE {
+    SERVICE <https://dbpedia.org/sparql> {
+        VALUES (?s ?p ?o) {
+            (<http://example.org/a> <http://example.org/uri> <http://example.org/URI>)
+            (<http://example.org/a> <http://example.org/simpleLiteral> "Simple Literal")
+            (<http://example.org/a> <http://example.org/dataType> "String Literal"^^xsd:string)
+            (<http://example.org/a> <http://example.org/language> "String Language"@en)
+            (<http://example.org/a> <http://example.org/language> "String Language"@en)
+        }
+    }
+    FILTER( ?o IN (<http://example.org/URI>, "Simple Literal", "String Literal"^^xsd:string, "String Language"@en) )
+}"""
+    results = helper.query_with_retry(g, q)
+
+    expected = freeze_bindings(
+        [
+            {Variable('o'): URIRef('http://example.org/URI')},
+            {Variable('o'): Literal('Simple Literal')},
+            {
+                Variable('o'): Literal(
+                    'String Literal',
+                    datatype=URIRef('http://www.w3.org/2001/XMLSchema#string'),
+                )
+            },
+            {Variable('o'): Literal('String Language', lang='en')},
+        ]
+    )
+    assert expected == freeze_bindings(results.bindings)
+
+
+@pytest.fixture(scope="module")
+def module_httpmock() -> Generator[ServedBaseHTTPServerMock, None, None]:
+    with ServedBaseHTTPServerMock() as httpmock:
+        yield httpmock
+
+
+@pytest.fixture(scope="function")
+def httpmock(
+    module_httpmock: ServedBaseHTTPServerMock,
+) -> Generator[ServedBaseHTTPServerMock, None, None]:
+    module_httpmock.reset()
+    yield module_httpmock
+
+
+@pytest.mark.parametrize(
+    ("response_bindings", "expected_result"),
+    [
+        (
+            [
+                {"type": "uri", "value": "http://example.org/uri"},
+                {"type": "literal", "value": "literal without type or lang"},
+                {"type": "literal", "value": "literal with lang", "xml:lang": "en"},
+                {
+                    "type": "typed-literal",
+                    "value": "typed-literal with datatype",
+                    "datatype": f"{XSD.string}",
+                },
+                {
+                    "type": "literal",
+                    "value": "literal with datatype",
+                    "datatype": f"{XSD.string}",
+                },
+                {"type": "bnode", "value": "ohci6Te6aidooNgo"},
+            ],
+            [
+                URIRef('http://example.org/uri'),
+                Literal('literal without type or lang'),
+                Literal('literal with lang', lang='en'),
+                Literal(
+                    'typed-literal with datatype',
+                    datatype=URIRef('http://www.w3.org/2001/XMLSchema#string'),
+                ),
+                Literal('literal with datatype', datatype=XSD.string),
+                BNode('ohci6Te6aidooNgo'),
+            ],
+        ),
+        (
+            [
+                {"type": "invalid-type"},
+            ],
+            ValueError,
+        ),
+    ],
+)
+def test_with_mock(
+    httpmock: ServedBaseHTTPServerMock,
+    response_bindings: List[Dict[str, str]],
+    expected_result: Union[List[Identifier], Type[Exception]],
+) -> None:
+    """
+    This tests that bindings for a variable named var
+    """
+    graph = Graph()
+    query = """
+    PREFIX ex: <http://example.org/>
+    SELECT ?var
+    WHERE {
+        SERVICE <REMOTE_URL> {
+            ex:s ex:p ?var
+        }
+    }
+    """
+    query = query.replace("REMOTE_URL", httpmock.url)
+    response = {
+        "head": {"vars": ["var"]},
+        "results": {"bindings": [{"var": item} for item in response_bindings]},
+    }
+    httpmock.responses[MethodName.GET].append(
+        MockHTTPResponse(
+            200,
+            "OK",
+            json.dumps(response).encode("utf-8"),
+            {"Content-Type": ["application/sparql-results+json"]},
+        )
+    )
+    catcher: Optional[pytest.ExceptionInfo[Exception]] = None
+
+    with ExitStack() as xstack:
+        if isinstance(expected_result, type) and issubclass(expected_result, Exception):
+            catcher = xstack.enter_context(pytest.raises(expected_result))
+        else:
+            expected_bindings = [{Variable("var"): item} for item in expected_result]
+        bindings = graph.query(query).bindings
+    if catcher is not None:
+        assert catcher is not None
+        assert catcher.value is not None
+    else:
+        assert expected_bindings == bindings
 
 
 if __name__ == "__main__":
@@ -151,3 +332,4 @@ if __name__ == "__main__":
     test_service_with_implicit_select()
     test_service_with_implicit_select_and_prefix()
     test_service_with_implicit_select_and_base()
+    test_service_node_types()