Notes & Questions

[wadetandy]

Wanted a place to capture random thoughts while looking over the code and also a place to address questions you ask in your comments.

[wadetandy]

https://github.com/wagenet/graphiti-rails/blob/master/lib/graphiti/rails/exceptions_app.rb#L22-L23 Do we actually want to log?

This probably should depend on the specific error type? If the response is >=500, I'd think logging is appropriate. If it's something like a 400 bad request that we have gotten from our request validator, then logging is probably not necessary.

[wadetandy]

https://github.com/wagenet/graphiti-rails/blob/master/lib/graphiti/rails/context.rb#L5-L10 Any downside to including this in all Rails controllers?

Would follow the lead of some other projects and have this as a configurable option:

Graphiti::Rails.configure do |c|
  c.base_controller = 'ApplicationController'
end

[wadetandy]

The list of errors you pulled from graphiti to merge into action_dispatch.rescue_reponses doesn't include the new InvalidRequest error, which will be one of the critical ones we need to handle.

That has a custom error handler subclass in graphti_errors: https://github.com/graphiti-api/graphiti_errors/blob/master/lib/graphiti_errors.rb#L23-L26

[richmolj]

I wrote my own notes here: https://gist.github.com/richmolj/ded6c99353ead759785caaeeb68a7e60

To touch on a few things Wade mentioned:

If the response is >=500, I'd think logging is appropriate. If it's something like a 400 bad request that we have gotten from our request validator, then logging is probably not necessary.

I think these are good defaults, we just need the ability to override with register_graphiti_exception FooError, log: false. One example is avoiding logging on 404 - maybe, maybe not. I think we have a few internal BGov errors as well were we just want to eliminate legacy noise.

Would follow the lead of some other projects and have this as a configurable option:

I'd prefer including a mixin as a little more explicit. The downside to applying to all controllers is running AMS/whatever and graphiti side-by-side.

[wagenet]

@wadetandy for now, I think we'll only log fatal exceptions. See eeb6b6d.

[wagenet]

See also #7

[wagenet]

For easier discussion, I've split these up into multiple issues. If I missed anything, please open a new issue.