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

Clear defaultGraph clears all graphs when using a remote repository #4126

Closed
maxstolze opened this issue Aug 24, 2022 · 8 comments · Fixed by #4127
Closed

Clear defaultGraph clears all graphs when using a remote repository #4126

maxstolze opened this issue Aug 24, 2022 · 8 comments · Fixed by #4127
Assignees
Labels
🐞 bug issue is a bug
Milestone

Comments

@maxstolze
Copy link
Contributor

maxstolze commented Aug 24, 2022

Current Behavior

We used con.clear((org.eclipse.rdf4j.model.Resource) null); to clear the defaultGraph.
Now this clears all graphs in the repository

Only happens with remote repositories. No problem with an in memory store.

Expected Behavior

Only clear defaultGraph.

Steps To Reproduce

Using RDF4J 4.1.0 with:

  • GraphDB 10.0.0, 10.0.1 & 10.0.2
  • Native- & MemoryStore on RDF4J Server 4.1.0

Clear with: con.clear((org.eclipse.rdf4j.model.Resource) null);

Version

4.1.0

Are you interested in contributing a solution yourself?

No response

Anything else?

No response

@maxstolze maxstolze added the 🐞 bug issue is a bug label Aug 24, 2022
@hmottestad hmottestad self-assigned this Aug 25, 2022
hmottestad added a commit that referenced this issue Aug 25, 2022
@hmottestad
Copy link
Contributor

@maxstolze did this work before?

@fkleedorfer
Copy link
Contributor

I am pretty certain that the follwing used to work prior to 4.x

con.clear(new org.eclipse.rdf4j.model.Resource[] {null});

Not so sure about

con.clear((org.eclipse.rdf4j.model.Resource) null);

(it's a suggestion made by @jeenbroekstra in gitter, which was the last thing we tried before we gave up and it made its way into the issue by accident)

We don't really care about the regression, if it even is one, but we cannot seem to figure out how to clear just the default graph in a remote repo

@hmottestad
Copy link
Contributor

I've tried both myself without getting it to work either. I don't think there actually is any difference between the two method calls, they both resolve to an array with a null element.

I've tried 4.1.0, 4.0.4, 4.0.0 and 3.7.7. None of these work.

If you could find out which version that this used to work for it would be very helpful for me. Testing this is very time consuming for me. I need to know if this used to work before so that I can see what has changed in the code.

@fkleedorfer
Copy link
Contributor

When we developed code using this, we were using 3.7.0. We switched directly to 4.0.4 from that.

I am pretty sure it used to work with remote repos, but I cannot guarantee it. There is a small chance that we overlooked the problem. (We've used this code in the preparation of test cases since January 2022, and in our setup, we can run the tests against a remote repo only manually on the developer machine - the fully automated tests run agains an in-memory db. However, we do run them against the remote repo manually every now and then, and I do not recall having this problem until we switched to 4.x. - recently)

@abrokenjester
Copy link
Contributor

This should work and I'm fairly certain it has worked in the past. I had a look though and I think the problem may have been introduced quite a while back, when we switched the implementation of the HTTPRepositoryConnection over from using the legacy handling of clear operations to using the extended transaction protocol. See HTTPRepositoryConnection.clear lines 665 and further.

If I read it through a bit further it converts the clear to just a statement removal operation using wildcards, which eventually is then sent to the server as a serialized binary RDF document (see removeModel, line 557 and further). As far as I can tell that serialization neglects to take the case of the null context into account so it sends up just sending a REMOVE(wildcard, wildcard, wildcard) operation to the server.

Quite a serious gap in our test coverage to be honest.

The fix should probably be added to HTTPRepositoryConnection#removeWithoutCommit - at that point the null input param for the context should probably be intercepted and replaced an RDF4J.NIL token. I haven't checked the server side but i expect that is what we used to use for this kind of thing over HTTP.

@hmottestad
Copy link
Contributor

I've created a branch with some tests and a fix which uses RDF4J.NIL. Works well enough, but there is a flushing issue for transactions that clear a graph and then tries to retrieve all statements.

#4127

@hmottestad
Copy link
Contributor

It's really hard to debug those tests though since you can't add a break point to the server code. At these times I really wish we were using Spring Boot because then we would be able to debug both the client and the server code from a single test.

@abrokenjester
Copy link
Contributor

It's really hard to debug those tests though since you can't add a break point to the server code. At these times I really wish we were using Spring Boot because then we would be able to debug both the client and the server code from a single test.

As an alternative you could introduce a new unit test for the TransactionController, and just mock out the server request/response (we have the spring web mock library available there I think). Won't give you the full e2e but might be easier to debug with.

hmottestad added a commit that referenced this issue Aug 27, 2022
…nd fix a bug with deprecated contexts in Changeset
hmottestad added a commit that referenced this issue Aug 31, 2022
…nd fix a bug with deprecated contexts in Changeset
hmottestad added a commit that referenced this issue Sep 1, 2022
@hmottestad hmottestad added this to the 4.1.1 milestone Sep 1, 2022
@hmottestad hmottestad changed the title Clear defaultGraph clears all graphs Clear defaultGraph clears all graphs when using a remote repository Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug issue is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants