From ed503d51a39943b482e917028d589cc26ec01c95 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 3 Jun 2019 19:38:16 +0200 Subject: 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. --- app/graphql/gitlab_schema.rb | 25 ++++++++++ app/graphql/types/base_object.rb | 5 ++ .../unreleased/bvl-use-global-ids-graphql.yml | 5 ++ doc/development/api_graphql_styleguide.md | 21 ++++++-- lib/gitlab/graphql/loaders/batch_model_loader.rb | 2 +- spec/graphql/features/authorization_spec.rb | 2 +- spec/graphql/gitlab_schema_spec.rb | 58 ++++++++++++++++++++++ spec/requests/api/graphql/gitlab_schema_spec.rb | 13 ++++- spec/requests/api/graphql/group_query_spec.rb | 2 +- .../api/graphql/namespace/projects_spec.rb | 2 +- 10 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/bvl-use-global-ids-graphql.yml diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index f8ad6bee21b..2e5bdbd79c8 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -45,6 +45,31 @@ class GitlabSchema < GraphQL::Schema super(query_str, **kwargs) 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 def max_query_complexity(ctx) diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb index 82b78abd573..e40059c46bb 100644 --- a/app/graphql/types/base_object.rb +++ b/app/graphql/types/base_object.rb @@ -6,5 +6,10 @@ module Types prepend Gitlab::Graphql::ExposePermissions 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 diff --git a/changelogs/unreleased/bvl-use-global-ids-graphql.yml b/changelogs/unreleased/bvl-use-global-ids-graphql.yml new file mode 100644 index 00000000000..34cb65e6001 --- /dev/null +++ b/changelogs/unreleased/bvl-use-global-ids-graphql.yml @@ -0,0 +1,5 @@ +--- +title: Use global IDs when exposing GraphQL resources +merge_request: 29080 +author: +type: added diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index 8d2bfff3a5d..38270af682e 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -32,6 +32,21 @@ a new presenter specifically for GraphQL. The presenter is initialized using the object resolved by a field, and 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 GraphQL uses [cursor based @@ -79,14 +94,14 @@ look like this: { "cursor": "Nzc=", "node": { - "id": "77", + "id": "gid://gitlab/Pipeline/77", "status": "FAILED" } }, { "cursor": "Njc=", "node": { - "id": "67", + "id": "gid://gitlab/Pipeline/67", "status": "FAILED" } } @@ -330,7 +345,7 @@ argument :project_path, GraphQL::ID_TYPE, required: true, description: "The project the merge request to mutate is in" -argument :iid, GraphQL::ID_TYPE, +argument :iid, GraphQL::STRING_TYPE, required: true, description: "The iid of the merge request to mutate" diff --git a/lib/gitlab/graphql/loaders/batch_model_loader.rb b/lib/gitlab/graphql/loaders/batch_model_loader.rb index 5a0099dc6b1..50d3293fcbb 100644 --- a/lib/gitlab/graphql/loaders/batch_model_loader.rb +++ b/lib/gitlab/graphql/loaders/batch_model_loader.rb @@ -12,7 +12,7 @@ module Gitlab # rubocop: disable CodeReuse/ActiveRecord 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.each do |model, info| ids = info.map { |i| i[:id] } diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index f5eb628a982..c427893f9cc 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -282,7 +282,7 @@ describe 'Gitlab::Graphql::Authorization' do issue_ids = issue_edges.map { |issue_edge| issue_edge['node']&.fetch('id') } 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 it 'does not check access on fields that will not be rendered' do diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index e9149f4250f..4076c1f824b 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -107,6 +107,64 @@ describe GitlabSchema do 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 described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins] end diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index b6ca9246399..28676bb02f4 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'GitlabSchema configurations' do include GraphqlHelpers - let(:project) { create(:project) } + set(:project) { create(:project) } shared_examples 'imposing query limits' do describe '#max_complexity' do @@ -136,4 +136,15 @@ describe 'GitlabSchema configurations' do post_graphql(query, current_user: nil) 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 diff --git a/spec/requests/api/graphql/group_query_spec.rb b/spec/requests/api/graphql/group_query_spec.rb index db9f2ac9dd0..e0f1e4dbe9e 100644 --- a/spec/requests/api/graphql/group_query_spec.rb +++ b/spec/requests/api/graphql/group_query_spec.rb @@ -56,7 +56,7 @@ describe 'getting group information' do post_graphql(group_query(group1), current_user: user1) 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']['path']).to eq(group1.path) expect(graphql_data['group']['description']).to eq(group1.description) diff --git a/spec/requests/api/graphql/namespace/projects_spec.rb b/spec/requests/api/graphql/namespace/projects_spec.rb index e05273da4bd..de1cd9586b6 100644 --- a/spec/requests/api/graphql/namespace/projects_spec.rb +++ b/spec/requests/api/graphql/namespace/projects_spec.rb @@ -60,7 +60,7 @@ describe 'getting projects', :nested_groups do expect(graphql_data['namespace']['projects']['edges'].size).to eq(1) 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 -- cgit v1.2.1