Visibility profiles should always evaluate based on profile context

[gmac]

Describe the bug

I'm playing around with the new visibility features and really like the direction this is going. Nice work. The DX is pretty unintuitive though because of all the nuanced interactions. It could be a lot simpler and more reliable, IMO. Consider:

class TestSchema < GraphQL::Schema
  use GraphQL::Schema::Visibility, profiles: {
    public: { public: true },
    internal: { internal: true },
  }
end

class PublicType < GraphQL::Schema::Object
  def self.visible?(context)
    super && (context[:internal] || context[:public])
  end
end

class InternalType < GraphQL::Schema::Object
  def self.visible?(context)
    super && context[:internal]
  end
end

# This works...
TestSchema.execute(
  context: { internal: true },
  visibility_profile: :internal,
)

# This doesn't work...
TestSchema.execute(
  context: {},
  visibility_profile: :internal,
)

# This does the wrong thing...
TestSchema.execute(
  context: { public: true },
  visibility_profile: :internal,
)

This gets even more nuanced with the preload attribute in play:

class TestSchema < GraphQL::Schema
  use GraphQL::Schema::Visibility, preload: true, profiles: {
    public: { public: true },
    internal: { internal: true },
  }
end

# This works...
TestSchema.execute(
  context: { internal: true },
  visibility_profile: :internal,
)

# This works...
TestSchema.execute(
  context: {},
  visibility_profile: :internal,
)

# This works, but is counter-intuitive as to why...
TestSchema.execute(
  context: { public: true },
  visibility_profile: :internal,
)

Versions

graphql version: 2.4

Expected behavior

What I'd expect would be for visibility to always favor evaluating based on the profile definition. The profile settings should always be provided to visible? as a separate argument or through context, then documentation should indicate that we should always check static profile settings before other aspects of request context, ie:

# Profile definitions...
class TestSchema < GraphQL::Schema
  use GraphQL::Schema::Visibility, profiles: {
    public: { public: true },
    internal: { internal: true },
  }
end

# Visibility checks...
class Widget < GraphQL::Schema::Object
  def self.visible?(context)
    super && context.visibility_profile_settings[:internal]
  end
end

# This works consistently because we evaluate based on profile settings...
TestSchema.execute(
  context: {},
  visibility_profile: :internal,
)

Actual behavior

Visibility without preloading computes based on request context, which may or may not match what's defined for the visibility profile. Preloaded profiles may not match request context. Also, docs say that visibility_profile is a context key, when it actually appears to be a query argument.

[rmosolgo]

Hey @gmac, thanks for reviewing this feature and sharing your thoughts. I definitely want to work out these rough edges you ran into.

context[:visibility_profile]: Hmm, I'm not sure how this mistake got through. At least one bit of code is still trying to use context[:visibility_profile] (here). I think I should migrate everything to use context[:visibility_profile] but retain the keyword argument for compat (with a deprecation warning).

profiles: without preload:. What this does is creates aVisibility::Profile instance the first time each profile is used, and re-uses that for subsequent queries. But it doesn't pass the configured profile's context to .visible? calls -- instead, it currently uses the context from the first query that uses the profile. Following from your suggestion, a better implementation might be pass just the configured context (from profiles: ...). This would ensure that the preconfigured context is sufficient for implementing the visibility profile and prevent any weirdness where the first query's context is unusual in some way. What do you think of that approach?

[gmac]

I think I should migrate everything to use context[:visibility_profile] but retain the keyword argument for compat (with a deprecation warning).

I don't have strong feelings here as long as it works one way. Once I followed the code and found that it came from a query argument, that actually seemed pretty sensible – it's more deliberate than stuffing magic keys into context. That said, we already have a long history of magic context keys, so for us the context approach is almost more conventional.

A better implementation might be pass just the configured context (from profiles: ...). This would ensure that the preconfigured context is sufficient for implementing the visibility profile and prevent any weirdness where the first query's context is unusual in some way.

I think I'd stick with this suggestion that makes both request context and profile settings available:

class Widget < GraphQL::Schema::Object
  def self.visible?(context)
    super && context.visibility_profile_settings[:internal]
  end
end

The reason there is the very useful dynamic: true option which solves a huge problem for us... we mostly use static visibility profiles, but occasionally hook a feature to a beta flag that we enable for select API clients. Clients with these special beta flags will need to use dynamic visibility, which is based on context. So, our visibility check wants to look something like this, which considers both the static profile settings and the live context:

class Widget < GraphQL::Schema::Object
  def self.visible?(context)
    super && begin
      if (profile_settings = context.visibility_profile_settings)
        profile_settings[:internal]
      else
        context[:api_client].beta_enabled?(:my_beta_flag)
      end
    end
  end
end

At the front of the request, we'd then simply choose not to declare a visibility profile for clients that we see have certain beta flag sensitivities.

Another possible approach would be to increase argument arity to visible?(context, profile), but that's highly breaking and not particularly better than just formalizing access to the current visibility profile settings through context.

[gmac]

Actually, follow-up thought here: maybe we could get the best of both worlds… in a request that defines visibility_profile, then it would make sense to pass off just the profile settings as context to visible? checks, whereas requests without visibility_profile would get the full request context. This would completely block profile-based visibility from ever evaluating incorrectly due to developer error, while still fully supporting the dynamic visibility fallback case.

This puts pretty strong gravity on the visibility_profile setting given that it will affect which context gets passed into visible? calls. Thus, I think that sways me towards keeping it as a formal query argument.

[rmosolgo]

Agreed on how those contexts should work. I took a crack at it here: #5235