katharsis-project / katharsis-framework

Katharsis adds powerful layer for RESTful endpoints providing implementenation of JSON:API standard
http://katharsis.io
Apache License 2.0
134 stars 65 forks source link

Failed TypeParsing during QuerySpec filter deserialization aborts the request #368

Open Ramblurr opened 7 years ago

Ramblurr commented 7 years ago
@JsonApiResource
class Task {
  @JsonApiId
  String id;

  @JsonApiToOne
  Project project;
}

@JsonApiResource
class Project {
  @JsonApiId
  String id;

  @JsonApiToMany
  List<Task> tasks;
}

This query parameter results in an exception from the TypeParser that it can't coerce String.class into a Project.class

/tasks?filter[tasks][project]=1 , meaning: only return tasks that have project.id = 1

This was valid in theQueryParams days because QueryParams was just a dumb bag of params, whereas QuerySpec is being smart and helpful. However we Implemented this filter[resource][relationship]=ID pattern as a shortcut for filter[resource][relationship][id]=ID extensively in our project.

Putting aside the discussion on which "more correct" ([relationship][id]= vs [relationship]=), we can't easily change our query param format now that it is in production.

I see a couple solutions:

  1. Simple solution: Katharsis should be a little less opinionated here. When the TypeParser fails to coerce a type into what it expects, don't throw an exception that aborts the request. Instead, just return the original String value and let the repository handle it.
  2. "Smart" solution: Identify when query param field references a relationship, and try to fetch it by ID from the appropriate repository

(1) would be perfectly serviceable, as I don't see a good reason to abort the request entirely, that should be up to the repository programmer.

I think (2) would be handy, but not appropriate in all situations, so it should be optional if implemented.

This issue came up as we attempted to transition from 2.6 Katharsis' QueryParams to 3.0 QuerySpec.

It is important to note that the JSONAPI spec doesn't say anything on the matter of filtering semantics, except that it is agnostic:

Note: JSON API is agnostic about the strategies supported by a server. The filter query parameter can be used as the basis for any number of filtering strategies.

hibaymj commented 7 years ago

I appreciate the spot the QueryParams to QuerySpec conversion got you into, in regards to the code you've already released.

From a framework design perspective, I see the initial as an absolute non-starter. This framework is about creating an easy curated way to create JsonApi and hypermedia. To dump the control flow back into the developers hands in the repository in an inconsistent and indeterminate state just doesn't fly.

The second one seems to be the only option, and I don't see it being a terribly complicated implementation, as any bracket chain longer than 1 should identify a relationship.

remmeier commented 7 years ago

both 1 and 2 would be good to support I guess.

regarding being agnostic: applications are also free to implement there own serializer/deserializer to do the parameter handling manually. The DefaultQuerySpecDeserializer follows the JSON API recommendation which should be fine for many use cases. But some more feature flags to customize the behavior would be useful for sure, because doing it manually is quite a bit of work. That fact should probably be made a bit clearer in the docs.

In general I think we should also go for the dot notation instead of brackets, meaning [type][attr1.attr2][operator] whereas type and operator are optional. The brackets have been the Katharsis default in the past. Both should be supported. But there should be an option to do dots only (because not always it is clear what is a type and what is an attribute if both carry the same name)