graphiti-api / graphiti-rails

MIT License
54 stars 28 forks source link

Include Graphiti stuff only in api #52

Open mkamensky opened 4 years ago

mkamensky commented 4 years ago

It seems that the stuff here: https://github.com/graphiti-api/graphiti-rails/blob/2b787402f3247a5fdb27e0cb36f53522dac49557/lib/graphiti/rails.rb#L39 is included in all controllers, not just the ones that use Graphiti. Is it possible to opt-out?

jspooner commented 4 years ago

I have an existing API with existing v1 and v2 routes. When I added a Graphiti v3 route my request specs for v2 started failing. The root cause for this failure is Graphiti::Rails:: wrap_graphiti_context https://github.com/graphiti-api/graphiti-rails/blob/master/lib/graphiti/rails/context.rb#L26

 NameError:
       undefined local variable or method `jsonapi_context' for #<Api::V2::Accounts::AutocompleteController:0x00007fc893742760>
     # ./app/controllers/api/v2/accounts/autocomplete_controller.rb:47:in `method_missing'

I can't explain why respond_to?(:jsonapi_context) evaluates to true yet a call to jsonapi_context results in a NameError

My solution was to monkey patch this method like this.

module Graphiti
  module Rails
    # Wraps controller actions in a [Graphiti Context](https://www.graphiti.dev/guides/concepts/resources#context) which points to the
    # controller instance by default.
    module Context
      def graphiti_context
        self
      end
    end
  end
end
wagenet commented 4 years ago

@mkamensky I do not believe it's currently possible to opt-out. What issues is it causing for you?

wagenet commented 4 years ago

@jspooner this sounds very odd. I don't have any specific leads offhand. If you're able to come up with a small reproduction I might be able to figure it out.

mkamensky commented 4 years ago

@wagenet Truth is I no longer remember, I think it had something to do with debugging, but I can no longer recall what... I see that I overrode debug_graphiti and wrap_graphiti_context with just yield, but I can't remember what was the reason, and whether it solved it.

Ayat11 commented 3 years ago

Is it still not possible to opt-out? I have some controllers that need to use Graphiti and other that don't, so I'm getting errors for the ones that don't, is there a way to specify which controllers use graphiti?

wagenet commented 3 years ago

@Ayat11 Can you share one of the errors here?

conwayje commented 3 years ago

My org is seeing problems stemming from this everywhere-insertion as well and might need to migrate off of Graphiti because of it 😞

We have some existing controllers (classic Rails controllers and a v1 API) that we don't want Graphiti to touch at all. Our plan was for v2 to be the only Graphiti-powered portion of our app.

However, when we install graphiti-rails, certain cases break in those existing controllers. For example, where one endpoint is supposed to respond with a specific error message, we instead get an exception from the sub-gem rescue_registry from @wagenet :

"Didn't expect RescueRegistry context to be set in controller"

Which breaks specs/expected behavior. We're a bit scared to move forward with Graphiti given the fact that it touches huge portions of our app that we don't want it to touch at all, and it feels risky to go down this road even though our request/api specs are pretty thorough. We may use the deprecated graphiti gem style or try to submit a PR to fix, but drawbacks in any case.

richmolj commented 3 years ago

@conwayje can you give us a tiny demo app that reproduces the error? Sounds like that's what @waganet needs to debug

bilouw commented 1 year ago

Hi !

I have the same problem ! When i try to sign_in with wrong password, the post to /sign_in return 200 OK instead of 401 because of this error:

image

As we can see, an 401 is raise but at the end it return an 200 (i'm pretty sure it's because of the error).

The weird thing is that i have another /sign_in page (on another devise model/namespace), and on this sign_in page, it don't raise the RescueRegistry Error, and the server return 401 correctly.

Don't know why ... any clues ? Also, how can we fix this error once and for all ?

EDIT: I searched a little bit about this ActiveSupport.on_load method and found that there is others hooks available, and there is an hook named action_controller_api that seems to only hook ActionController::API controllers !

image

Tell me if i'm wrong but it seems to be exactly what we need ? Maybe we could just replace the hook ?

EDIT 2: I think i know why one of my /sign_in return 401 correctly and the other don't. I override devise controllers and views for one of my devise model, but not for the other. So i assume graphiti is include and raise the error only in my overrode controllers.

EDIT 3: Ok, so i found a solution. I had a cors issue so turbo-stream was not loading in one of my /sign_in page. When i use turbo-stream to render errors everything seems to work fine. The RegistryRescue error seems to be raise only if we render error without turbo-stream ! Good to know until we find a solution !

cagedartist commented 1 year ago

We have the same issue as described by conwayje. Non-Graphiti-related tests that expect errors are disrupted by an error message "Didn't expect RescueRegistry context to be set in controller" - coming from somewhere in `process_action'. First line of stack trace is ... activerecord-6.1.7.3/lib/active_record/railties/controller_runtime.rb:27:in 'process_action' ...but line 27 is 'super', I believe.

UPDATE: Apparently, RescueRegistry is logging a warning in 'process_action' when RescueRegistry.context is set. Code.

In our case, it isn't causing tests to fail, but it seems to indicate a problem.

onwardmk commented 8 months ago

I have the same issue. Is it recommended to use the deprecated graphiti gem without graphiti-rails?