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

Harden SPARQLConnection.commit() against empty transactions #1960

Closed
knoan opened this issue Mar 2, 2020 · 4 comments
Closed

Harden SPARQLConnection.commit() against empty transactions #1960

knoan opened this issue Mar 2, 2020 · 4 comments
Assignees
Labels
bug
Milestone

Comments

@knoan
Copy link

@knoan knoan commented Mar 2, 2020

How to Reproduce

Execute the following snippet against a SPARQL 1.1 Update Endpoint.

final SPARQLRepository repository=new SPARQLRepository("{query/update endpoint}");

repository.init();

try (final RepositoryConnection connection=repository.getConnection()) {

	connection.begin();

	connection.prepareUpdate(
			"prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#>\n"
			+"\n"
			+"delete data { rdf:nil rdf:value rdf:nil }"
	).execute();

	connection.commit();

} finally {
	repository.shutDown();
}

Expected Results

Update silently executed.

Actual Results

Exception is possibly thrown.

Notes

  • updates on SPARQL endpoints are immediately executed rather than queued to the current transaction (see org.eclipse.rdf4j.repository.sparql.query.SPARQLUpdate:41)
  • on commit an empty update is submitted to the endpoint (see org.eclipse.rdf4j.repository.sparql.SPARQLConnection:394) and if the endpoint doesn't gracefully handle the event an exception is thrown
  • works as expected when replacing the SPARQL update with the functionally equivalent connection.remove(RDF.NIL, RDF.VALUE, RDF.NIL)
  • removing transaction begin/commit is not an option in a real world setting, as the offending code is not necessarily aware of the specific type of the target repository
@knoan

This comment has been minimized.

Copy link
Author

@knoan knoan commented Mar 2, 2020

P.S. Possibly relevant also for other repository types

P.P.S. as a general remark, I think read-only transactions should be supported by all repositories and gracefully committed as a no-op

@hmottestad

This comment has been minimized.

Copy link
Contributor

@hmottestad hmottestad commented Mar 6, 2020

Exception is possibly thrown.

Are exceptions always thrown, or just sometimes? Which exceptions have you seen?

@knoan

This comment has been minimized.

Copy link
Author

@knoan knoan commented Mar 6, 2020

Are exceptions always thrown, or just sometimes? Which exceptions have you seen?

It depends on the endpoint: some will simply ignore the empty update, while other will respond with something like 400 Bad Request, causing an exception to be always thrown.

@jeenbroekstra jeenbroekstra added the bug label Mar 8, 2020
@jeenbroekstra jeenbroekstra added this to To do in Next releases via automation Mar 8, 2020
@jeenbroekstra

This comment has been minimized.

Copy link
Contributor

@jeenbroekstra jeenbroekstra commented Mar 8, 2020

Good find, thanks. I think there are two separate (though obviously related) issues here: the first is the fact that an empty commit string should not be transmitted, the second being that a SPARQL update that is part of a transaction should not be immediately executed, but should be appended. I'll separate that second problem out into its own ticket.

@jeenbroekstra jeenbroekstra self-assigned this Mar 8, 2020
@jeenbroekstra jeenbroekstra added this to the 3.1.2 milestone Mar 8, 2020
@jeenbroekstra jeenbroekstra moved this from To do to In progress in Next releases Mar 8, 2020
jeenbroekstra added a commit that referenced this issue Mar 9, 2020
jeenbroekstra added a commit that referenced this issue Mar 9, 2020
- check if txn string has content before sending update
- added regression test
- minor refactor: use StringBuilder instead of StringBuffer
Next releases automation moved this from In progress to Done Mar 10, 2020
jeenbroekstra added a commit that referenced this issue Mar 10, 2020
- check if txn string has content before sending update
- added regression test
- minor refactor: use StringBuilder instead of StringBuffer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Next releases
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.