Fix simple literals returned as NULL using SERVICE (issue #1278) by gitmpje · Pull Request #1894 · RDFLib/rdflib (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits14 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Summary of changes

Fixes #1278 simple literals returned as NULL. The resolution uses same logic as here:

def parseJsonTerm(d):
"""rdflib object (Literal, URIRef, BNode) for the given json-format dict.

input is like:
{ 'type': 'uri', 'value': 'http://famegame.com/2006/01/username' }
{ 'type': 'literal', 'value': 'drewp' }
"""

t = d["type"]
if t == "uri":
return URIRef(d["value"])
elif t == "literal":
return Literal(d["value"], datatype=d.get("datatype"), lang=d.get("xml:lang"))
elif t == "typed-literal":
return Literal(d["value"], datatype=URIRef(d["datatype"]))
elif t == "bnode":
return BNode(d["value"])
else:
raise NotImplementedError("json term type %r" % t)

Checklist

Checked that there aren't other open pull requests for
the same change.
Added tests for any changes that have a runtime impact.
Checked that all tests and type checking passes.
Considered granting push permissions to the PR branch,
so maintainers can fix minor issues and keep your PR up to date.

Mark van der Pas added 2 commits

May 8, 2022 11:15

… sparql/results/jsonresults parser.

gitmpje changed the title~~Fix simple literals returned as NULL using SERVICE (issue 1278)~~ Fix simple literals returned as NULL using SERVICE (issue #1278)

May 8, 2022

Mark van der Pas added 3 commits

May 8, 2022 11:32

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, a bit swamped fixing bugs and reviewing/fixing older PRs right now so I won't be able to take a detailed look until later, but it would be good to expand testing for SERVICE directive to ensure that each branch here is covered, which I'm not sure it will be, though I may be wrong.

Also it seems your test is failing: https://github.com/RDFLib/rdflib/runs/6341689732?check_suite_focus=true#step:9:372

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

aucampia dismissed their stale review

May 8, 2022 15:06

Accidentally clicked approve instead of comment.

Coverage increased (+0.02%) to 88.29% when pulling 6bd50f0 on gitmpje:issue1278_null_literals into 246c887 on RDFLib:master.

Mark van der Pas added 2 commits

May 11, 2022 16:59

I added a more general test for different node types returned by a SERVICE clause to test_service.py.

@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.

Also make existing tests stricter.

Use longer variable names so it is easier to read
Add type hints
Add comment explaining the reason for typed-literal
Improved exception in case of invalid/unsupported type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@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!

@RDFLib/core I will merge this by 2022-05-17 if there is no further feedback.

@ghost ghost removed the review wanted

This indicates that the PR is ready for review

label

Jun 4, 2022