Same Interface Shows Up Multiple Times in "Implements"

[dleavitt]

Per #3613 I put interfaces in my interfaces 🎉

But now my types have too many interfaces 🤢

Specifically, if you've got a schema like this:

interface Node { ... }
interface Named implements Node { ... }
interface Timestamped implements Node { ... }

And you add a type Thing that implements Named and Timestamped, printing the schema will give you this:

type Thing implements Named & Node & Node & Timestamped { ... }

I didn't see much in the spec about this but the reference implementation doesn't like it:
https://github.com/graphql/graphql-js/blob/95dac43fd4bff037e06adaa7cfb44f497bca94a7/src/type/validate.ts#L338-L348

It's also not stable - graphql-ruby will add an additional & Node each time you parse and then print the SDL.

First commit: does it just in the printer.

Second commit: tries to do it for the schema/introspection as well. Wasn't 100% sure about where the deduping should happen though (#implements vs #interfaces vs #interface_type_memberships vs #own_interface_type_memberships.)

[rmosolgo]

D'oh! Thanks for the fix on this.

One thing on my mind is, what about the list of type_memberships, does it also have duplicates in it? (It strikes me that .uniq might clean up the return value of this method, but is the internal structure of the schema still "wrong" in some way?)

I definitely want to get this fixed, but I'd prefer a solution that builds the schema properly rather than one that cleans up structure issues at the "last minute." I suspect that what we really need is some duplicate checking in this method:

graphql-ruby/lib/graphql/schema/member/has_interfaces.rb

Line 7 in faeef68

def implements(*new_interfaces, **options)

Here's where an interfaces's interfaces are added to the object type:

graphql-ruby/lib/graphql/schema/member/has_interfaces.rb

Lines 22 to 25 in faeef68

	# If this interface has interfaces of its own, add those, too
	int.interfaces.each do |next_interface|
	implements(next_interface)
	end

Maybe that line should somehow check whether this type already implemented that interface.

Alternatively, here's some code that replaces "temporary" interface implementations with newly-created ones:

graphql-ruby/lib/graphql/schema/member/has_interfaces.rb

Lines 36 to 49 in faeef68

	# Remove any String or late-bound interfaces which are being replaced
	own_interface_type_memberships.reject! { |old_i_m|
	if !(old_i_m.respond_to?(:abstract_type) && old_i_m.abstract_type.is_a?(Module))
	old_int_type = old_i_m.respond_to?(:abstract_type) ? old_i_m.abstract_type : old_i_m
	old_name = Schema::Member::BuildType.to_type_name(old_int_type)
	new_memberships.any? { |new_i_m|
	new_int_type = new_i_m.respond_to?(:abstract_type) ? new_i_m.abstract_type : new_i_m
	new_name = Schema::Member::BuildType.to_type_name(new_int_type)
	new_name == old_name
	}
	end
	}

Sometimes, when a schema is loading, it uses Strings or LateBoundType instances here, and those objects are cleaned up by that code. But maybe that code could be expanded to replace any type membership that matches the newly-created ones.

Anyhow... not sure exactly which approach would work best, but I'd like to get the schema structure "right" on the way in somehow. Would you like to take a crack at it? If not, I can probably take a look soon.

[dleavitt]

Thanks for the quick and thoughtful response! Agree and I'll look into a righter approach.

For any other reckless interfaciers who find this through google, here's a workaround:

# BaseObject
def self.interfaces(...)
  super.uniq
end

# BaseInterface
definition_methods do
  def interfaces(...)
    super.uniq
  end
end

[dleavitt]

Added another commit along the lines of your second suggestion:

graphql-ruby/lib/graphql/schema/member/has_interfaces.rb

Lines 48 to 53 in 0f6aead

	elsif old_i_m.respond_to?(:abstract_type)
	# Remove any existing implementations of the same interface
	new_memberships.any? { |new_i_m|
	new_i_m.respond_to?(:abstract_type) && new_i_m.abstract_type == old_i_m.abstract_type
	}
	end

Tests seem to pass, but:

• Does the way I'm comparing the interfaces seem right?
• Something could implement the same interface twice but with different options on the type membership. Not sure if this matters or how we'd tell. I think the last one implemented will win right now FWIW.
• It looks like interfaces from superclasses get concatted to the return value of .interfaces, so we need to dedupe in there anyway.

[rmosolgo]

Looking good so far, thanks again for taking another look at this!

comparing the interfaces?

Yeah, I think that's good. The previous code uses names, but I think that's because it's trying to match Strings or LateBoundTypes (which only have names).

last one implemented will win

Given the options, seems good to me 👍

interfaces from superclasses get concatted

Good call 🍻

[dleavitt]

It looks like the last implementation winning and overwriting existing memberships options is what's causing the test failures. Deduplicating when new interfaces are added means that things like this won't work:

graphql-ruby/spec/graphql/schema/dynamic_members_spec.rb

Lines 196 to 197 in b957fb5

	implements HasLanguages, future_schema: true
	implements HasLanguages, future_schema: false

Seems safest to keep all the type memberships around and just dedupe the returned list of interfaces.

[rmosolgo]

🤦 D'oh, of course it does. I was thinking only of the case where you might have two interface definitions with the same name (but the interfaces are different in Ruby-land), but I forgot about that feature, where you use the same interface but provide different options to the implementation relationship. Thanks for looking into it, but sorry I pointed you in the wrong direction.

Thanks again for this fix! I'm going to follow up on one more thing on another branch (#3954)

[dleavitt]

No problem! Thanks for this amazing library!