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.

Sign up for GitHub

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

Jump to bottom

.set("val").sparql may result in DELETE DATA query with variables #352

Open
andrewzhurov opened this issue May 10, 2023 · 6 comments · May be fixed by #353
Open

.set("val").sparql may result in DELETE DATA query with variables #352

andrewzhurov opened this issue May 10, 2023 · 6 comments · May be fixed by #353

Comments

@andrewzhurov
Copy link

Paths, such as:

await path.create("http://example.com/subj1")
          .somePredicate
          .set("new value")
          .sparql

May result in DELETE DATA queries with variables, such as:

DELETE DATA {
  <http://example.com/subj1> <http://example.com/somePredicate> ?somePredicate.
}
;
INSERT DATA {
  <http://example.com/subj1> <http://example.com/somePredicate> "new value".
}

Which is okay by LDFlex, as seen in this test.

However, as per the spec, variables are not allowed in DELETE DATA queries, and other query engines that are more strictly conformant to the spec may fail to execute such queries, e.g., Comunica fails with:

SparqlParser.js:1193 Uncaught (in promise) Error: Detected illegal variable in BGP
    at Yr (SparqlParser.js:1193:19)
    at Object.performAction (SparqlParser.js:336:62)
    at tn.parse (SparqlParser.js:787:36)
    at Parser.d.parse (sparql.js:36:37)
    at s.run (ActorQueryParseSparql.ts:27:33)
    at s.runObservable (Actor.ts:83:37)
    at i.mediate (Mediator.ts:86:18)
    at async a.queryOrExplain (QueryEngineBase.ts:165:13)
    at async a.query (QueryEngineBase.ts:87:11)
    at async a.queryOfType (QueryEngineBase.ts:70:11)

Perhaps having such paths result in DELETE ... WHERE ... would make LDFlex more conformant with the spec and allow their execution by other engines.

Environment:
"@ldflex/comunica": "^5.0.1",
"ldflex": "^2.15.2",
"@comunica/query-sparql": "^2.6.9",
@jeswr
Copy link
Member

jeswr commented May 10, 2023

Thanks for creating the issue! Happy to accept a PR which makes sure a DELETE ... WHERE ... query is used when variables are present :)

@andrewzhurov
Copy link
Author

Hi, Jesse!
I wouldn't mind fixing it, for that I'd need to adjust the test, for that I'd need a working test workflow, and it fails for me at this moment. Same as on the CI. Any estimates on when it'll get patched?

@jeswr
Copy link
Member

jeswr commented May 23, 2023

I'm happy for you to branch off this commit where CI is passing. The subsequent dependabot commits need to be reverted as the upgrade to @rdfjs/data-model v2 (which is ESM only) is what broke things in the first place.

@jeswr
Copy link
Member

jeswr commented May 23, 2023

Once you create a PR I'll revert those commits on main, and manually do relevant upgrades once merging your work.

@andrewzhurov
Copy link
Author

andrewzhurov commented May 23, 2023 •
edited

From what I reckon also vars are disalowed in INSERT DATA.
However LDflex already guards againts it, so nevermind that.

@jeswr
Copy link
Member

jeswr commented May 23, 2023 •
edited

However LDflex already guards againts it, so nevermind that.

To elaborate on the reason why it guards against INSERT and not DELETE.

The statement DELETE WHERE { ?s a ?o } is valid SPARQL which will delete all of the type statements in my knowledge base (note that this is just syntactical shorthand for DELETE { ?s a ?o } WHERE { ?s a ?o }). I would recommend you use the short form in your PR

On the other hand INSERT WHERE { ?s a ?o } is invalid SPARQL; and this makes sense because I'm just going to try and insert already existing statements in my knowledge base (take all the type statements in my knowledge base and add them again). So INSERT WHERE { ... } is just a noop and thus it doesn't make sense.

@andrewzhurov andrewzhurov linked a pull request May 24, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix SparqlHandler to refrain from returning DELETE DATA with variables andrewzhurov/LDflex
2 participants
@andrewzhurov @jeswr