Skip to content

[GraphQL] GlobalID / ID compatibility layer

Alex Kalderimis requested to merge ajk-globalid-compatibilty into master

What does this MR do?

This MR changes the unification rules so that variables of type ID are valid values for arguments of the more restricted GlobalID type.

What does this mean for me?

Now that this is merged, we can freely change the type of arguments that represent global IDs from ID_TYPE to ::Types::GlobalIDType[T] where T is a generic type variable for the object the ID represents.

(note that is is always OK to change the types of fields so long as they have a compatible representation - in this case ID and e.g. ProjectID are both encoded as strings, so this is fine. The reason that this is OK for fields and not arguments is that in GraphQL variable arguments need to have their nominal type declared in the query signature, but fields are always inferred. This means that changing the type of fields between types with compatible representations will never break queries)

For example if you have the following code:

class ProjectResolver < BaseResolver
  argument :id, ID_TYPE

  def resolve(id:)
    GitlabSchema.object_from_id(id, expected_type: ::Project)
  end
end

We can change it to:

class ProjectResolver < BaseResolver
  argument :id, ::Types::GlobalIDType[::Project]

  def resolve(id:)
    id = Types::GlobalIDType[Project].coerce_isolated_input(id)
    GitlabSchema.find_by_gid(id)
  end
end

Note the obligatory use of #coerce_isolated_input - this is explained below.

Eventually this becomes:

class ProjectResolver < BaseResolver
  argument :id, ::Types::GlobalIDType[::Project]

  def resolve(id:)
    GitlabSchema.find_by_gid(id)
  end
end

Why does it do this?

See: #233992 (closed)

The current state is that we have a lot of code that accepts GlobalID arguments. These are all typed as ID which is just a fancy String. In Ruby-land, they appear to us as String.

We have a better alternative, ::Types::GlobalIDType, which is safer, does parsing and validation before business logic is run and can be parameterized. Unfortunately, changing argument types is not backwards compatible.

What we want to get to is:

  • arguments are typed as GlobalIDType[Project] or GlobalIDType[Issue] or whatever
  • Clients are aware of this type, and name it in their queries
  • The framework rejects illegal/unparseable inputs
  • In our code, we get GlobalID instances

Since this is not fully backwards compatible (due to the second bit - clients need to be aware of it), we need a bridge to allow us to accept ID inputs to GlobalIDType arguments.

The major downside of this is that in Ruby-land, the value can be either a String (if the client named it as ID) or a GlobalID if the client used the newer type in the query. This means that during the transition, all code that consumes these ids must be prepared for them to be either a string or a global-ID instance.

As a convenience to the developer, they can call ::Types::GlobalIDType[ModelClass]#coerce_isolated_input to perform this coercion, raising the correct errors if the input is incorrect. So in real code, we would have something like:

class ProjectResolver < BaseResolver
  argument :id, ::Types::GlobalIDType[::Project]

  def resolve(id:)
    id = Types::GlobalIDType[Project].coerce_isolated_input(id)
    GitlabSchema.find_by_gid(id)
  end
end

Then, once the transition is complete, we just remove id = Types::GlobalIDType[Project].coerce_isolated_input(id) and passing id: ID becomes illegal.

It would be great to add a cop to enforce this, but I'm not entirely sure how to write such a thing.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Alex Kalderimis

Merge request reports