Deprecation warnings with graphql-1.13.1

[exaspark]

Deprecation warnings with graphql-1.13.1

[grxy]

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?

[23tux]

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:

graphql-guard/lib/graphql/guard.rb

Lines 16 to 19 in fb42e72

	MASKING_FILTER = ->(schema_member, ctx) do
	mask = schema_member.graphql_definition.metadata[:mask]
	mask ? mask.call(ctx) : true
	end

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?

[23tux]

@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:

graphql-guard/lib/graphql/guard.rb

Lines 57 to 63 in fb42e72

	def add_schema_masking!(schema_definition)
	schema_definition.class_eval do
	def self.default_filter
	GraphQL::Filter.new(except: default_mask).merge(only: MASKING_FILTER)
	end
	end
	end

I see some room for improvement here:

• I don't like using class_eval to monkey patch the .default_filter method
• I noticed that GraphQL::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:

graphql-guard/lib/graphql/guard.rb

Lines 95 to 100 in fb42e72

	GraphQL::ObjectType.accepts_definitions(guard: GraphQL::Define.assign_metadata_key(:guard))
	GraphQL::Field.accepts_definitions(guard: GraphQL::Define.assign_metadata_key(:guard))
	GraphQL::Field.accepts_definitions(mask: GraphQL::Define.assign_metadata_key(:mask))
	GraphQL::Schema::Object.accepts_definition(:guard)
	GraphQL::Schema::Field.accepts_definition(:guard)
	GraphQL::Schema::Field.accepts_definition(:mask)

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?

[rmosolgo]

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...

[pierry01]

👍 up

[asgeo1]

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.

[23tux]

Is there any news on this? I get the same warnings when updating to graphql-ruby 1.13.8

[23tux]

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>'

[rmosolgo]

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]

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!

[mcelicalderon]

@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 #53 (comment). 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.

[mcelicalderon]

That's fine, thank you!