graphiti-api / graphiti-rails

MIT License
54 stars 28 forks source link

Possibly provide detailed error messages for non-JSON API responses #10

Closed wagenet closed 5 years ago

wagenet commented 5 years ago

There are a few places in the code specifically handling the :jsonapi content type:

https://github.com/wagenet/graphiti-rails/commit/cc6492a851c9748ee6e9f8081b2b4dedb4bff85b#diff-057c6c9dd4f8a50d518efafb3a39597bR52 https://github.com/wagenet/graphiti-rails/commit/cc6492a851c9748ee6e9f8081b2b4dedb4bff85b#diff-fdb5492c892a0baaa7b246e37a0593cfR19

This is smart, but we'll have to consider :json (and maybe :xml) as well. This is for users who want everything about graphiti, but they just dislike the crufty JSON:API payload. I think even these users want the JSON:API error payload - at worst they would want all the same data rendered slightly differently, but I think matching JSON:API works for now. It might even be a JSON:API gateway drug.

– @richmolj

The point in explicitly matching JSON API is that unlike JSON and XML, it has a specific payload format for errors. I think we could definitely choose to have a flag to handle other types specially though.

– @wagenet

richmolj commented 5 years ago

What's the disadvantage of rendering the JSON:API errors payload structure when the format is JSON or XML? I get that only JSON:API defines a standard, but for the others we have to render something, might as well render tons of helpful error info?

I bring this up specifically because there are real-life users who want everything but the included/etc JSON:API payload, and I'd like to give them helpful errors by default. All of these would be good with me:

Does that make sense?

wagenet commented 5 years ago

@richmolj My current approach is to only go through Graphiti's handler for JSON:API by default, since there's a clear argument to be made that Rail's JSON:API error rendering is invalid.

However, it's a bit more complex for other cases since there's no "correct" way to handle them. I think it probably makes sense to allow people to opt-in, essentially saying "I'm using GraphitiErrors style rendering" for this content-type.

wagenet commented 5 years ago

(@richmolj's comment was actually before mine, but something strange happened with Github's ordering.)

wagenet commented 5 years ago

This can now be done by modifying Graphiti::Rails.handled_exception_formats. By default this is only [:jsonapi].

richmolj commented 5 years ago

I think the issue here might be the Railtie working as global configuration, instead of controller-specific. IOW if the developer had

class GraphitiApplicationController < ApplicationController
  include Graphiti::Rails
end

To me this is exactly what you wrote above - the developer is saying "I'm using GraphitiErrors style rendering" for all subclasses of this controller, regardless of content-type. This is opposed to the automatic application of graphiti-rails via the Railtie, which could affect non-Graphiti controllers.

IOW with the Railtie [:jsonapi] makes sense as a default, but the mixin seems more explicit what's affected and we could add additional defaults.

All that said, I can respect the user-friendliness of the Railtie approach, and I'll support it if that's the direction you'd like to head. Not to mention, this can be solved with documentation. Mostly just wanted to confirm my understanding of the issue and at least note the difference of approaches. Reopening to bounce this off you, but feel free to close and proceed as you feel best 👍

wagenet commented 5 years ago

@richmolj I'm fairly open to the direction this goes long term. My inclination is to leave it as zero-config and Rails-like as possible in the first pass and let people request more flexibility for their specific use-case. If it turns out there's a consistent demand for something then I'm very much open to accounting for it.

wagenet commented 5 years ago

I think after #22 the APIs will be pretty good.