Skip to content
代码片段 群组 项目
提交 e7d893eb 编辑于 作者: Luke Duncalfe's avatar Luke Duncalfe 提交者: Pedro Pombeiro
浏览文件

Remove some graphql-ruby v1 workarounds

As we've now upgraded to 2.x.
上级 c5ca2ef7
No related branches found
No related tags found
无相关合并请求
......@@ -207,8 +207,6 @@ Naming/FileName:
- 'qa/qa/specs/**/*'
- 'qa/tasks/**/*.rake'
- '**/*.ru'
- 'app/graphql/types/issue_connection.rb'
- 'app/graphql/types/group_connection.rb'
IgnoreExecutableScripts: true
AllowedAcronyms:
......
......@@ -6,7 +6,7 @@ class RunnerGroupsResolver < BaseResolver
include Gitlab::Graphql::Authorize::AuthorizeResource
include ResolvesGroups
type 'Types::GroupConnection', null: true
type Types::GroupType.connection_type, null: true
authorize :read_runner
authorizes_object!
......
......@@ -4,7 +4,7 @@ module Resolvers
class GroupsResolver < BaseResolver
include ResolvesGroups
type "Types::GroupConnection", null: true
type Types::GroupType.connection_type, null: true
argument :search, GraphQL::Types::String,
required: false,
......
......@@ -15,8 +15,7 @@ class BaseParentResolver < Issues::BaseResolver
raise Gitlab::Graphql::Errors::ArgumentError, Types::IssuableStateEnum::INVALID_LOCKED_MESSAGE
}
# see app/graphql/types/issue_connection.rb
type 'Types::IssueConnection', null: true
type Types::IssueType.connection_type, null: true
def resolve_with_lookahead(**args)
return Issue.none if resource_parent.nil?
......
......@@ -21,8 +21,7 @@ class IssuesResolver < Issues::BaseResolver
raise Gitlab::Graphql::Errors::ArgumentError, Types::IssuableStateEnum::INVALID_LOCKED_MESSAGE
}
# see app/graphql/types/issue_connection.rb
type 'Types::IssueConnection', null: true
type Types::IssueType.connection_type, null: true
before_connection_authorization do |nodes, current_user|
projects = nodes.map(&:project)
......
# frozen_string_literal: true
# Normally this wouldn't be needed and we could use
#
# type Types::GroupType.connection_type, null: true
#
# in a resolver. However we can end up with cyclic definitions.
# Running the spec locally can result in errors like
#
# NameError: uninitialized constant Types::GroupType
#
# or other errors. To fix this, we created this file and use
#
# type "Types::GroupConnection", null: true
#
# which gives a delayed resolution, and the proper connection type.
#
# See gitlab/app/graphql/types/ci/runner_type.rb
# Reference: https://github.com/rmosolgo/graphql-ruby/issues/3974#issuecomment-1084444214
# and https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#testing-tips-and-tricks
#
Types::GroupConnection = Types::GroupType.connection_type
# frozen_string_literal: true
# Normally this wouldn't be needed and we could use
#
# type Types::IssueType.connection_type, null: true
#
# in a resolver. However we can end up with cyclic definitions.
# Running the spec locally can result in errors like
#
# NameError: uninitialized constant Resolvers::GroupIssuesResolver
#
# or other errors. To fix this, we created this file and use
#
# type "Types::IssueConnection", null: true
#
# which gives a delayed resolution, and the proper connection type.
#
# See app/graphql/resolvers/base_issues_resolver.rb
# Reference: https://github.com/rmosolgo/graphql-ruby/issues/3974#issuecomment-1084444214
# and https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#testing-tips-and-tricks
#
Types::IssueConnection = Types::IssueType.connection_type
......@@ -2269,101 +2269,6 @@ end
`spec/requests/api/graphql/ci/pipeline_spec.rb` regardless of the query being
used to fetch the pipeline data.
- There can be possible cyclic dependencies in our GraphQL types.
See [Adding field with resolver on a Type causes "Can't determine the return type " error on a different Type](https://github.com/rmosolgo/graphql-ruby/issues/3974#issuecomment-1084444214)
and [Fix unresolved name due to cyclic definition](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84202/diffs#diff-content-32d14251082fd45412e1fdbf5820e62d157e70d2).
In particular, this can happen with `connection_type`. Typically we might use the following in a resolver:
```ruby
type Types::IssueType.connection_type, null: true
```
However this might cause a cyclic definition, which can result in errors like:
```ruby
NameError: uninitialized constant Resolvers::GroupIssuesResolver
or
GraphQL::Pagination::Connections::ImplementationMissingError
```
though you might see something different.
To fix this, we must create a new file that encapsulates the connection type,
and then reference it using double quotes. This gives a delayed resolution,
and the proper connection type. For example:
[app/graphql/resolvers/base_issues_resolver.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/graphql/resolvers/base_issues_resolver.rb)
originally contained the line
```ruby
type Types::IssueType.connection_type, null: true
```
Running the specs locally for this file caused the
`NameError: uninitialized constant Resolvers::GroupIssuesResolver` error.
The fix was to create a new file, [app/graphql/types/issue_connection.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/graphql/types/issue_connection.rb) with the
line:
```ruby
Types::IssueConnection = Types::IssueType.connection_type
```
and in [app/graphql/resolvers/base_issues_resolver.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/graphql/resolvers/base_issues_resolver.rb)
we use the line
```ruby
type "Types::IssueConnection", null: true
```
Only use this style if you are having spec failures. We should not typically
use this pattern. This issue should disappear after we've upgraded to `2.x`.
- There can be instances where a spec fails because the class is not loaded correctly.
It relates to the
[circular dependencies problem](https://github.com/rmosolgo/graphql-ruby/issues/1929) and
[Adding field with resolver on a Type causes "Can't determine the return type " error on a different Type](https://github.com/rmosolgo/graphql-ruby/issues/3974).
Unfortunately, the errors generated don't really indicate what the problem is. For example,
remove the quotes from the `Rspec.describe` in
[ee/spec/graphql/resolvers/compliance_management/merge_requests/compliance_violation_resolver_spec.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/spec/graphql/resolvers/compliance_management/merge_requests/compliance_violation_resolver_spec.rb).
Then run `rspec ee/spec/graphql/resolvers/compliance_management/merge_requests/compliance_violation_resolver_spec.rb`.
This generates errors with the expectations. For example:
```ruby
1) Resolvers::ComplianceManagement::MergeRequests::ComplianceViolationResolver#resolve user is authorized filtering the results when given an array of project IDs finds the filtered compliance violations
Failure/Error: expect(subject).to contain_exactly(compliance_violation)
expected collection contained: [#<MergeRequests::ComplianceViolation id: 4, violating_user_id: 26, merge_request_id: 4, reason: "approved_by_committer", severity_level: "low">]
actual collection contained: [#<MergeRequests::ComplianceViolation id: 4, violating_user_id: 26, merge_request_id: 4, reason: "app...er_id: 27, merge_request_id: 5, reason: "approved_by_merge_request_author", severity_level: "high">]
the extra elements were: [#<MergeRequests::ComplianceViolation id: 5, violating_user_id: 27, merge_request_id: 5, reason: "approved_by_merge_request_author", severity_level: "high">]
# ./ee/spec/graphql/resolvers/compliance_management/merge_requests/compliance_violation_resolver_spec.rb:55:in `block (6 levels) in <top (required)>'
```
However, this is not a case of the wrong result being generated, it's because of the loading order
of the `ComplianceViolationResolver` class.
The only way we've found to fix this is by quoting the class name in the spec. For example, changing
```ruby
RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolationResolver do
```
into:
```ruby
RSpec.describe 'Resolvers::ComplianceManagement::MergeRequests::ComplianceViolationResolver' do
```
See [this merge request](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/87295#note_946174036) for some discussion.
Only use this style if you are having spec failures. We should not typically use this pattern.
This issue may disappear after we've upgraded to `2.x`.
- When testing resolvers using `GraphqlHelpers#resolve`, arguments for the resolver can be handled two ways.
1. 95% of the resolver specs use arguments that are Ruby objects, as opposed to when using the GraphQL API
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe 'Resolvers::ComplianceManagement::MergeRequests::ComplianceViolationResolver',
RSpec.describe Resolvers::ComplianceManagement::MergeRequests::ComplianceViolationResolver,
feature_category: :compliance_management do
using RSpec::Parameterized::TableSyntax
......@@ -34,8 +34,6 @@
target_branch: merge_request_outside_group.target_branch)
end
let_it_be(:described_class) { Resolvers::ComplianceManagement::MergeRequests::ComplianceViolationResolver }
before do
merge_request.metrics.update!(merged_at: 3.days.ago)
merge_request2.metrics.update!(merged_at: 1.day.ago)
......
0% 加载中 .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册