graphql-devise / graphql_devise

GraphQL interface on top devise_token_auth
MIT License
197 stars 36 forks source link

NoMethodError: undefined method `resource=' for nil:NilClass #259

Closed celsoMartins closed 10 months ago

celsoMartins commented 10 months ago

Hi.

I'm trying to test the userLogin mutation with a simple spec but I'm getting the error above, indicating that I don't have a controller definition in my context.

I didn't add it as a bug report because I may be missing something in my setup.

I already have an application with devise that controls the admin section, and it is only RoR.

Our graphql API was public, but now we need to add authentication to control some actions, such as like a content.

I followed the doc to add the graphql-devise to an existing schema, but after some mistake-solving, I got stuck.

I'll appreciate any help.

What am I missing?

Thanks.

The error:

NoMethodError: undefined method `resource=' for nil:NilClass

  0) Types::MutationType#login when the tip exists for valid parameters logs in
     Failure/Error: controller.resource = resource

     NoMethodError:
       undefined method `resource=' for nil:NilClass
     # /Users/celsomartins/.rvm/gems/ruby-3.2.2@aposta10/gems/graphql_devise-1.3.0/lib/graphql_devise/concerns/controller_methods.rb:71:in `generate_auth_headers'

Where:

    def generate_auth_headers(resource)
      auth_headers = resource.create_new_auth_token
      controller.resource = resource <================= the null pointer exception happens here
      access_token_name = DeviseTokenAuth.headers_names[:'access-token']
      client_name = DeviseTokenAuth.headers_names[:'client']

      # NOTE: Depending on the DTA version, the token will be an object or nil
      if controller.token
        controller.token.client = auth_headers[client_name]
        controller.token.token = auth_headers[access_token_name]
      else
        controller.client_id = auth_headers[client_name]
        controller.token = auth_headers[access_token_name]
      end

      auth_headers
    end

class GraphqlController < ApiApplicationController
  def execute
    result = if params[:_json].present?
               multiplex_operation
             else
               single_operation
             end

    expires_in(90.seconds, public: true, stale_while_revalidate: 30.days, stale_if_error: 30.days)
    render json: result
  end

  private

  def multiplex_operation
    queries = params[:_json].map do |param|
      {
        query: param[:query],
        variables: param[:variables],
        context: gql_devise_context(:user),
        operation_name: param[:operationName]
      }
    end
    Aposta10Schema.multiplex(queries)
  end

  def single_operation
    operation_name = params[:operationName]
    Aposta10Schema.execute(
      query: params[:query],
      variables: variables,
      context: gql_devise_context(:user),
      operation_name: operation_name
    )
  end

  def variables
    variables = params[:variables]
    variables = JSON.parse(params[:variables]) if params[:variables].present? && request.method == 'GET'
    variables
  end
end
# frozen_string_literal: true

class ApiApplicationController < ApplicationController
  skip_before_action :verify_authenticity_token

  include GraphqlDevise::SetUserByToken
end
# frozen_string_literal: true

class GraphqlController < ApiApplicationController
  def execute
    result = if params[:_json].present?
               multiplex_operation
             else
               single_operation
             end

    expires_in(90.seconds, public: true, stale_while_revalidate: 30.days, stale_if_error: 30.days)
    render json: result
  end

  private

  def multiplex_operation
    queries = params[:_json].map do |param|
      {
        query: param[:query],
        variables: param[:variables],
        context: gql_devise_context(:user),
        operation_name: param[:operationName]
      }
    end
    Aposta10Schema.multiplex(queries)
  end

  def single_operation
    operation_name = params[:operationName]
    Aposta10Schema.execute(
      query: params[:query],
      variables: variables,
      context: gql_devise_context(:user),
      operation_name: operation_name
    )
  end

  def variables
    variables = params[:variables]
    variables = JSON.parse(params[:variables]) if params[:variables].present? && request.method == 'GET'
    variables
  end
end
  describe '#login' do
    context 'for valid parameters' do
      it 'logs in' do
        user = Fabricate :user

        mutation =
          %(mutation {
              userLogin(email: "#{user.email}", password: "#{user.password}") {
                credentials {
                  uid
                }
              }
            })

        result = Aposta10Schema.execute(mutation).as_json

        expect(result['data']['userLogin']['credentials']['uid']).to eq('SUCCESS')
      end
    end
  end
# frozen_string_literal: true

module Types
  class MutationType < Types::BaseObject
    field_class GraphqlDevise::Types::BaseField

    field :distrust_bookmaker, mutation: Mutations::DistrustBookmakerMutation
    field :like_tip, mutation: Mutations::LikeTipMutation
    field :trust_bookmaker, mutation: Mutations::TrustBookmakerMutation
  end
end
mcelicalderon commented 10 months ago

Hey @celsoMartins! It looks like the problem is in your specs

describe '#login' do
    context 'for valid parameters' do
      it 'logs in' do
        user = Fabricate :user

        mutation =
          %(mutation {
              userLogin(email: "#{user.email}", password: "#{user.password}") {
                credentials {
                  uid
                }
              }
            })

       # You are not inserting any context in here, and `context[:controller]` is what is missing
        result = Aposta10Schema.execute(mutation).as_json

        expect(result['data']['userLogin']['credentials']['uid']).to eq('SUCCESS')
      end
    end
  end

