Open IngusSkaistkalns opened 2 years ago
:+1: up
Hmm, I'm getting this error now on the latest graphql-ruby
(1.13.4)
undefined method `graphql_definition' for #<GraphQL::Schema::TypeMembership
Did you mean? graphql_name
/bundle/ruby/3.0.0/bundler/gems/graphql-guard-ca368a8dbdac/lib/graphql/guard.rb:17:in `block in <class:Guard>'
Not quite sure what's happening as I didn't think they removed the .graphql_definition
yet? In the PR, it seems deprecated, not removed. Not quite following what's going on.
Downgrading to graphql 1.12.23
addresses the issue, and I no longer get the error.
Is there any news on this? I get the same warnings when updating to graphql-ruby 1.13.8
I just tried to find a fix for this, but I'm not sure how to get the metadata
from schema_member
without using graphql_definition
in those lines:
In sum there are 3 places where #graphql_definition
is still used.
As mentioned here https://github.com/rmosolgo/graphql-ruby/blob/master/CHANGELOG.md#deprecations-3
All features using those legacy definitions are already removed and all behaviors should have been ported to class-based definitions. So, you should be able to remove those calls entirely.
But of course, we can't just remove the call itself, somehow it has to load the masks.
Maybe @rmosolgo can help?
I just discovered, that it's not only a deprecation warning, but an error:
NoMethodError:
undefined method `graphql_definition' for #<GraphQL::Schema::TypeMembership:0x0000557f69381178>
Did you mean? graphql_name
# /usr/local/bundle/gems/graphql-guard-2.0.0/lib/graphql/guard.rb:17:in `block in <class:Guard>'
metadata
is part of the legacy type definition system, you're right that there's no way to access it apart from .graphql_definition
. Instead of .metadata
, you should store custom values in instance variables on your definitions. For example:
module MaskFilter
attr_accessor :mask
end
class Types::BaseObject < GraphQL::Schema::Object
extend MaskFilter
end
# ^^ do the same for other base classes
MASKING_FILTER = ->(schema_member, ctx) do
mask = schema_member.respond_to?(:mask) && schema_member.mask
mask ? mask.call(ctx) : true
end
Hope that helps!
@rmosolgo thanks for the quick answer!
It seems, that the changes require more work than I first anticipated to get masking to work. So I wondered, if you can give some advice how to implement it?
The current implementation is this: https://github.com/exAspArk/graphql-guard/blob/fb42e72112c51cde2380c0a62d31fb3d63515d45/lib/graphql/guard.rb#L57-L63
I see some room for improvement here:
class_eval
to monkey patch the .default_filter
methodGraphQL::Filter
is marked private in the docs (see https://graphql-ruby.org/api-doc/2.0.0/GraphQL/Filter)What would be a more future proof way of providing a masking feature? I found the visiblity stuff in your docs (see https://graphql-ruby.org/schema/dynamic_types.html#using-visiblecontext-1), but it seems that it's intended to dynamically switch the implementation of a type, right?
Regarding "metadata is part of the legacy type definition system": Does this imply, that using .accept_definitions
is the wrong way now? See:
https://github.com/exAspArk/graphql-guard/blob/fb42e72112c51cde2380c0a62d31fb3d63515d45/lib/graphql/guard.rb#L95-L100
Before you could write something like this
field :title, String, null: true, guard: ->(obj, args, ctx) { ctx[:current_user].admin? }
I'm not sure how I would achieve something like this using your approach with attr_accessor :mask
. Could you elaborate?
future proof way of providing a masking feature?
Yes, def self.visible?(ctx)
is the way. Here are some docs on that: https://graphql-ruby.org/authorization/visibility.html
Besides swapping implementations at runtime, you can also hide implementations at runtime -- and if a type, field, argument, etc is hidden, then it doesn't exist for the current query.
using
.accept_definitions
Yes, it was a way to bridge the gap from legacy definition to class-based definition, but it's removed in graphql-ruby 2.0.
Honestly, for class-based definitions, I would recommend a method-based approach instead of a proc-based approach. Something like this:
module GraphQL::Guard::Integration
def masked?(context)
# library users should implement this method to hide things
false
end
def visible?(context)
(!masked?(context)) && super
end
def guarded?(*args) # for objects, `[obj, ctx]`, but for fields and arguments, `[obj, args, ctx]`
# library users should implement this method to flag things as not authorized
false
end
def authorized?(*args)
(!guarded?(*args)) && super
end
end
class Types::BaseObject < GraphQL::Schema::Object
extend GraphQL::Guard::Integration
def self.guarded?(obj, ctx)
ctx[:current_user].admin?
end
end
# or:
class Types::BaseField < GraphQL::Schema::Field
include GraphQL::Guard::Integration
def guarded?(obj, args, ctx)
ctx[:current_user].admin?
end
end
More details about graphql-ruby's .authorized?
method here: https://graphql-ruby.org/authorization/authorization.html
Hope that helps!
Hello @exAspArk, are there any plans for this library to support GraphQL Ruby 1.13 or 2.0? Are you open to PRs to address this issue?
@rmosolgo thank you, this helps a lot. We are going to need something similar in https://github.com/graphql-devise/graphql_devise and I think it might help for this gem too.
It looks like our only way to implement this, will be to provide a custom field class in the gem, and that one should be able to handle what you described in https://github.com/exAspArk/graphql-guard/issues/53#issuecomment-1057079969. Is there a way to assign a default field_class
for all types of fields? From the docs it'd seem like we will have to call field_class
in every base type like enum or object. But from a gem's perspective it would be amazing if we could assign a field class for ALL in a single call. Of course, I see how that might not be possible and we might have to document this so users do this in their projects for every base type.
Yeah, there's not a way to install a field class for every base type in one line of code -- the base Object, Interface, and Mutation classes all need that configuration. I definitely agree that would be nice, but I haven't been able to figure out how it'd work...
That's fine, thank you!
Any updates? I started having this problem after updating some old libs 😞
Is still repo still active?
Sorry, I don't have the bandwidth to address it. But if anyone is willing to submit a PR, I'd be happy to review it!
FWIW to others.
My company moved away from this repo recently. GraphQL implemented some hooks for authorization: https://graphql-ruby.org/mutations/mutation_authorization.html#checking-the-user-permissions
So we switched over to the base GraphQL gem for authorization.
Also note: Rails 6.1.7.8 does not agree with the highest GraphQL version this gem allows. We were getting this error:
NameError: uninitialized constant GraphQL::Compatibility::ExecutionSpecification::SpecificationSchema::OpenStruct (NameError)
/tmp/repo/vendor/bundle/ruby/3.2.0/gems/graphql-1.12.24/lib/graphql/compatibility/execution_specification/specification_schema.rb:6:in `<module:SpecificationSchema>'
Hello,
Deprecation warnings starting graphql-1.13.1:
From changelog
Also I noticed, that inline guards return Array wrapped guard Proc and not just proc right away. Is this intended outcome in graphql or some undesired behaviour - I don't know, should investigate more.