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

Fix SparqlHandler to refrain from returning DELETE DATA with variables #353

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix SparqlHandler to refrain from returning DELETE DATA with variables #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/SparqlHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return predicateObjects2.filter(({ predicate, objects }) => objects === null || predicate === null).length === 0;
return predicateObjects2.some(({ predicate, objects }) => objects === null || predicate === null);

Copy link
Member

@jeswr jeswr May 24, 2023 •
edited

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?

Copy link
Author

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

Copy link
Author

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).

}

// Join clauses into a SPARQL query
return where.length === 0 ?
// If there are no WHERE clauses, just mutate raw data
`${mutationType} DATA ${mutationClauses}` :
// Otherwise, return a DELETE/INSERT ... WHERE ... query
`${mutationType} ${mutationClauses} WHERE {\n ${where.join('\n ')}\n}`;
return [
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 ' : ' ',
Copy link
Member

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.

Copy link
Author

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.

mutationClauses,
// Add WHERE clauses, if any
where.length === 0 ? '' : ` WHERE {\n ${where.join('\n ')}\n}`,
].join('');
}

expressionToTriplePatterns([root, ...pathExpression], lastVar, scope = {}) {
Expand Down
2 changes: 1 addition & 1 deletion test/integration/sparql-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ describe('a query path with a path expression handler', () => {
it('resolves a path where an object map has different conditions', async () => {
const query = await person.delete({ friends: null, firstName: 'Ruben' }).sparql;
expect(query).toEqual(deindent(`
DELETE DATA {
DELETE {
<https://example.org/#me> <${FOAF}knows> ?knows.
<https://example.org/#me> <${FOAF}givenName> "Ruben".
}`));
Expand Down
2 changes: 1 addition & 1 deletion test/unit/SparqlHandler-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ describe('a SparqlHandler instance', () => {
];

expect(await handler.handle({}, { mutationExpressions })).toEqual(deindent(`
DELETE DATA {
DELETE {
<https://example.org/#D0> <https://example.org/#Dp1> ?Dp1.
}`));
});
Expand Down