-
Notifications
You must be signed in to change notification settings - Fork 18
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 SparqlHandler to refrain from returning DELETE DATA with variables #353
base: master
Are you sure you want to change the base?
Conversation
@jeswr Let me know if there are any adjustments you'd like to have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work @andrewzhurov ! Just have a couple of comments.
mutationType, | ||
// Add DATA clause if there are no variables in mutation expression, | ||
// that is so when subject is specified (not coming from a WHERE clause) and all predicateObjects have predicates and objects | ||
where.length === 0 && arePredicateObjectsSpecified(predicateObjects) ? ' DATA ' : ' ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC there is nothing stopping us from instantiating the subject as a variable - can you check this and if so also include the DATA
when the subject is a variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I gather, according to the spec INSERT DATA must not allow variables in quad patterns, so when there is a subject variable it's not INSERT DATA but INSERT.
When there are no where
and predicate objects are specified - there are no vars and we're safe to use INSERT DATA, it seems.
@@ -82,13 +82,19 @@ export default class SparqlHandler { | |||
mutations.push(...this.triplePatterns(subject, predicate, objectStrings, reverse)); | |||
} | |||
const mutationClauses = `{\n ${mutations.join('\n ')}\n}`; | |||
function arePredicateObjectsSpecified(predicateObjects2) { | |||
return predicateObjects2.filter(({ predicate, objects }) => objects === null || predicate === null).length === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return predicateObjects2.filter(({ predicate, objects }) => objects === null || predicate === null).length === 0; | |
return predicateObjects2.some(({ predicate, objects }) => objects === null || predicate === null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a function rather than just assigning const variablePredicateOrObject = predicateObjects.some(({ predicate, objects }) => objects === null || predicate === null);
and using that const
later on
Furthermore objects
is a list so you may need to check if each element in that array could be a variable. I'm not currently in a position to check for myself if it is possible for those objects to be variables - so could you quickly investigate this yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a function and not a const in order to not execute it ahead of time, but only after where.length === 0 predicate passes, to give us a bit of performance. And it's a function and not an inline expression to give a bit of readability.
There is some
! Gotta use that. JS is not my first language.:)
I'll give objects
a look, thanks for showing the gotcha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding objects
, it seems we either have them undefined altogether, or values present (a value cannot be undefined).
An expression like this
which is, interestingly, consider invalid by sparql.js
Although it seems to be valid, according to the spec |
Closes #352