In here you are testing the schema directly, and not going through the GraphqlController which in this case is responsible for inserting the correct context into the schema query execution:

context: gql_devise_context(:user),

You have two alternatives:

  1. Insert the correct context in your specs as you have them right now. But together with this you would have to write some other integration specs that make sure you are inserting the right context into the query execution.
  2. Change your specs to request which will be closer to how your real app behaves and this is what I would recommend
celsoMartins commented 10 months ago

Hey @mcelicalderon

Thanks for your prompt response.

After your answer, I came up with this and it is working now.

We will solve the main problem and then we will evaluate your suggestion about testing the controller.

Thanks again.

  describe '#userLogin' do
    context 'for valid parameters' do
      it 'logs in' do
        user = Fabricate :user

        mutation =
          %(mutation {
              userLogin(email: "#{user.email}", password: "#{user.password}") {
                credentials {
                  uid
                }
              }
            })

        graphql_controller_double = instance_double(GraphqlController, resource: user)
        allow(graphql_controller_double).to(receive(:resource=))
        allow(graphql_controller_double).to(receive(:token))
        allow(graphql_controller_double).to(receive(:client_id=).with(any_args))
        allow(graphql_controller_double).to(receive(:token=).with(any_args))
        expect(graphql_controller_double).to(receive(:sign_in).with(:user, user, { bypass: false, store: false }))

        context = { controller: graphql_controller_double }

        result = Aposta10Schema.execute(mutation, variables: nil, context: context).as_json

        expect(result['data']['userLogin']['credentials']['uid']).to eq user.uid
      end
    end
  end
mcelicalderon commented 10 months ago

Yes, I guess you need to mock if you want to do that, but then of course, your specs become dependent of how the gem works with the controller. I realize it's not ideal for testing how the gem works (I mean having to inject the controller so it's used within the schema execution), but it was necessary to have some control at the request level. Perhaps as an improvement we could provide a test helper that will provide the necessary context to do schema level specs. But again, I think it would be preferable to do request specs to avoid this type of problem while also executing tests that are closer to how the app actually behaves

celsoMartins commented 10 months ago

I'm not sure if calling the controller is closer to what happens in the real world. Either using the react app or using Altair/GraphiQL we do not call the controller, but the query directly.

Our spec to the controller makes a very simple query to make sure the main parts are working.

But after the tracing bullet, we'll write a spec to the controller to deal with authenticated mutations and/or queries. But usually, we create only one for each one, and the main behavior is tested directly on mutations and queries.

It is the first authentication coming from React and RN in this app and this behavior has not been tested yet.

# frozen_string_literal: true

RSpec.describe GraphqlController do
  describe 'GET #execute' do
    context 'with a valid bookmaker list' do
      it 'returns the query result' do
        bookmaker = Fabricate :bookmaker, highlighted: true
        book_maker_review = Fabricate :book_maker_review, bookmaker: bookmaker, status: :published
        query =
          %(query {
            bookMakerReviewsList {
              bookMakerReviews {
                id
              }
            }
          })

        get :execute, params: { format: :json, query: query, variables: {} }

        expect(response).to have_http_status :ok
        expect(response.body).to eq("{\"data\":{\"bookMakerReviewsList\":{\"bookMakerReviews\":[{\"id\":\"#{book_maker_review.id}\"}]}}}")
      end
    end

    context 'with no data' do
      it 'returns an empty array' do
        query =
          %(query {
            bookMakerReviewsList {
              bookMakerReviews {
                id
              }
            }
          })

        get :execute, params: { format: :json, query: query }

        expect(response.body).to eq('{"data":{"bookMakerReviewsList":{"bookMakerReviews":[]}}}')
      end
    end

    context 'with an invalid query' do
      it 'logs the error' do
        query =
          %(query {
            bookMakerReviewsList(period: "bla") {
              bookMakerReviews {
                id
              }
            }
          })

        expect(Rollbar).not_to(receive(:critical))
        expect(Rails.logger).not_to(receive(:error))

        get :execute, params: { format: :json, query: query }

        expect(response.body).to include('error')
      end
    end

    context 'with multiplex' do
      it 'parses the query and returns the same result' do
        Fabricate :book_maker_review, status: :published
        Fabricate :book_maker_review, status: :published
        query =
          %(query {
            bookMakerReviewsList(limit: 1) {
              bookMakerReviews {
                id
              }
            }
          })

        get :execute, params: { _json: [{ query: query }] }

        expect(response).to have_http_status :ok
      end
    end
  end
end
mcelicalderon commented 10 months ago

I mean request specs, not controller specs that are now discouraged.

Either using the react app or using Altair/GraphiQL we do not call the controller, but the query directly.

Making an HTTP request to the endpoint where you have mounted your GraphQL API will go through a controller. That controller is the one that is going to call Aposta10Schema.execute and run the query. Anyway, you can take a look at how we test this gem for an example on using request specs :)