Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception handling #45

Open
mkamensky opened this issue Jan 20, 2020 · 9 comments
Open

Exception handling #45

mkamensky opened this issue Jan 20, 2020 · 9 comments

Comments

@mkamensky
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Ok, found jsonapi_errors, I missed the repo part

@jandudulski
Copy link

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

@toddkummer
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants