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

Include Graphiti stuff only in api #52

Open
mkamensky opened this issue Apr 22, 2020 · 11 comments
Open

Include Graphiti stuff only in api #52

mkamensky opened this issue Apr 22, 2020 · 11 comments

Comments

@mkamensky
Copy link
Contributor

It seems that the stuff here:

ActiveSupport.on_load(:action_controller) do

is included in all controllers, not just the ones that use Graphiti. Is it possible to opt-out?

@jspooner
Copy link

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

wagenet commented May 21, 2020

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

@wagenet
Copy link
Collaborator

wagenet commented May 21, 2020

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

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

Ayat11 commented Dec 16, 2020

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

wagenet commented Jan 22, 2021

@Ayat11 Can you share one of the errors here?

@conwayje
Copy link

conwayje commented Aug 4, 2021

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

richmolj commented Aug 4, 2021

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

@bilouw
Copy link

bilouw commented Mar 9, 2023

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

cagedartist commented Mar 30, 2023

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

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

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

9 participants