Breaking change: null data returned when resource not found

[bart-degreed]

Description

Earlier, when a resource does not exist, a 404 with an empty response body was returned. After fetch of latest master (with v4 merged in), I am now getting 404 with the next body:

Request URL: https://localhost/api/users/123

Response body:

{
  "links": {
    "self": "https://localhost/api/users"
  },
  "data": null
}

Note the self link is wrong too: it misses the identity.

Traced this down to

JsonApiDotNetCore/src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs

Line 108 in a801d95

// remove the null argument as soon as this has been resolved:

, so it may be intentional.

[maurei]

the current response body is conform spec (except for the self link). Relevant passage:

... except when the request warrants a 200 OK response with null as the primary data (as described above).

It is the result of the rework of the serialization layer: it serializes null as null instead of throwing a 404. This was more reusable because there is various places where null should be represented as null (empty relationships). Also, personally, I prefer using 404s for non-existing routes as opposed to non-existing resources.

Nevertheless the 404 was that was thrown previously was also conform spec. We could add a switch for that in IJsonApiOptions or even allow to configure this on the level of each resource.

[maurei]

I will make a separate issue for the bad self link

[bart-degreed]

Perhaps I am missing something, but the spec reads:

A server MUST respond with 404 Not Found when processing a request to fetch a single resource that does not exist, except when the request warrants a 200 OK response with null as the primary data (as described above).

Both are not happening. The spec refers to a 200 with null data -or- a 404. This issue is about 404 comined with null data, which is not what the spec describes.

A bit below:

A server MAY include error details with error responses.

What is currently implemented is not error details, but a success response body, combined with a http status code that represents an error.

[maurei]

Ah -- my bad. I misread and thought it was returning a 200 with that error body. I will investigate and fix this next week.

[wisepotato]

working on this. To the following, after discussion with @maurei :

• Return a 200 when the route exists, but no data is returned. This is related to the needed 'warrant a 200' in the documentation.
• Return a 404 when the requested route could never return a resource.

[maurei]

Yep, that's correct. Elaborating on your last point

• Return a 404 when the requested route could never return a resource.

Would be good if an error object was returned.

Pointers:

• this is where the error object is currently being formatted
• this is (I think) the origin of the 404 that is currently being thrown

[bart-degreed]

Return a 200 when the route exists, but no data is returned.

I think this is not compliant with the spec:

A server MUST respond with 404 Not Found when processing a request to fetch a single resource that does not exist

[maurei]

The same line in the spec states an exception to that rule:

except when the request warrants a 200 OK response with null as the primary data (as described above).

Where I believe the as described above refers to the following passage:

null is only an appropriate response when the requested URL is one that might correspond to a single resource, but doesn’t currently.

I.e. my reading of the specs is that when a route (i.e. relationship) exists (articles/1/author) but there is no associated data, it's okay to return null as the primary data with HTTP 200.

But IMO all of this is pretty ambiguous :/ This rule + exception is explicitly formulated for fetching single resources. Nothing is stated about the analogous scenario of a has-many route (route articles/1/tags). The equivalent of this rule+exception would be either a 404 or an empty list of resource objects [ ] (rather than null) as the primary data.

[bart-degreed]

null is only an appropriate response when...

I believe this applies solely to related data, as stated in the Note block below it and illustrated by the example.

In short: http://example.com/articles/1/author may return 200 when article 1 exists, but does not have an associated author.

Then, the spec resumes:

A server MUST respond with 404 Not Found when processing a request to fetch a single resource that does not exist, except when the request warrants a 200 OK response with null as the primary data (as described above).

The "except when... described above" refers to the author example. In all other cases, 404 must be used.

So I believe it is wrong to return 200 for http://example.com/articles/1 when article 1 does not exist. And that is the scenario I submitted this bug for.

[wisepotato]

This is in-line with what @maurei have discussed. However, we also discussed that the 200 should apply for when user id =1, exists and a GET request is sent to:

http://example.com/users/1/books

Books being a to-many resource, but this is not described in the spec, but we made a judgement call.

We are in agreement, but the spec is ambiguous on the multiple resources front.

[bart-degreed]

Thanks for the clarification.

[wisepotato]

hi #640 was just merged, which is the preliminary to #636. We're working on it.

[bart-degreed]

#640 breaks our code. It throws at:

JsonApiDotNetCore/src/JsonApiDotNetCore/Middleware/CurrentRequestMiddleware.cs

Line 119 in d9dd760

throw new JsonApiException(500, $"We could not convert the id '{value}'");

This is because we encode our IDs, to prevent callers guessing IDs. And in some cases, they contain additional information needed to fetch the right data. So our requests look like:

/api/videos/ngebwA

For this to work, we have a custom base class that derives from Identifiable<T>, overriding GetStringId and GetTypedId. Additionally we do the same conversion in a custom controller base class. So while our TId in controllers/resources is Int64, the URL contains encoded strings.

[bart-degreed]

Looking at the changes, the trim/split/findindex url parsing feels a bit brittle to me. Wouldn't it be an option to use the route values, like it is done at:

JsonApiDotNetCore/src/JsonApiDotNetCore/Middleware/CurrentRequestMiddleware.cs

Line 251 in d9dd760

private ResourceContext GetCurrentEntity()

?

For example, the debugger shows me for the request path:

api/v3/users/ABC/completions

these route values:

{
  "action": "GetRelationship"
  "controller": "Users"
  "id": "ABC"
  "relationshipName": "completions"
}

Hope this helps.

[wisepotato]

Thank you, apparently the test cases were not comprehensive, the RequestMiddleware was not tested before this PR, so this was bound to happen. Could you make a new issue? That would help, for when I get to it tomorrow.

I did think that somebody would say that they have super-custom ids, but I didn't test it enough apparently ;)

[maurei]

Having obfuscated ID's is a good use-case. A way forward could be to not throw in the current request middleware but just store the route data as is. This way the door is left open for extensibility as @bart-degreed describes, while still being able to generate a useful error response based on the obfuscated id