Commit ed503d51 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Expose IDs in GraphQL as a GlobalID

This exposes all fields named `id` as GlobalIDs so they can be used
across our entire GraphQL implementation.

When the objects loaded are `ApplicationRecord`s. We'll use our
existing batchloading to find them. Otherwise, we'll fall back to the
default implementation of `GlobalID`: Calling the `.find` method on
the class.
parent 07630b3b
...@@ -45,6 +45,31 @@ class GitlabSchema < GraphQL::Schema ...@@ -45,6 +45,31 @@ class GitlabSchema < GraphQL::Schema
super(query_str, **kwargs) super(query_str, **kwargs)
end end
def id_from_object(object)
unless object.respond_to?(:to_global_id)
# This is an error in our schema and needs to be solved. So raise a
# more meaningfull error message
raise "#{object} does not implement `to_global_id`. "\
"Include `GlobalID::Identification` into `#{object.class}"
end
object.to_global_id
end
def object_from_id(global_id)
gid = GlobalID.parse(global_id)
unless gid
raise Gitlab::Graphql::Errors::ArgumentError, "#{global_id} is not a valid GitLab id."
end
if gid.model_class < ApplicationRecord
Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find
else
gid.find
end
end
private private
def max_query_complexity(ctx) def max_query_complexity(ctx)
......
...@@ -6,5 +6,10 @@ module Types ...@@ -6,5 +6,10 @@ module Types
prepend Gitlab::Graphql::ExposePermissions prepend Gitlab::Graphql::ExposePermissions
field_class Types::BaseField field_class Types::BaseField
# All graphql fields exposing an id, should expose a global id.
def id
GitlabSchema.id_from_object(object)
end
end end
end end
---
title: Use global IDs when exposing GraphQL resources
merge_request: 29080
author:
type: added
...@@ -32,6 +32,21 @@ a new presenter specifically for GraphQL. ...@@ -32,6 +32,21 @@ a new presenter specifically for GraphQL.
The presenter is initialized using the object resolved by a field, and The presenter is initialized using the object resolved by a field, and
the context. the context.
### Exposing Global ids
When exposing an `id` field on a type, we will by default try to
expose a global id by calling `to_global_id` on the resource being
rendered.
To override this behaviour, you can implement an `id` method on the
type for which you are exposing an id. Please make sure that when
exposing a `GraphQL::ID_TYPE` using a custom method that it is
globally unique.
The records that are exposing a `full_path` as an `ID_TYPE` are one of
these exceptions. Since the full path is a unique identifier for a
`Project` or `Namespace`.
### Connection Types ### Connection Types
GraphQL uses [cursor based GraphQL uses [cursor based
...@@ -79,14 +94,14 @@ look like this: ...@@ -79,14 +94,14 @@ look like this:
{ {
"cursor": "Nzc=", "cursor": "Nzc=",
"node": { "node": {
"id": "77", "id": "gid://gitlab/Pipeline/77",
"status": "FAILED" "status": "FAILED"
} }
}, },
{ {
"cursor": "Njc=", "cursor": "Njc=",
"node": { "node": {
"id": "67", "id": "gid://gitlab/Pipeline/67",
"status": "FAILED" "status": "FAILED"
} }
} }
...@@ -330,7 +345,7 @@ argument :project_path, GraphQL::ID_TYPE, ...@@ -330,7 +345,7 @@ argument :project_path, GraphQL::ID_TYPE,
required: true, required: true,
description: "The project the merge request to mutate is in" description: "The project the merge request to mutate is in"
argument :iid, GraphQL::ID_TYPE, argument :iid, GraphQL::STRING_TYPE,
required: true, required: true,
description: "The iid of the merge request to mutate" description: "The iid of the merge request to mutate"
......
...@@ -12,7 +12,7 @@ module Gitlab ...@@ -12,7 +12,7 @@ module Gitlab
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def find def find
BatchLoader.for({ model: model_class, id: model_id }).batch do |loader_info, loader| BatchLoader.for({ model: model_class, id: model_id.to_i }).batch do |loader_info, loader|
per_model = loader_info.group_by { |info| info[:model] } per_model = loader_info.group_by { |info| info[:model] }
per_model.each do |model, info| per_model.each do |model, info|
ids = info.map { |i| i[:id] } ids = info.map { |i| i[:id] }
......
...@@ -282,7 +282,7 @@ describe 'Gitlab::Graphql::Authorization' do ...@@ -282,7 +282,7 @@ describe 'Gitlab::Graphql::Authorization' do
issue_ids = issue_edges.map { |issue_edge| issue_edge['node']&.fetch('id') } issue_ids = issue_edges.map { |issue_edge| issue_edge['node']&.fetch('id') }
expect(issue_edges.size).to eq(visible_issues.size) expect(issue_edges.size).to eq(visible_issues.size)
expect(issue_ids).to eq(visible_issues.map { |i| i.id.to_s }) expect(issue_ids).to eq(visible_issues.map { |i| i.to_global_id.to_s })
end end
it 'does not check access on fields that will not be rendered' do it 'does not check access on fields that will not be rendered' do
......
...@@ -107,6 +107,64 @@ describe GitlabSchema do ...@@ -107,6 +107,64 @@ describe GitlabSchema do
end end
end end
describe '.id_from_object' do
it 'returns a global id' do
expect(described_class.id_from_object(build(:project, id: 1))).to be_a(GlobalID)
end
it "raises a meaningful error if a global id couldn't be generated" do
expect { described_class.id_from_object(build(:commit)) }
.to raise_error(RuntimeError, /include `GlobalID::Identification` into/i)
end
end
describe '.object_from_id' do
context 'for subclasses of `ApplicationRecord`' do
it 'returns the correct record' do
user = create(:user)
result = described_class.object_from_id(user.to_global_id.to_s)
expect(result.__sync).to eq(user)
end
it 'batchloads the queries' do
user1 = create(:user)
user2 = create(:user)
expect do
[described_class.object_from_id(user1.to_global_id),
described_class.object_from_id(user2.to_global_id)].map(&:__sync)
end.not_to exceed_query_limit(1)
end
end
context 'for other classes' do
# We cannot use an anonymous class here as `GlobalID` expects `.name` not
# to return `nil`
class TestGlobalId
include GlobalID::Identification
attr_accessor :id
def initialize(id)
@id = id
end
end
it 'falls back to a regular find' do
result = TestGlobalId.new(123)
expect(TestGlobalId).to receive(:find).with("123").and_return(result)
expect(described_class.object_from_id(result.to_global_id)).to eq(result)
end
end
it 'raises the correct error on invalid input' do
expect { described_class.object_from_id("bogus id") }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
end
end
def field_instrumenters def field_instrumenters
described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins] described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins]
end end
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe 'GitlabSchema configurations' do describe 'GitlabSchema configurations' do
include GraphqlHelpers include GraphqlHelpers
let(:project) { create(:project) } set(:project) { create(:project) }
shared_examples 'imposing query limits' do shared_examples 'imposing query limits' do
describe '#max_complexity' do describe '#max_complexity' do
...@@ -136,4 +136,15 @@ describe 'GitlabSchema configurations' do ...@@ -136,4 +136,15 @@ describe 'GitlabSchema configurations' do
post_graphql(query, current_user: nil) post_graphql(query, current_user: nil)
end end
end end
context "global id's" do
it 'uses GlobalID to expose ids' do
post_graphql(graphql_query_for('project', { 'fullPath' => project.full_path }, %w(id)),
current_user: project.owner)
parsed_id = GlobalID.parse(graphql_data['project']['id'])
expect(parsed_id).to eq(project.to_global_id)
end
end
end end
...@@ -56,7 +56,7 @@ describe 'getting group information' do ...@@ -56,7 +56,7 @@ describe 'getting group information' do
post_graphql(group_query(group1), current_user: user1) post_graphql(group_query(group1), current_user: user1)
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(graphql_data['group']['id']).to eq(group1.id.to_s) expect(graphql_data['group']['id']).to eq(group1.to_global_id.to_s)
expect(graphql_data['group']['name']).to eq(group1.name) expect(graphql_data['group']['name']).to eq(group1.name)
expect(graphql_data['group']['path']).to eq(group1.path) expect(graphql_data['group']['path']).to eq(group1.path)
expect(graphql_data['group']['description']).to eq(group1.description) expect(graphql_data['group']['description']).to eq(group1.description)
......
...@@ -60,7 +60,7 @@ describe 'getting projects', :nested_groups do ...@@ -60,7 +60,7 @@ describe 'getting projects', :nested_groups do
expect(graphql_data['namespace']['projects']['edges'].size).to eq(1) expect(graphql_data['namespace']['projects']['edges'].size).to eq(1)
project = graphql_data['namespace']['projects']['edges'][0]['node'] project = graphql_data['namespace']['projects']['edges'][0]['node']
expect(project['id']).to eq(public_project.id.to_s) expect(project['id']).to eq(public_project.to_global_id.to_s)
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment