Skip to content

Remove unneeded overrides of find_object

What does this MR do and why?

This MR removes around 30 unneeded overrides of the find_object method in GraphQL mutations. This is a second pass at cleaning up duplicate instances of this method, see !114555 (merged) for details of the first pass.

Many of the removed methods make use of an :expected_type check. This is no longer required in most cases (i.e. those removed here) so these mutations do not need to override the base class implementation of find_object.

The :expected_type checks date back to when ids were all represented by a single generic ID type. This has been replaced with specific types of the form GlobalIdType[::Project], GlobalIdType[::Issue], etc.

These specific types already handle the type checking when the GID argument is parsed. This happens before resolve is called on the mutation. So when an argument is defined as GlobalIdType[::Project] passing a GID of the wrong type (like gid://gitlab/Issue/1) will fail before the :expected_type check is ever performed.

See #257883 (closed) for more context of the history of this.

Note on behaviour changes

Many of the find_object implementations that are being removed here are slightly different to the default base class method.

The removed methods are of the form:

def find_object(id: id)
  ::GitlabSchema.object_from_id(id, expected_type: ::SomeModel)
end

whereas the base implementation looks like this:

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

The object_from_id version performs 2 actions, it parses the string to a Gitlab::GlobalId and then calls find_by_gid.

So with this MR, all of the mutations with object_from_id change behaviour from:

gid = parse_gid(id)
find_by_gid(gid)

to

find_by_gid(id)

This is ok as the first step is not required because the object referenced by id will already be a parsed GID instance. In the methods being removed we were calling a redundant second parse, which is effectively a noop.

[2] pry(main)> gid = GitlabSchema.parse_gid('gid://gitlab/Project/1')
=> #<GlobalID:0x00007f9906603f50 @uri=#<URI::GID gid://gitlab/Project/1>>
[3] pry(main)> GitlabSchema.parse_gid(gid)
=> #<GlobalID:0x00007f9906603f50 @uri=#<URI::GID gid://gitlab/Project/1>>

Summary

Broadly there are 3 types of changes in this MR split into 3 commits:

  • Removal of find_object overrides without extra modification. These will fall back to the implementation from Gitlab::Graphql::Authorize::AuthorizeResource
  • Update of calls from authorized_find!(id) to authorized_find!(id: id) and subsequent removal of redundant find_object overrides.
  • Modification of mutation unit specs to use a GID instance rather than a string. The latter will never be passed to these mutations when executed in a full stack GraphQL call.
Edited by Malcolm Locke

Merge request reports