Skip to content

load: on plain argument #2762

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

Closed
hvenables opened this issue Feb 19, 2020 · 5 comments
Closed

load: on plain argument #2762

hvenables opened this issue Feb 19, 2020 · 5 comments

Comments

@hvenables
Copy link

hvenables commented Feb 19, 2020

We are starting to migrate to using loads: on plain arguments following this being merged #2720

When i use this in the context of a mutation and it fails the check i get a return of nil

module Mutations
  class SomeMutation

  argument :object_id, ID, required: true, loads: Types::ObjectType, pundit_action: :view

  def resolve(object)
    ...
  end
end

So in the above example if you cant view object_id it returns nil

However

module Types
  class SomeType

    field :some_field, Boolean, null: true do
      argument :object_id, ID, required: true, loads: Types::ObjectType, pundit_action: :view
    end
  end
end

if i do this in the context of a type then i get GraphQL::UnauthorizedError error that bubbles out of graphql-ruby. Is this indented behaviour that loads works differently for different cases?

I also dont seem to be able to catch the error inside my graphql schema with

rescue_from(GraphQL::UnauthorizedError) do |err, _obj, _args, _ctx, _field|
  ...
end

We're on graphql 1.10.3 and graphql-pro 1.13.0

@rmosolgo
Copy link
Owner

Thanks for the detailed report, I'll take a look!

@rmosolgo
Copy link
Owner

I tried to replicate this issue in a little script, but it seemed to work for me:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "graphql", "1.10.4"
  source "https://gems.graphql.pro" do
    gem "graphql-pro"
  end
  gem "pundit"
end

class ThingPolicy
  def initialize(obj, viewer)
  end

  def view?
    false
  end
end

class BaseArgument < GraphQL::Schema::Argument
  include GraphQL::Pro::PunditIntegration::ArgumentIntegration
  pundit_role nil
end

class BaseField < GraphQL::Schema::Field
  include GraphQL::Pro::PunditIntegration::FieldIntegration
  pundit_role nil
  argument_class(BaseArgument)
end

class BaseObject < GraphQL::Schema::Object
  include GraphQL::Pro::PunditIntegration::ObjectIntegration
  pundit_role nil
  field_class(BaseField)
end

class Thing < BaseObject
  field :name, String, null: false
end

class Query < BaseObject
  field :thing, Thing, null: true do
    argument :thing_id, ID, required: true, loads: Thing,
    pundit_role: :view,
    pundit_policy_class: ThingPolicy
  end

  def thing(thing: nil)
    thing
  end
end

class Schema < GraphQL::Schema
  query Query

  def self.object_from_id(id, ctx)
    OpenStruct.new(name: id)
  end

  def self.resolve_type(abst_type, obj, ctx)
    Thing
  end
end

pp Schema.execute('{ thing(thingId: "stuff") { name } }').to_h

It returned nil as expected:

$ ruby test.rb
{"data"=>{"thing"=>nil}}

Could you please share the full error message and stack trace? That would help me figure out what code is involved (and what code is not catching the error properly!)

@sshaw
Copy link

sshaw commented Oct 20, 2020

if i do this in the context of a type then i get GraphQL::UnauthorizedError error that bubbles out of graphql-ruby. Is this indented behaviour that loads works differently for different cases?

I also dont seem to be able to catch the error inside my graphql schema with

Are you useing Execution::Interpreter and/or Analysis::AST? May be related to #3198.

@rmosolgo
Copy link
Owner

Errors have been re-written for the interpreter and they're turned on by default in 1.12. I don't have any more info to debug here, so I'll close the issue. If you're still running into trouble after updating graphql-ruby, please open a new issue!

@bessey
Copy link
Contributor

bessey commented May 3, 2022

@rmosolgo I am bumping into this too (Harry and I work together). The issue I am having is that the behaviour of loads: with respect to pundit_policy_class: differs between the plain argument and the mutation argument use cases. You seem to have run into this in your example because you amended pundit_policy_class.

Here's an enhanced example that demonstrates the difference in behaviour:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "graphql", "1.13.10"
  source "https://gems.graphql.pro" do
    gem "graphql-pro", "1.21.4"
  end
  gem "pundit"
end

class ThingPolicy
  def initialize(viewer, obj)
    @obj = obj
  end

  def view?
    puts "ThingPolicy#view? ran on #{@obj}"
    true
  end

  def view_and_more?
    puts "ThingPolicy#view_and_more? ran on #{@obj}"
    true
  end
end

class BaseArgument < GraphQL::Schema::Argument
  include GraphQL::Pro::PunditIntegration::ArgumentIntegration
  pundit_role nil
end

class BaseField < GraphQL::Schema::Field
  include GraphQL::Pro::PunditIntegration::FieldIntegration
  pundit_role nil
  argument_class(BaseArgument)
end

class BaseObject < GraphQL::Schema::Object
  include GraphQL::Pro::PunditIntegration::ObjectIntegration
  pundit_role :view
  field_class(BaseField)
end

class BaseMutation < GraphQL::Schema::Mutation
  include GraphQL::Pro::PunditIntegration::MutationIntegration

  object_class BaseObject
  argument_class BaseArgument
  field_class BaseField

  pundit_role nil
end

class ThingType < BaseObject
  field :name, String, null: false
end

class Query < BaseObject
  pundit_role nil
  field :thing, ThingType, null: true do
    argument :thing_id, ID, required: true, loads: ThingType, pundit_role: :view_and_more
  end

  def thing(thing: nil)
    thing
  end
end

class CreateThing < BaseMutation
  null true

  argument :thing_id, ID, required: true, loads: ThingType, pundit_role: :view_and_more

  payload_type ThingType

  def resolve(thing:)
    thing
  end
end

class Mutation < BaseObject
  pundit_role nil
  field :create_thing, mutation: CreateThing
end

Thing = Struct.new(:id, :name)

class Schema < GraphQL::Schema
  query Query
  mutation ::Mutation

  def self.object_from_id(id, ctx)
    Thing.new(id, "test")
  end

  def self.resolve_type(abst_type, obj, ctx)
    ThingType
  end
end

pp Schema.execute('mutation { createThing(thingId: "stuff") { name } }').to_h

pp Schema.execute('{ thing(thingId: "stuff") { name } }').to_h

which leads to the output

# Mutation case
ThingPolicy#view? ran on #<struct Thing id="stuff", name="test">
ThingPolicy#view_and_more? ran on #<struct Thing id="stuff", name="test">
ThingPolicy#view? ran on #<struct Thing id="stuff", name="test">
{"data"=>{"createThing"=>{"name"=>"test"}}}

# Query case
ThingPolicy#view? ran on #<struct Thing id="stuff", name="test">
Traceback (most recent call last):
	75: from ./gql-repro.rb:104:in `<main>'
...gems/graphql-pro-1.21.4/lib/graphql/pro/pundit_integration.rb:359:in `rescue in pundit_policy': No policy found for: (GraphQL::Pro::PunditIntegration::PolicyNotFoundError)

- BaseArgument: Query.thing.thingId
- Runtime value: `nil`

Pundit error: Pundit::NotDefinedError, unable to find policy `NilClassPolicy` for `nil`

So the mutation case seems sane, if in a confusing order (shouldn't the argument's pundit_role be called first, and then the object's? I would have thought so)

The query case however, the meaning of loads: has changed entirely. Now, despite having already run the loader code and authorized the loaded objects, afterwards we attempt to authorize with view_and_more? using Query's object (nil) and its policy (which is not found). Shouldn't this case also be authorizing with the already loaded object and its policy, matching the mutation?

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