Handle 404 in CardDAV multi get #830
Conversation
DeepDiver1975
force-pushed
the
multiget-404
branch
from
ced73a6 to
17f54b3
Compare
|
@evert ready to go from my pov - let me know what you think - THX |
|
I'll take care about CalDAV multiget once we agree on this being the right way to do it |
| } | ||
| } catch (NotFound $ex) { | ||
| $result[$path] = [ | ||
| 404 => [], |
There was a problem hiding this comment.
I'm not sure about this response status. This tells me that the path is found, but it had 0 properties.
So two ideas:
- Set
$result[$path] = nullfor items that are not found. - Add a new
statusproperty to every item in$result, indicating the "overall" status of the thing on that path.
Either works for me, just null is a good indicator that the thing didn't exist and makes it very obvious, whereas adding a status would allow us to handle errors other than 404's in the future.
DeepDiver1975
force-pushed
the
multiget-404
branch
2 times, most recently
from
8aab1fe to
583610a
Compare
|
@evert please have a look - THX |
| $writer->writeElement('{DAV:}status', 'HTTP/1.1 ' . $status . ' ' . \Sabre\HTTP\Response::$statusCodes[$status]); | ||
| // in case of a 404 we simply don't expect any properties at all | ||
| $empty = ($status !== 404); |
There was a problem hiding this comment.
I think I don't like this line. Could it get removed?
I don't think realistically that there might be a case where there's a resource-level 404 status, where we're returning properties anyway, but I don't really see a reason to special-case this.
There was a problem hiding this comment.
If we don't set empty to true we will get a propset with 418/teapot inside. Kind of strange to me.
|
Great patch otherwise 👍 |
| @@ -137,7 +139,6 @@ function xmlSerialize(Writer $writer) { | |||
| $writer->writeElement('{DAV:}prop', $properties); | |||
| $writer->writeElement('{DAV:}status', 'HTTP/1.1 ' . $status . ' ' . \Sabre\HTTP\Response::$statusCodes[$status]); | |||
| $writer->endElement(); // {DAV:}propstat | |||
|
|
|||
| } | |||
| if ($empty) { | |||
There was a problem hiding this comment.
@DeepDiver1975 ah then this block really should be $empty && !$status all along. Preferably this function should be rewritten a bit so there's no duplication in writing the status element.
There was a problem hiding this comment.
poor coding on my side. Might have been drunk. Would explain the teapot
There was a problem hiding this comment.
There is one Test here which expects an explicit status of 200 and an teaport propset.
I'll post it in a bit.
There was a problem hiding this comment.
There was 1 failure:
1) Sabre\DAVACL\PrincipalMatchTest::testPrincipalMatch
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
<d:status>HTTP/1.1 200 OK</d:status>
<d:href>/principals/user1</d:href>
- <d:propstat>
- <d:prop/>
- <d:status>HTTP/1.1 418 I'm a teapot</d:status>
- </d:propstat>
</d:multistatus>
583610a to
680c65e
Compare
|
|
@evert Can I drag your attention onto this one? |
|
Hey @DeepDiver1975 , I think i made a mistake conflating the inner propfind status (418 in your failing testcase) with the outer one. Sorry about that :( Fairly sure your original patch was good. I suppose once the code is fixed so that unittest passes again we're good here. Sorry for all the wait. |
DeepDiver1975
force-pushed
the
multiget-404
branch
from
222b890 to
6f5b1ff
Compare
|
@evert all green now - please have a second look. THX |
| @@ -994,20 +995,25 @@ function getPropertiesForMultiplePaths(array $paths, array $propertyNames = []) | |||
| $result = [ | |||
| ]; | |||
|
|
|||
| $nodes = $this->tree->getMultipleNodes($paths); | |||
| foreach ($paths as $path) { | |||
There was a problem hiding this comment.
I'm super sorry for not spotting this issue earlier. I'm not as focused as I used to... but I see a new problem :(
The new problem with this patch, that I didn't see before, is that you're now no longer using getMultipleNodes(). While this solves the problem, the new problem is that that function was actually implemented to solve a fairly large performance problem.
There's several requests (such as sync-collection and multiget reports) that need to do a lot of individual lookups, and by adding the "Multiple" interfaces, new optimizations were possible.
So what can be done? I think the best thing is that getMultipleNodes actually gets changed to catch errors. The simplest solution there would be to omit (or return null) for the paths that throw a NotFound.
Then any non-NotFound exceptions will still 'fatal' though, so if we handle those as well in this patch we need a different mechanism. Perhaps the result of getMultipleNodes could return either a Node or an instance of Exception in case there was a problem? That would at least give us all the information (although it's a bit unconventional).
Sorry again :/
DeepDiver1975
force-pushed
the
multiget-404
branch
from
70154d0 to
e546822
Compare
refs #829