Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix simple literals returned as NULL using SERVICE (issue #1278) #1894

Merged
merged 14 commits into from May 15, 2022
Merged

Fix simple literals returned as NULL using SERVICE (issue #1278) #1894

merged 14 commits into from May 15, 2022

Conversation

gitmpje
Copy link
Contributor

@gitmpje gitmpje commented May 8, 2022

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.

@gitmpje 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
aucampia
aucampia previously approved these changes May 8, 2022
Copy link
Member

@aucampia aucampia left a comment

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.

test/test_issues/test_issue1278.py Outdated Show resolved Hide resolved
@aucampia
Copy link
Member

aucampia commented May 8, 2022

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 aucampia dismissed their stale review May 8, 2022 15:06

Accidentally clicked approve instead of comment.

@gitmpje
Copy link
Contributor Author

gitmpje commented May 8, 2022

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
Copy link

coveralls commented May 8, 2022

Coverage Status

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

@aucampia
Copy link
Member

aucampia commented May 8, 2022

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
Copy link
Contributor Author

gitmpje commented May 11, 2022

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

@aucampia aucampia self-assigned this May 12, 2022
@aucampia
Copy link
Member

@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.
Copy link
Member

@aucampia aucampia left a comment

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.

@aucampia aucampia requested a review from a team May 12, 2022 20:41
@aucampia aucampia added the review wanted This indicates that the PR is ready for review label May 12, 2022
@gitmpje
Copy link
Contributor Author

gitmpje commented May 13, 2022

@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 aucampia removed their assignment May 15, 2022
@aucampia
Copy link
Member

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

@sonarcloud
Copy link

sonarcloud bot commented May 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
2.8% 2.8% Duplication

@aucampia aucampia merged commit e9908b4 into RDFLib:master May 15, 2022
@ghost ghost removed the review wanted This indicates that the PR is ready for review label Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In using SERVICE, "string" variables get retrieved as NULL
3 participants