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: two issues with the N3 serializer #1858

Merged

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Apr 22, 2022

Summary of changes

This patch fixes two issues with the N3 serializer:

  • The N3 serializer incorrectly considered a subject as already
    serialized if it has been serialized inside a quoted graph.
  • The N3 serializer does not consider that the predicate of
    a triple can also be a graph.

Other changes included in this patch:

  • Changed test.testutils.GraphHelper to support nested/quoted graphs.
  • Moved the tests from test/test_n3_formula.py into
    test/test_serializers/test_serializer_n3.py.
  • Include positive syntax tests from the N3 test suite that is smaller
    than 1024KB and that is not using new N3 syntax into round trip tests.
    This is mainly to check that there is no regressions after the changes
    made.

Fixes:

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.

@aucampia aucampia marked this pull request as draft April 22, 2022 17:41
@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch 4 times, most recently from d57d967 to e8202e0 Compare April 23, 2022 19:48
@aucampia aucampia changed the title Fix the N3 serializer fix: two issues with the N3 serializer Apr 23, 2022
@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch 2 times, most recently from 8068241 to 05692e7 Compare April 23, 2022 19:57
@aucampia aucampia marked this pull request as ready for review April 23, 2022 20:00
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The N3 serializer does not consider that the predicate of a triple can also be a graph.

It can? Eww. Excellent catch.

@aucampia
Copy link
Member Author

aucampia commented Apr 24, 2022

The N3 serializer does not consider that the predicate of a triple can also be a graph.

It can? Eww. Excellent catch.

It is a bit odd yes, noticed it when I ran roundtrip for this test:

https://github.com/w3c/N3/blob/c44d123c5958ca04117e28ca3769e2c0820f72e6/tests/N3Tests/manifest-parser.ttl#L1097-L1102

:cwm_syntax_neg-formula-predicate.n3
        a              test:TestN3PositiveSyntax ;
        mf:action      <cwm_syntax/neg-formula-predicate.n3> ;
        mf:name        "neg-formula-predicate.n3" ;
        rdfs:comment   "A predicate cannot be a formula, I hope ; yes it can!";
        rdft:approval  rdft:Approved .

File here:

https://github.com/w3c/N3/blob/c44d123c5958ca04117e28ca3769e2c0820f72e6/tests/N3Tests/cwm_syntax/neg-formula-predicate.n3

:a {:b :c :d} :e .

And the only reason it failed round tripping is because the namespace was not being serialized for the quad in the middle, with this fix the reconstituted graph looks like this:

DEBUG    test.test_roundtrip:test_roundtrip.py:237 source = /home/iwana/sw/d/github.com/iafork/rdflib.cleanish/test/data/suites/w3c/n3/N3Tests/cwm_syntax/neg-formula-predicate.n3, serailized = 
@prefix ns1: <file:///home/iwana/sw/d/github.com/iafork/rdflib.cleanish/test/data/suites/w3c/n3/N3Tests/cwm_syntax/neg-formula-predicate.n3#> .

ns1:a{
    ns1:b ns1:c ns1:d .

} ns1:e .

Without it:

DEBUG    test.test_roundtrip:test_roundtrip.py:237 source = /home/iwana/sw/d/github.com/iafork/rdflib.cleanish/test/data/suites/w3c/n3/N3Tests/cwm_syntax/neg-formula-predicate.n3, serailized = 

<file:///home/iwana/sw/d/github.com/iafork/rdflib.cleanish/test/data/suites/w3c/n3/N3Tests/cwm_syntax/neg-formula-predicate.n3#a>{
    ns1:b ns1:c ns1:d .

} ns1:e .

Had to double check the grammar to believe it also:

https://github.com/w3c/N3/blob/master/grammar/n3.ebnf

[14] predicate                          ::= (expression | '<-' expression)
                                            /* allow inverting first predicate in a path */
[16] expression                         ::= path

[17] path	                              ::= pathItem ('!' path | '^' path)?

[18] pathItem                           ::= iri
                                          | blankNode
                                          | quickVar
                                          | collection
                                          | blankNodePropertyList
                                          | literal
                                          | formula

[22] formula                            ::= '{' formulaContent? '}'

[23] formulaContent                     ::= n3Statement ('.' formulaContent?)?
                                          | sparqlDirective formulaContent?

@ghost
Copy link

ghost commented Apr 24, 2022

The comment suggests it's perhaps not intended to be that way but is an entailment of the EBNF expression.

rdfs:comment   "A predicate cannot be a formula, I hope ; yes it can!";

@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch 3 times, most recently from 315bbe5 to 5b2c826 Compare April 24, 2022 15:38
@aucampia aucampia added format: N3 Related to N3 format. review wanted This indicates that the PR is ready for review labels Apr 25, 2022
@aucampia
Copy link
Member Author

@eikeon not sure if you are familiar with the N3 serializer but if you are please have a look, I'm fairly confident about this change but maybe I'm missing something here also.

@aucampia
Copy link
Member Author

@blackwint3r this ideally needs one more review, please consider doing a review if you have time.

@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch from 5b2c826 to 277af70 Compare April 30, 2022 21:34
@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch from 277af70 to 50b1746 Compare May 3, 2022 20:10
@coveralls
Copy link

coveralls commented May 3, 2022

Coverage Status

Coverage increased (+0.007%) to 88.255% when pulling f38979e on aucampia:iwana-20220419T2343-n3_serialize_quoted_graph into 5e5dcea on RDFLib:master.

@aucampia
Copy link
Member Author

aucampia commented May 4, 2022

resolving conflicts

@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch from 50b1746 to 1f0641f Compare May 4, 2022 21:12
@aucampia aucampia mentioned this pull request May 8, 2022
4 tasks
This patch fixes two issues with the N3 serializer:
- The N3 serializer incorrectly considered a subject as already
  serialized if it has been serialized inside a quoted graph.
- The N3 serializer does not consider that the predicate of
  a triple can also be a graph.

Other changes included in this patch:
- Changed `test.testutils.GraphHelper` to support nested/quoted graphs.
- Moved the tests from `test/test_n3_formula.py` into
  `test/test_serializers/test_serializer_n3.py`.
- Include positive syntax tests from the N3 test suite that is smaller
  than 1024KB and that is not using new N3 syntax into round trip tests.
  This is mainly to check that there is no regressions after the changes
  made.

Fixes:
- RDFLib#1807
- RDFLib#1701
@aucampia aucampia force-pushed the iwana-20220419T2343-n3_serialize_quoted_graph branch from 1f0641f to f38979e Compare May 12, 2022 21:44
@aucampia
Copy link
Member Author

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

@aucampia
Copy link
Member Author

Merging this with only one approval as it is a fairly simple fix and includes extensive testing.

@aucampia aucampia merged commit b4f779b into RDFLib:master May 16, 2022
@ghost ghost removed the review wanted This indicates that the PR is ready for review label Jun 4, 2022
@aucampia aucampia deleted the iwana-20220419T2343-n3_serialize_quoted_graph branch June 4, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format: N3 Related to N3 format.
Projects
None yet
2 participants