Visibility leaves dangling types

[gmac]

Not sure if this is a bug or not... it looks like Visibility and Warden put different priority on fields belonging to objects and interfaces. Consider:

interface Sprocket {
  size: Int # Interface implements field as public
}

type Widget implements Sprocket {
  size: Int # Object implements field as private
}

It looks like Warden would resolve Widget.size as public, because the interface would have the final say on its fields. With Visibility it seems the object field has final say and goes private.

In some ways the Visibility implementation makes sense because the more specific implementation wins, EXCEPT – we're allowing the object to renege on its interface. This turns into a footgun that allows developers to compose invalid schemas; best case scenario the more restrictive override should probably error.

I'm seeing echoes of this in other places with dangling objects and interfaces left in the schema without fields or implementations. On one hand this is a feature of faster visibility, on the other hand it's an open door for sloppy mistakes. I'm curious if there's a middle ground here that capitalizes on Visibility advantages while also enforcing strong schema guarantees, even if only in certain circumstances?

[rmosolgo]

middle ground

I'm open to revisiting this. At first, I wrote it off because the new approach didn't have any top-level caches. But now it does, so this might be possible.

Regarding fields in particular, I think the skip_visible local variable here is responsible for this behavior:

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

Lines 150 to 157 in 19dcf75

	skip_visible = context.respond_to?(:types) && context.types.is_a?(GraphQL::Schema::Visibility::Profile)
	i = 0
	while (ancestor = ancs[i])
	if ancestor.respond_to?(:own_fields) &&
	visible_interface_implementation?(ancestor, context, warden) &&
	(f_entry = ancestor.own_fields[field_name]) &&
	(skip_visible || (f_entry = Warden.visible_entry?(:visible_field?, f_entry, context, warden)))
	return (skip_visible ? f_entry : f_entry.ensure_loaded)

I think that's needed because Visibility::Profile handles array entries itself (instead of using Warden.visible_entry?), and the difference in handling inherited fields may actually be a mistake here. Perhaps that code could be modified to use some different check in place of Warden.visible_entry? when it detects a Visibility profile in use.

In Warden, abstract types with no members were hidden. I didn't implement this in Visibility::Profile because I wanted to support incrementally loading type definitions in development. But in practice I think load_all_types became necessary more often than I wished. So maybe the compatibility won't cost so much after all. The first place I would look at adding that is here:

graphql-ruby/lib/graphql/schema/visibility/profile.rb

Line 121 in 19dcf75

if @cached_visible[t_defn] && referenced?(t_defn)

and

graphql-ruby/lib/graphql/schema/visibility/profile.rb

Line 131 in 19dcf75

if t && @cached_visible[t] && referenced?(t)

Perhaps it could call into @cached_possible_types. I just took a quick look at the implementation of that and it looks like it might not call load_all_types after all.

If you're interested in exploring it, I hope those clues are helpful. Otherwise I'll take a look soon, probably early next week.

[gmac]

Looking at this again a bit closer, my main reservation of the new system is that it leaves unreachable elements in the schema, which is a bit of a "soft" security risk. Another example:

enum WidgetOrder { # << visibility unspecified
  ID
}

type Widget { # << visibility hidden
  id: ID
}

type Query {
  widget(order: WidgetOrder): Widget # << visibility unspecified, but hidden via Widget return
}

So in the above, WidgetOrder is a newly-visible orphan within the schema. The hidden Widget type took care of hiding the field that returns it, however it left the argument enum dangling in the schema. While this is all correctable using manual visibility definitions (and is arguably more correct to require manual definition everywhere), it's also a sharp edge leaving the door open for mistakes that can now leak schema. In the end, I think I generally lean towards trusting the tool to prevent human error here if the visibility system now has the necessary information anyway.

[rmosolgo]

👀 I'm surprised that didn't turn up in my migration notes, I guess I didn't have a setup like that in my tests.

sharp edge

It's true, that switching could result in publishing some orphan types. But a schema dump in source control would catch that (is it fair to assume that those are set up in the wild?). I built migration mode to help with this too, but if this case doesn't appear in my notes, then I don't know for sure that migration mode would cover it. I think it would need a call to Schema.types, where Widget is hidden -- schema dump or introspection query would do this.

[rmosolgo]

I added a failing test for that case here: #5291

I'll see if it's possible to implement just like Warden.