graphiti-api / graphiti-rails

MIT License
54 stars 28 forks source link

Exception handling #45

Open mkamensky opened 4 years ago

mkamensky commented 4 years ago

Could someone provide a more detailed explanation of the way exception handling works? I added

  register_exception ActiveRecord::RecordNotFound, status: 404

to my controller, but it does not seem to be caught. I stepped into it and verified that it is added to a list of handled exceptions, but couldn't find what I need to do for these to actually be rescued.

I previously used rescue_from, and don't mind using it again, but I would need to produce a standard jsonapi response, and couldn't find a method (in either graphiti or graphiti-rails) that would do that. Also couldn't find something that converts object.errors to such a format. I assume I'm missing something fundamental...

richmolj commented 4 years ago

I'm not totally sure what you're missing but you should get both of those out of the both. FWIW the way Graphiti works you shouldn't ever be raising that error, we instead raise Graphiti::Errors::RecordNotFound which is handled automatically. Similarly render jsonapi_errors: should automatically handle the validation error payload. I suggest taking a look at the tutorial repo which has both of those working correctly.

mkamensky commented 4 years ago

Thanks. In my situation I'm not raising the exception, Rails does when I look for the record. I look for it before rendering the response for the purpose of authorization, then if I don't find it, I would like to respond with legal jsonapi errors.

I couldn't find jsonapi_errors anywhere in the tutorial.

mkamensky commented 4 years ago

Ok, found jsonapi_errors, I missed the repo part

jandudulski commented 4 years ago

You have to make sure that Accept header is sent with application/json or application/vnd.api+json

toddkummer commented 3 years ago

I was having trouble getting errors to be rescued and the reason seems to be the following configuration setting:

# Raise exceptions instead of rendering exception templates
config.action_dispatch.show_exceptions = false

That's the default value in the config/environments/test.rb file. When I remove that setting, any unhandled exceptions get wrapped by the rails html that makes it easy to see errors when your testing manually through a browser or Postman. However, it makes tests (I'm using RSpec) that fail due to exceptions verbose to the point of being unintelligible.

It seems like I want the setting as is in test, but that means I can't test the Graphiti error responses.

Is my analysis accurate? Any suggestions?

richmolj commented 3 years ago
config.action_dispatch.show_exceptions = false

This is the same as in our sample app which is not having problems. So a good place to look is the difference in configuration between your app and the sample app.

Might also want to check https://github.com/graphiti-api/graphiti-rails#debug-exception-format

toddkummer commented 3 years ago

Thanks for the quick feedback. The sample repo does not have any tests around error handling. I added the following tests to spec/api/v1/employees/create_spec.rb and it illustrates the issue.

  context 'with invalid age' do
    let(:payload) do
      {
        data: {
          type: 'employees',
          attributes: {
            age: 'Forty Two'
          }
        }
      }
    end

    it 'does not handle error when show exceptions is false' do
      expect { make_request }.to raise_exception(Graphiti::Errors::InvalidRequest)
    end

    context 'with helper that sets show exceptions to true' do
      it 'handles error' do
        handle_request_exceptions do
          expect { make_request }.not_to raise_exception
        end
      end
    end
  end

It also shows that the helper handle_request_exceptions is what I was missing.

I don't have permission, otherwise I'd be happy to push up the branch with the tests.

richmolj commented 3 years ago

Ah, I see, thanks for the sample code - I think this is where we were missing each other, right? https://www.graphiti.dev/guides/concepts/error-handling#testing

toddkummer commented 3 years ago

When I read that the first time, I didn't grasp the full meaning. I think the disconnect for me might be that I assumed that this would handle errors the same way rails does.

Going back to my initial question, the answer seems to be that you do have to set show_exceptions to true in order to test error handling with controllers. The helper provides a clean way to do it on a test-by-test basis, but this does mean that if a controller fails with an error that is not handled, you get the rails html in your test results.

Probably yet another reason to keep controllers simple and find a direct way to test. (I'm a big fan of the resource specs.)

As feedback, I'm leaning towards just using rescue_from. There's some nuance here that I don't want the next developer to have to think about.