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

PATCH requests with non-matching deletions should succeed #1085

Closed
RubenVerborgh opened this issue Feb 5, 2019 · 15 comments
Closed

PATCH requests with non-matching deletions should succeed #1085

RubenVerborgh opened this issue Feb 5, 2019 · 15 comments
Assignees

Comments

@RubenVerborgh
Copy link
Contributor

We currently have evidence (inrupt/generator-solid-react#41 (comment)) that a patch with a DELETE block that has no matches will cause a 409. Is this correct? Should there at least be one match with a DELETE?

@kjetilk
Copy link
Member

kjetilk commented Feb 6, 2019

Hmmm, no, 409 doesn't make sense to me either. That sounds like a 404.

@rubensworks
Copy link
Contributor

From the SPARQL UPDATE spec:

3.1.3.3 DELETE WHERE
Analogous to DELETE/INSERT, deleting triples that are not present, or from a graph that is not present will have no effect and will result in success.

So DELETE's without matches should return a 200 it seems. So this indeed looks like a bug in NSS.

@RubenVerborgh RubenVerborgh changed the title Do we correctly handle DELETE patches when there are no matches? PATCH requests with non-matching deletions should succeed Feb 6, 2019
RubenVerborgh added a commit that referenced this issue Feb 6, 2019
RubenVerborgh added a commit to linkeddata/rdflib.js that referenced this issue Feb 6, 2019
RubenVerborgh added a commit that referenced this issue Feb 6, 2019
RubenVerborgh added a commit to linkeddata/rdflib.js that referenced this issue Feb 6, 2019
RubenVerborgh added a commit that referenced this issue Feb 7, 2019
@timbl
Copy link
Contributor

timbl commented Feb 8, 2019

Note: 409 Conflict

--
  |

A SPARQL update message often contains both a DELETE and then an INSERT.
  | This may be used to update a field from one value to another.
  | When more than one application or user is using the same data,
  | there may arise times when the DELETE fails
  | because another user has already deleted the same data.
  | In this case it very important
  | that the delete does not fail silently.
  | The HTTP server MUST return error status 409.
  | ("409 Conflict" indicates that the request could not be processed because of conflict in the request, such as an edit conflict).
  | The client can then for example inform the user by backing out the
  | change the user was trying to make, or it can retry a reservation later.
  | The atomicity of the DELETE,INSERT function can be used to provide
  | various mutual exclusion systems, such as reserving a resource
  | or generating unique sequential numbers, and so on.
 

@RubenVerborgh
Copy link
Contributor Author

RubenVerborgh commented Feb 8, 2019

@timbl I agree, but unfortunately that is in conflict with the SPARQL 1.1 UPDATE W3C Recommendation:

Note that the deletion of non-existing triples has no effect, i.e., triples in the QuadData that did not exist in the Graph Store are ignored.

That's why I created linkeddata/rdflib.js#299, could you have a look at that one?

@timbl
Copy link
Contributor

timbl commented Feb 8, 2019

Grumble. a system has to have semaphores
https://en.wikipedia.org/wiki/Semaphore_(programming)

@kjetilk
Copy link
Member

kjetilk commented Feb 8, 2019

But don't we have a semaphore in the shape of an ETag? Couldn't we do something like https://www.w3.org/1999/04/Editing/ ?

@RubenVerborgh
Copy link
Contributor Author

But don't we have a semaphore in the shape of an ETag?

That might be too restricted. If you changed something unrelated to the file, I might still want to send my patch.

@kjetilk
Copy link
Member

kjetilk commented Feb 9, 2019

But don't we have a semaphore in the shape of an ETag?

That might be too restricted. If you changed something unrelated to the file, I might still want to send my patch.

Yes, but you would have to see how you solve the potential conflict. Besides, you can always do a HEAD just before your PATCH to minimize the time window a conflict might occur.

I think apps that expects frequent writes from many parties will need to make sure that resource descriptions don't grow too large by finding a way to divide-and-conquer, to lower the likelihood of conflicts. I think something like https://www.w3.org/1999/04/Editing/ is a reasonable thing to implement, and that resources SHOULD have an ETag.

@TallTed
Copy link
Member

TallTed commented Feb 13, 2019

I am very disappointed to see the SPARQL 1.1 UPDATE spec excerpt, which says to me there's a tragic bug in the SPARQL 1.1 UPDATE spec, and a failure to learn from existing standards and writings, particularly those which support multiple users acting on the same resources (e.g., ODBC clients of a DBMS instance).

I would suggest that the appropriate response is not a full-success, but a warning or soft-fail/soft-success like ODBC's SQL_SUCCESS_WITH_INFO -- so the user/app can know that the thing they tried to delete wasn't there to delete, which would allow them to address typos and similar situations -- while also not being a hard-fail, so handling can be gentler (and users/apps might choose to ignore the INFO portion, but this is then an informed choice, and they need not be left guessing whether they'd actually deleted the thing)...

(We MUST find a way to modify W3 specs more quickly than is currently possible, especially when such app-blocking errors are discovered!)

@RubenVerborgh
Copy link
Contributor Author

I would suggest that the appropriate response is not a full-success, but a warning or soft-fail/soft-success like ODBC's SQL_SUCCESS_WITH_INFO -- so the user/app can know that the thing they tried to delete wasn't there to delete

I like that idea; we can indeed add extra info to a 200.

However, the problem here is that we want it to work like a semaphore: a patch with both a delete and an insert should only succeed if the delete succeeded. Learning afterwards that one did through but not the other is a consolation prize.

@kjetilk
Copy link
Member

kjetilk commented Feb 14, 2019

I think that the immediate fix is to use the idea from the 1999 Editing note even though there are cases where it would not work. If we go much further, we will quickly find ourselves in WebDAV-land (which we might need soon anyway).

As for the problematic 200, indeed, we could perhaps use another 2xx code, but perhaps we could do a 303 pointing to a resource with more information?

@RubenVerborgh
Copy link
Contributor Author

I think that the immediate fix is to use the idea from the 1999 Editing note

Unfortunately, the situation is worse, since PATCHes now also fail because of locking issues that are way harder to fix. So we might need to abandon PATCH for the time being: LDflex/Query-Solid#16 (which, for the record, I do not like, but I see no other options in a reasonable time frame).

@timbl
Copy link
Contributor

timbl commented Feb 18, 2019

This issue is about changing the Solid API, which is every important and much more important than a bug in a particular server, implementation.

@timbl
Copy link
Contributor

timbl commented Feb 18, 2019

In my opinion, the solid system works, by using 409 Conflict as a crucial part of the architecture. It is in formally "willful violation" of the SPARQL Update spec, and the two communities should in the long term discuss that issue. But for now, the solid system works, and we must keep that. Possible future paths are many - a different mime type, dropping sparql update for n3 patches, changing the update spec, etc. But for now we must keep the 409 to keep the collaborative editing working.

@kjetilk
Copy link
Member

kjetilk commented Feb 18, 2019

OK. I'll close this and #1086.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants