We return 404 error if result is empty. It's a bit confusing for client applications.

Here is some explanations: http://stackoverflow.com/a/13367198/272927

CommentFileSizeAuthor
#11 restws-2149553-11.patch1.08 KBChi
#3 restws-2149553-1.patch1.61 KBChi
#1 restws-2149553-0.patch953 bytesChi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi’s picture

Chi CreditAttribution: Chi commented
Title: Create proper response for empty query result. » Build a proper http response for empty query result
Status: Active » Needs review
FileSize
restws-2149553-0.patch953 bytes

Status: Needs review » Needs work

The last submitted patch, 1: restws-2149553-0.patch, failed testing.

Chi’s picture

Chi CreditAttribution: Chi commented
Status: Needs work » Needs review
FileSize
restws-2149553-1.patch1.61 KB
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work

We should still return 404 for non existing pages, only an empty result should return 200, correct?

Chi’s picture

Chi CreditAttribution: Chi commented

404 for non existing resources and 200 for empty collections.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Yes, and a page that does not exist is a non existing resource?

sepgil’s picture

sepgil CreditAttribution: sepgil commented

Since pages are viewed as resources in REST I would asume that is correct.

Chi’s picture

Chi CreditAttribution: Chi commented

What kind of pages we are talking about. The patch impacts only query resources. For example, if we haven't created any articles
GET node.json?type=article should return empty result set {"list":[]} (response code 200 OK).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Yes, and

GET node.json?type=article&page=999

should return a 404, because page 999 does not exist.

Chi’s picture

Chi CreditAttribution: Chi commented

Drupal Core always returns the last existing page in such cases.
https://drupal.org/project/project_module?page=999

We might as well fix it in the same manner.

Chi’s picture

Chi CreditAttribution: Chi commented
Status: Needs work » Needs review
FileSize
restws-2149553-11.patch1.08 KB

This one returns empty data set only for first page.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work 
+++ b/restws.formats.inc
@@ -405,8 +407,13 @@ abstract class RestWSBaseFormat implements RestWSFormatInterface {
+        return array();

That looks wrong, shouldn't we return the usual structure with the "list" element being an empty array in it?

And this should come with a test case that checks on empty node list, should be easy to add in the existing query test.

Chi’s picture

Chi CreditAttribution: Chi commented

In the last patch we return {"list":[]} or <list/> for empty result sets.
Is it correct?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Yes, that would be correct. Oh, I think I was looking in the wrong function when looking a t the source code.

Anyway, we should not return an empty array, but rather the usual $uris variable with the self, first and last variables in it.