rdfjs / N3.js

Lightning fast, spec-compatible, streaming RDF for JavaScript
http://rdf.js.org/N3.js/
Other
676 stars 127 forks source link

`termFromId` and `termToId` do not roundtrip on quoted literals #312

Closed jeswr closed 1 year ago

jeswr commented 1 year ago

As demonstrated by this failing test doing a round-trip of termFromId and termToId causes Literal TermTypes to become NamedNode term types because the internal id without literals gets used in https://github.com/rdfjs/N3.js/blob/520054a9fb45ef48b5b58851449942493c57dace/src/N3DataFactory.js#L236 and the result is that termFromId interprets it as a namedNode because it does not contain quotes.

We need to either (a) add quotes to literals missing them when Literals are created, or (b) remove https://github.com/rdfjs/N3.js/blob/520054a9fb45ef48b5b58851449942493c57dace/src/N3DataFactory.js#L236 which is the optimisation causing the bug.

Note that NamedNodes can also become literals if we create a NamedNode of the form new NamedNode('"s"')

RubenVerborgh commented 1 year ago

I think the problem is in the test: new Literal('abc') is not meaningful, because the (internal) Literal constructor (which is the Term constructor) requires a correct id as argument. This can also be seen here: https://github.com/rdfjs/N3.js/compare/main...jeswr:N3.js:bug/literal-roundtrip-failure#diff-8dc308ca806460d3608d347268016fb538008d8f9926902f331cc3553ababbfaR228

Note that this differs from the exported literal function (with which the test would succeed): https://github.com/rdfjs/N3.js/blob/520054a9fb45ef48b5b58851449942493c57dace/src/N3DataFactory.js#L339-L342

jeswr commented 1 year ago

because the (internal) Literal constructor

These constructors are actually exported (c.f. https://github.com/rdfjs/N3.js/blob/520054a9fb45ef48b5b58851449942493c57dace/src/index.js#L38-L39). Perhaps it then worth removing these exports in the next mver (which I believe will be required in https://github.com/rdfjs/N3.js/pull/311 anyway since there are a couple of minor breaking changes required to migrate from RDF* -> RDF-star)?

RubenVerborgh commented 1 year ago

Good idea, following up in https://github.com/rdfjs/N3.js/issues/313