nodeSolidServer / node-solid-server

Solid server on top of the file-system in NodeJS
https://solidproject.org/for-developers/pod-server
Other
1.78k stars 299 forks source link

PATCH requests with non-matching deletions should succeed #1085

Closed RubenVerborgh closed 5 years ago

RubenVerborgh commented 5 years ago

We currently have evidence (https://github.com/Inrupt-inc/generator-solid-react/pull/41#discussion_r254013484) 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 commented 5 years ago

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

rubensworks commented 5 years ago

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.

timbl commented 5 years ago

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 commented 5 years ago

@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 https://github.com/linkeddata/rdflib.js/pull/299, could you have a look at that one?

timbl commented 5 years ago

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

kjetilk commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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: https://github.com/solid/query-ldflex/issues/16 (which, for the record, I do not like, but I see no other options in a reasonable time frame).

timbl commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

OK. I'll close this and #1086.