Vacasa / drf-jsonapi

JSON API with Django Rest Framework
MIT License
3 stars 5 forks source link

Fix error handling for sparse fieldsets #2

Open tpansino opened 6 years ago

tpansino commented 6 years ago

Assume we have a JSON API endpoint called /users with a first_name attribute on the resource object.

The JSON API spec says that GET /users?fields[user]=first_name should return a sparse array of user objects with only the first_name attribute in the objects.

What actually happens currently is all attributes are returned, the fields[users] parameter is ignored, because the code currently matches against fields[user] instead. Furthermore, there's no feedback to a developer to indicate that the requested fields[.*] parameter is actually invalid and the request is bad.

Expected behavior:

tpansino commented 6 years ago

So in reviewing this ticket some time later, I've discovered another discrepancy with our implementation. We're using plural form for the endpoints (/users) but singular form for the resource object ("type": "user"), and all of the examples in the JSON API spec have the resource object type matching the endpoint name (eg: "type": "users"). The spec also says we should be consistent with plural vs. singular naming, which we are generally, just not in this aspect.

Need feedback from the team on how we want to deal with this, as I have a feeling the fixes needed for this ticket will be affected by that decision.

In the meantime, the user story to use for testing here is basically unchanged:

morgandenning commented 6 years ago

@tpansino My understanding from the documentation when it mentions consistency is that we must be consistent with the type attribute, so we do not have endpoints where the type is plural, and some where type is singular. I also believe it makes sense for the type to be singular, as it is in the data attribute for a single object.

That said, however, I'm cool with whatever we decide. Some input from others may be beneficial

chrisbrantley commented 5 years ago

@tpansino this does not appear to be a bug in so far as the query params for sparse fieldsets work on the resource type and not necessarily the url base name for the resource. The rc1.0 supports keeping the urls and the type consistent (all plural or all singular) OR you can keep the urls plural and the types singular (my preferred method).

However, your second point of throwing a validation error when an invalid type is specified is a great idea and I will include that functionality in the rc1.0 branch.

chrisbrantley commented 5 years ago

@tpansino I was able to fix the validation issue in 1.0rc by splitting validation into 2 phases. The 1st phase runs before any queries so we can bail on invalid requests. The second phase is a "late" validation phase and runs after all the queries have run and serializers have been hydrated. We have to do this because it's impossible to know all the possible resource types available until we recurse through the serializer tree. This allows us to catch invalid types and display a validation error rather than just ignoring query params that the system can't do anything with.