diff options
Diffstat (limited to 'doc/development/api_graphql_styleguide.md')
| -rw-r--r-- | doc/development/api_graphql_styleguide.md | 246 |
1 files changed, 231 insertions, 15 deletions
diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index f807ed0f85e..de6840b2c6c 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -94,11 +94,16 @@ discussed in [Nullable fields](#nullable-fields). - Lowering the global limits for query complexity and depth. - Anything else that can result in queries hitting a limit that previously was allowed. -Fields that use the [`feature_flag` property](#feature_flag-property) and the flag is disabled by default are exempt -from the deprecation process, and can be removed at any time without notice. - See the [deprecating schema items](#deprecating-schema-items) section for how to deprecate items. +### Breaking change exemptions + +Two scenarios exist where schema items are exempt from the deprecation process, +and can be removed or changed at any time without notice. These are schema items that either: + +- Use the [`feature_flag` property](#feature_flag-property) _and_ the flag is disabled by default. +- Are [marked as alpha](#marking-schema-items-as-alpha). + ## Global IDs The GitLab GraphQL API uses Global IDs (i.e: `"gid://gitlab/MyObject/123"`) @@ -718,6 +723,28 @@ aware of the support. The documentation will mention that the old Global ID style is now deprecated. +## Marking schema items as Alpha + +Fields, arguments, enum values, and mutations can be marked as being in +[alpha](https://about.gitlab.com/handbook/product/gitlab-the-product/#alpha-beta-ga). + +An item marked as "alpha" is exempt from the deprecation process and can be removed +at any time without notice. + +This leverages GraphQL deprecations to cause the schema item to appear as deprecated, +and will be described as being in "alpha" in our generated docs and its GraphQL description. + +To mark a schema item as being in "alpha", use the `deprecated:` keyword with `reason: :alpha`. +You must provide the `milestone:` that introduced the alpha item. + +For example: + +```ruby +field :token, GraphQL::Types::String, null: true, + deprecated: { reason: :alpha, milestone: '10.0' }, + description: 'Token for login.' +``` + ## Enums GitLab GraphQL enums are defined in `app/graphql/types`. When defining new enums, the @@ -1848,35 +1875,59 @@ field :created_at, Types::TimeType, null: true, description: 'Timestamp of when ## Testing -### Writing unit tests +For testing mutations and resolvers, consider the unit of +test a full GraphQL request, not a call to a resolver. The reasons for this are +that we want to avoid lots of coupling to the framework, since this makes +upgrades to dependencies much more difficult. -Before creating unit tests, review the following examples: +You should: -- [`spec/graphql/resolvers/users_resolver_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/graphql/resolvers/users_resolver_spec.rb) -- [`spec/graphql/mutations/issues/create_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/graphql/mutations/issues/create_spec.rb) +- Prefer request specs (either using the full API endpoint or going through + `GitlabSchema.execute`) to unit specs for resolvers and mutations. +- Prefer `GraphqlHelpers#execute_query` and `GraphqlHelpers#run_with_clean_state` to + `GraphqlHelpers#resolve` and `GraphqlHelpers#resolve_field`. -It's faster to test as much of the logic from your GraphQL queries and mutations -with unit tests, which are stored in `spec/graphql`. +For example: -Use unit tests to verify that: +```ruby +# Good: +gql_query = %q(some query text...) +post_graphql(gql_query, current_user: current_user) +# or: +GitlabSchema.execute(gql_query, context: { current_user: current_user }) -- Types have the expected fields. -- Resolvers and mutations apply authorizations and return expected data. -- Edge cases are handled correctly. +# Deprecated: avoid +resolve(described_class, obj: project, ctx: { current_user: current_user }) +``` + +### Writing unit tests (deprecated) + +WARNING: +Avoid writing unit tests if the same thing can be tested with +a full GraphQL request. + +Before creating unit tests, review the following examples: + +- [`spec/graphql/resolvers/users_resolver_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/graphql/resolvers/users_resolver_spec.rb) +- [`spec/graphql/mutations/issues/create_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/graphql/mutations/issues/create_spec.rb) ### Writing integration tests Integration tests check the full stack for a GraphQL query or mutation and are stored in `spec/requests/api/graphql`. -For speed, you should test most logic in unit tests instead of integration tests. -However, integration tests that check if data is returned verify the following +For speed, consider calling `GitlabSchema.execute` directly, or making use +of smaller test schemas that only contain the types under test. + +However, full request integration tests that check if data is returned verify the following additional items: - The mutation is actually queryable in the schema (was mounted in `MutationType`). - The data returned by a resolver or mutation correctly matches the [return types](https://graphql-ruby.org/fields/introduction.html#field-return-type) of the fields and resolves without errors. +- The arguments coerce correctly on input, and the fields serialize correctly + on output. Integration tests can also verify the following items, because they invoke the full stack: @@ -1929,6 +1980,55 @@ end ### Testing tips and tricks +- Become familiar with the methods in the `GraphqlHelpers` support module. + Many of these methods make writing GraphQL tests easier. + +- Use traversal helpers like `GraphqlHelpers#graphql_data_at` and + `GraphqlHelpers#graphql_dig_at` to access result fields. For example: + + ```ruby + result = GitlabSchema.execute(query) + + mr_iid = graphql_dig_at(result.to_h, :data, :project, :merge_request, :iid) + ``` + +- Use `GraphqlHelpers#a_graphql_entity_for` to match against results. + For example: + + ```ruby + post_graphql(some_query) + + # checks that it is a hash containing { id => global_id_of(issue) } + expect(graphql_data_at(:project, :issues, :nodes)) + .to contain_exactly(a_graphql_entity_for(issue)) + + # Additional fields can be passed, either as names of methods, or with values + expect(graphql_data_at(:project, :issues, :nodes)) + .to contain_exactly(a_graphql_entity_for(issue, :iid, :title, created_at: some_time)) + ``` + +- Use `GraphqlHelpers#empty_schema` to create an empty schema, rather than creating + one by hand. For example: + + ```ruby + # good + let(:schema) { empty_schema } + + # bad + let(:query_type) { GraphQL::ObjectType.new } + let(:schema) { GraphQL::Schema.define(query: query_type, mutation: nil)} + ``` + +- Use `GraphqlHelpers#query_double(schema: nil)` of `double('query', schema: nil)`. For example: + + ```ruby + # good + let(:query) { query_double(schema: GitlabSchema) } + + # bad + let(:query) { double('Query', schema: GitlabSchema) } + ``` + - Avoid false positives: Authenticating a user with the `current_user:` argument for `post_graphql` @@ -1983,6 +2083,122 @@ 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 within 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`. Normally 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 + ``` + + 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: + + ```ruby + module Types + # rubocop: disable Graphql/AuthorizeTypes + class IssueConnectionType < CountableConnectionType + end + end + + Types::IssueConnectionType.prepend_mod_with('Types::IssueConnectionType') + ``` + + in [types/issue_connection_type.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/graphql/types/issue_connection_type.rb) + defines a new `Types::IssueConnectionType`, and is then referenced in + [app/graphql/resolvers/base_issues_resolver.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/graphql/resolvers/base_issues_resolver.rb) + + ```ruby + type "Types::IssueConnection", null: true + ``` + + Only use this style if you are having spec failures. This is not intended to be a new + pattern that we use. This issue may 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.descrbe` 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. This is not intended to be a new + pattern that we use. 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 + only strings and integers are used. This works fine in most cases. + 1. If your resolver takes arguments that use a `prepare` proc, such as a resolver that accepts timeframe + arguments (`TimeFrameArguments`), you must pass the `arg_style: :internal_prepared` parameter into + the `resolve` method. This tells the code to convert the arguments into strings and integers and pass + them through regular argument handling, ensuring that the `prepare` proc is called correctly. + For example in [`iterations_resolver_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/spec/graphql/resolvers/iterations_resolver_spec.rb): + + ```ruby + def resolve_group_iterations(args = {}, obj = group, context = { current_user: current_user }) + resolve(described_class, obj: obj, args: args, ctx: context, arg_style: :internal_prepared) + end + ``` + + One additional caveat is that if you are passing enums as a resolver argument, you must use the + external representation of the enum, rather than the internal. For example: + + ```ruby + # good + resolve_group_iterations({ search: search, in: ['CADENCE_TITLE'] }) + + # bad + resolve_group_iterations({ search: search, in: [:cadence_title] }) + ``` + + The use of `:internal_prepared` was added as a bridge for the + [GraphQL gem](https://graphql-ruby.org) upgrade. Testing resolvers directly will be + [removed eventually](https://gitlab.com/gitlab-org/gitlab/-/issues/363121), + and writing unit tests for resolvers/mutations is + [already deprecated](#writing-unit-tests-deprecated) + ## Notes about Query flow and GraphQL infrastructure The GitLab GraphQL infrastructure can be found in `lib/gitlab/graphql`. |