From 6f54567cce8749a5fc6b559298427762457a4511 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 25 Dec 2020 18:25:12 +0000 Subject: [PATCH 01/10] Add GraphQL verification tooling This adds query validation for GraphQL, exposed in a new raketask (`gitlab:graphql:validate`). Tests are added for the validation system, which handles common patterns in our front-end code (such as Apollo client directives, and the use of ee_else_ce). The new graphql files used in the tests are excluded from prettier formatting. A couple of clearly incorrect graphql files (discovered during testing) have been fixed. One remaining one has been marked as a known failure. --- .../queries/project_path.query.graphql | 2 +- config/known_invalid_graphql_queries.yml | 3 + .../group_specific_scanners.query.graphql | 2 +- .../instance_specific_scanners.query.graphql | 2 +- .../project_specific_scanners.query.graphql | 6 +- .../graphql/queries/author.fragment.graphql | 4 + .../graphql/queries/bad.fragment.graphql | 4 + .../graphql/queries/bad_argument.graphql | 5 + .../graphql/queries/client.query.graphql | 3 + .../graphql/queries/connection.query.graphql | 9 + .../queries/deeply/nested/bad_import.graphql | 7 + .../queries/deeply/nested/query.graphql | 7 + .../graphql/queries/duplicate_imports.graphql | 10 + .../queries/ee/author.fragment.graphql | 5 + .../graphql/queries/ee_else_ce.import.graphql | 9 + .../graphql/queries/missing_argument.graphql | 8 + .../graphql/queries/post.fragment.graphql | 8 + .../graphql/queries/post_by_slug.graphql | 7 + .../queries/post_by_slug.with_import.graphql | 9 + ...ost_by_slug.with_import.misspelled.graphql | 9 + .../graphql/queries/syntax-error.graphql | 5 + .../transitive_bad_import.fragment.graphql | 6 + .../queries/transitive_bad_import.graphql | 9 + .../gitlab/graphql/queries/typedefs.graphql | 3 + .../graphql/queries/unused_import.graphql | 8 + .../graphql/queries/wrong_field.graphql | 7 + .../queries/wrong_field.import.graphql | 7 + lib/gitlab/graphql/queries.rb | 196 ++++++++++++ lib/tasks/gitlab/graphql.rake | 42 ++- scripts/frontend/prettier.js | 2 +- spec/lib/gitlab/graphql/queries_spec.rb | 301 ++++++++++++++++++ 31 files changed, 696 insertions(+), 9 deletions(-) create mode 100644 config/known_invalid_graphql_queries.yml create mode 100644 fixtures/lib/gitlab/graphql/queries/author.fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/bad.fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/bad_argument.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/client.query.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/connection.query.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/deeply/nested/bad_import.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/deeply/nested/query.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/duplicate_imports.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/ee/author.fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/ee_else_ce.import.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/missing_argument.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/post.fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/post_by_slug.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.misspelled.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/syntax-error.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/transitive_bad_import.fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/transitive_bad_import.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/typedefs.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/unused_import.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/wrong_field.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/wrong_field.import.graphql create mode 100644 lib/gitlab/graphql/queries.rb create mode 100644 spec/lib/gitlab/graphql/queries_spec.rb diff --git a/app/assets/javascripts/repository/queries/project_path.query.graphql b/app/assets/javascripts/repository/queries/project_path.query.graphql index 74e73e075773ce..9e5c10b3de3b6b 100644 --- a/app/assets/javascripts/repository/queries/project_path.query.graphql +++ b/app/assets/javascripts/repository/queries/project_path.query.graphql @@ -1,3 +1,3 @@ query getProjectPath { - projectPath + projectPath @client } diff --git a/config/known_invalid_graphql_queries.yml b/config/known_invalid_graphql_queries.yml new file mode 100644 index 00000000000000..cc655b83ab3f9d --- /dev/null +++ b/config/known_invalid_graphql_queries.yml @@ -0,0 +1,3 @@ +--- +filenames: + - ee/app/assets/javascripts/security_configuration/dast_profiles/graphql/dast_site_profiles_extended.query.graphql diff --git a/ee/app/assets/javascripts/security_dashboard/graphql/group_specific_scanners.query.graphql b/ee/app/assets/javascripts/security_dashboard/graphql/group_specific_scanners.query.graphql index da3fcef1454af7..dac08118c590e6 100644 --- a/ee/app/assets/javascripts/security_dashboard/graphql/group_specific_scanners.query.graphql +++ b/ee/app/assets/javascripts/security_dashboard/graphql/group_specific_scanners.query.graphql @@ -1,4 +1,4 @@ -#import "./vulnerablity_scanner.fragment.graphql" +#import "./vulnerability_scanner.fragment.graphql" query groupSpecificScanners($fullPath: ID!) { group(fullPath: $fullPath) { diff --git a/ee/app/assets/javascripts/security_dashboard/graphql/instance_specific_scanners.query.graphql b/ee/app/assets/javascripts/security_dashboard/graphql/instance_specific_scanners.query.graphql index 8e569303888a9b..75524586e69e1b 100644 --- a/ee/app/assets/javascripts/security_dashboard/graphql/instance_specific_scanners.query.graphql +++ b/ee/app/assets/javascripts/security_dashboard/graphql/instance_specific_scanners.query.graphql @@ -1,4 +1,4 @@ -#import "./vulnerablity_scanner.fragment.graphql" +#import "./vulnerability_scanner.fragment.graphql" query instanceSpecificScanners { instanceSecurityDashboard { diff --git a/ee/app/assets/javascripts/security_dashboard/graphql/project_specific_scanners.query.graphql b/ee/app/assets/javascripts/security_dashboard/graphql/project_specific_scanners.query.graphql index f7f985253144dc..b916d586a8242a 100644 --- a/ee/app/assets/javascripts/security_dashboard/graphql/project_specific_scanners.query.graphql +++ b/ee/app/assets/javascripts/security_dashboard/graphql/project_specific_scanners.query.graphql @@ -1,7 +1,7 @@ -#import "./vulnerablity_scanner.fragment.graphql" +#import "./vulnerability_scanner.fragment.graphql" -query projectSpecificScanners($fullpath: id!) { - project(fullPath: $fullPath) { +query projectSpecificScanners($fullpath: ID!) { + project(fullPath: $fullpath) { vulnerabilityScanners { nodes { ...VulnerabilityScanner diff --git a/fixtures/lib/gitlab/graphql/queries/author.fragment.graphql b/fixtures/lib/gitlab/graphql/queries/author.fragment.graphql new file mode 100644 index 00000000000000..a10af1b3217823 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/author.fragment.graphql @@ -0,0 +1,4 @@ +fragment AuthorF on Author { + name + handle +} diff --git a/fixtures/lib/gitlab/graphql/queries/bad.fragment.graphql b/fixtures/lib/gitlab/graphql/queries/bad.fragment.graphql new file mode 100644 index 00000000000000..00d868792d2847 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/bad.fragment.graphql @@ -0,0 +1,4 @@ +fragment BadF on Blog { + wibble + wobble +} diff --git a/fixtures/lib/gitlab/graphql/queries/bad_argument.graphql b/fixtures/lib/gitlab/graphql/queries/bad_argument.graphql new file mode 100644 index 00000000000000..0b704d717dd5f4 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/bad_argument.graphql @@ -0,0 +1,5 @@ +query($bad: String) { + blog(title: $bad) { + description + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/client.query.graphql b/fixtures/lib/gitlab/graphql/queries/client.query.graphql new file mode 100644 index 00000000000000..dabe9735064dfb --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/client.query.graphql @@ -0,0 +1,3 @@ +query { + thingy @client +} diff --git a/fixtures/lib/gitlab/graphql/queries/connection.query.graphql b/fixtures/lib/gitlab/graphql/queries/connection.query.graphql new file mode 100644 index 00000000000000..eec3f9b867b6c1 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/connection.query.graphql @@ -0,0 +1,9 @@ +query($slug: String!) { + post(slug: $slug) { + author { + posts @connection(key: "posts") { + title + } + } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/deeply/nested/bad_import.graphql b/fixtures/lib/gitlab/graphql/queries/deeply/nested/bad_import.graphql new file mode 100644 index 00000000000000..2a83b9dd42c437 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/deeply/nested/bad_import.graphql @@ -0,0 +1,7 @@ +# import "../author.fragment.graphql" + +query($slug: String!) { + post(slug: $slug) { + author { ...AuthorF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/deeply/nested/query.graphql b/fixtures/lib/gitlab/graphql/queries/deeply/nested/query.graphql new file mode 100644 index 00000000000000..451d3c25f25179 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/deeply/nested/query.graphql @@ -0,0 +1,7 @@ +# import "../../author.fragment.graphql" + +query($slug: String!) { + post(slug: $slug) { + author { ...AuthorF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/duplicate_imports.graphql b/fixtures/lib/gitlab/graphql/queries/duplicate_imports.graphql new file mode 100644 index 00000000000000..de3ac9fa833658 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/duplicate_imports.graphql @@ -0,0 +1,10 @@ +# import "./author.fragment.graphql" +# import "./post.fragment.graphql" + +query($title: String!) { + blog(title: $title) { + description + mainAuthor { ...AuthorF } + posts { ...PostF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/ee/author.fragment.graphql b/fixtures/lib/gitlab/graphql/queries/ee/author.fragment.graphql new file mode 100644 index 00000000000000..884b683c5634e0 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/ee/author.fragment.graphql @@ -0,0 +1,5 @@ +fragment AuthorF on Author { + name + handle + verified +} diff --git a/fixtures/lib/gitlab/graphql/queries/ee_else_ce.import.graphql b/fixtures/lib/gitlab/graphql/queries/ee_else_ce.import.graphql new file mode 100644 index 00000000000000..5a4d0320eb6293 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/ee_else_ce.import.graphql @@ -0,0 +1,9 @@ +#import "ee_else_ce/author.fragment.graphql" + +query { + post(slug: "validating-queries") { + title + content + author { ...AuthorF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/missing_argument.graphql b/fixtures/lib/gitlab/graphql/queries/missing_argument.graphql new file mode 100644 index 00000000000000..499495d9363a35 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/missing_argument.graphql @@ -0,0 +1,8 @@ +query { + blog { + title + posts { + title + } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/post.fragment.graphql b/fixtures/lib/gitlab/graphql/queries/post.fragment.graphql new file mode 100644 index 00000000000000..0049964cfa5ad0 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/post.fragment.graphql @@ -0,0 +1,8 @@ +# import "./author.fragment.graphql" + +fragment PostF on Post { + name + title + content + author { ...AuthorF } +} diff --git a/fixtures/lib/gitlab/graphql/queries/post_by_slug.graphql b/fixtures/lib/gitlab/graphql/queries/post_by_slug.graphql new file mode 100644 index 00000000000000..a7febc39e817bf --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/post_by_slug.graphql @@ -0,0 +1,7 @@ +query { + post(slug: "validating-queries") { + title + content + author { name } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.graphql b/fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.graphql new file mode 100644 index 00000000000000..fef763c42833f2 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.graphql @@ -0,0 +1,9 @@ +#import "./author.fragment.graphql" + +query { + post(slug: "validating-queries") { + title + content + author { ...AuthorF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.misspelled.graphql b/fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.misspelled.graphql new file mode 100644 index 00000000000000..4b205860e6eb82 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/post_by_slug.with_import.misspelled.graphql @@ -0,0 +1,9 @@ +# import "./auther.fragment.graphql" + +query { + post(slug: "validating-queries") { + title + content + author { ...AuthorF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/syntax-error.graphql b/fixtures/lib/gitlab/graphql/queries/syntax-error.graphql new file mode 100644 index 00000000000000..f7d2730951d303 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/syntax-error.graphql @@ -0,0 +1,5 @@ +query } + blog(title: "boom") { + description + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/transitive_bad_import.fragment.graphql b/fixtures/lib/gitlab/graphql/queries/transitive_bad_import.fragment.graphql new file mode 100644 index 00000000000000..1a0769279ef7be --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/transitive_bad_import.fragment.graphql @@ -0,0 +1,6 @@ +# import "does-not-exist.graphql" + +fragment AuthorF on Author { + name + handle +} diff --git a/fixtures/lib/gitlab/graphql/queries/transitive_bad_import.graphql b/fixtures/lib/gitlab/graphql/queries/transitive_bad_import.graphql new file mode 100644 index 00000000000000..6520fd8d412226 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/transitive_bad_import.graphql @@ -0,0 +1,9 @@ +# import "./transitive_bad_import.fragment.graphql" + +query($slug: String!) { + post(slug: $slug) { + title + content + author { ...AuthorF } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/typedefs.graphql b/fixtures/lib/gitlab/graphql/queries/typedefs.graphql new file mode 100644 index 00000000000000..e25298661fd2f3 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/typedefs.graphql @@ -0,0 +1,3 @@ +type Author { + name: String +} diff --git a/fixtures/lib/gitlab/graphql/queries/unused_import.graphql b/fixtures/lib/gitlab/graphql/queries/unused_import.graphql new file mode 100644 index 00000000000000..19e9de90e81cc4 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/unused_import.graphql @@ -0,0 +1,8 @@ +# import "./author.fragment.graphql" + +query($slug: String!) { + post(slug: $slug) { + title + content + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/wrong_field.graphql b/fixtures/lib/gitlab/graphql/queries/wrong_field.graphql new file mode 100644 index 00000000000000..7903854c84e905 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/wrong_field.graphql @@ -0,0 +1,7 @@ +query { + blog(title: "A history of GraphQL") { + title + createdAt + categories { name } + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/wrong_field.import.graphql b/fixtures/lib/gitlab/graphql/queries/wrong_field.import.graphql new file mode 100644 index 00000000000000..534154e2877f55 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/wrong_field.import.graphql @@ -0,0 +1,7 @@ +# import "./bad.fragment.graphql" + +query($title: String!) { + blog(title: $title) { + ...BadF + } +} diff --git a/lib/gitlab/graphql/queries.rb b/lib/gitlab/graphql/queries.rb new file mode 100644 index 00000000000000..99a6f4a00546e1 --- /dev/null +++ b/lib/gitlab/graphql/queries.rb @@ -0,0 +1,196 @@ +# frozen_string_literal: true + +require 'find' + +module Gitlab + module Graphql + module Queries + IMPORT_RE = /^#\s*import "(?[^"]+)"$/m.freeze + # TODO: validate both the EE and CE versions + EE_ELSE_CE = /^ee_else_ce/.freeze + HOME_RE = /^~/.freeze + HOME_EE = %r{^ee/}.freeze + DOTS_RE = %r{^(\.\./)+}.freeze + DOT_RE = %r{^\./}.freeze + IMPLICIT_ROOT = %r{^app/}.freeze + CLIENT_DIRECTIVE = /@client/.freeze + CONN_DIRECTIVE = /@connection\(key: "\w+"\)/.freeze + + class WrappedError + delegate :message, to: :@error + + def initialize(error) + @error = error + end + + def path + [] + end + end + + class FileNotFound + def initialize(file) + @file = file + end + + def message + "File not found: #{@file}" + end + + def path + [] + end + end + + class Definition + attr_reader :file, :imports + + def initialize(path, fragments) + @file = path + @fragments = fragments + @imports = [] + @errors = [] + @ee_else_ce = [] + end + + def text(ee = false) + qs = [query] + all_imports(ee).uniq.sort.map { |p| fragment(p).query } + qs.join("\n\n").gsub(/\n\n+/, "\n\n") + end + + def query + return @query if defined?(@query) + + # CONN_DIRECTIVEs are purely client-side constructs + @query = File.read(file).gsub(CONN_DIRECTIVE, '').gsub(IMPORT_RE) do + path = $~[:path] + + if EE_ELSE_CE.match?(path) + @ee_else_ce << path.gsub(EE_ELSE_CE, '') + else + @imports << fragment_path(path) + end + + '' + end + rescue Errno::ENOENT + @errors << FileNotFound.new(file) + @query = nil + end + + def all_imports(ee = false) + return [] if query.nil? + + home = ee ? @fragments.home_ee : @fragments.home + eithers = @ee_else_ce.map { |p| home + p } + + (imports + eithers).flat_map { |p| [p] + @fragments.get(p).all_imports(ee) } + end + + def all_errors + return @errors.to_set if query.nil? + + paths = imports + @ee_else_ce.flat_map { |p| [@fragments.home + p, @fragments.home_ee + p] } + + paths.map { |p| fragment(p).all_errors }.reduce(@errors.to_set) { |a, b| a | b } + end + + def fragment(path) + @fragments.get(path) + end + + def fragment_path(import_path) + frag_path = import_path.gsub(HOME_RE, @fragments.home) + frag_path = frag_path.gsub(HOME_EE, @fragments.home_ee + '/') + frag_path = frag_path.gsub(DOT_RE) do + Pathname.new(file).parent.to_s + '/' + end + frag_path = frag_path.gsub(DOTS_RE) do |dots| + rel_dir(dots.split('/').count) + end + frag_path = frag_path.gsub(IMPLICIT_ROOT) do + (Rails.root / 'app').to_s + '/' + end + + frag_path + end + + def rel_dir(n_steps_up) + path = Pathname.new(file).parent + while n_steps_up > 0 + path = path.parent + n_steps_up -= 1 + end + + path.to_s + '/' + end + + # TODO: move these warnings to the rake task + def validate(schema) + return [:client_query, []] if CLIENT_DIRECTIVE.match?(text) + + errs = all_errors.presence || schema.validate(text) + if @ee_else_ce.present? + errs += schema.validate(text(true)) + end + + [:validated, errs] + rescue ::GraphQL::ParseError => e + [:validated, [WrappedError.new(e)]] + end + end + + class Fragments + def initialize(root, dir = 'app/assets/javascripts') + @root = root + @store = {} + @dir = dir + end + + def home + @home ||= (@root / @dir).to_s + end + + def home_ee + @home_ee ||= (@root / 'ee' / @dir).to_s + end + + def get(frag_path) + @store[frag_path] ||= Definition.new(frag_path, self) + end + end + + def self.find(root) + definitions = [] + + ::Find.find(root.to_s) do |path| + next unless path.ends_with?('.graphql') + next if path.ends_with?('.fragment.graphql') + next if path.ends_with?('typedefs.graphql') + + definitions << Definition.new(path, fragments) + end + + definitions + rescue Errno::ENOENT + [] # root does not exist + end + + def self.fragments + @fragments ||= Fragments.new(Rails.root) + end + + def self.all + ['.', 'ee'].flat_map do |prefix| + find(Rails.root / prefix / 'app/assets/javascripts') + end + end + + def self.known_failure?(path) + @known_failures ||= YAML.safe_load(File.read(Rails.root.join('config', 'known_invalid_graphql_queries.yml'))) + + @known_failures.fetch('filenames', []).any? { |known_failure| path.to_s.ends_with?(known_failure) } + end + end + end +end diff --git a/lib/tasks/gitlab/graphql.rake b/lib/tasks/gitlab/graphql.rake index 5a583183924f90..f708114c226929 100644 --- a/lib/tasks/gitlab/graphql.rake +++ b/lib/tasks/gitlab/graphql.rake @@ -33,6 +33,44 @@ namespace :gitlab do ) namespace :graphql do + desc 'Gitlab | GraphQL | Validate queries' + task validate: [:environment, :enable_feature_flags] do |t, args| + queries = if args.to_a.present? + args.to_a.flat_map { |path| Gitlab::Graphql::Queries.find(path) } + else + Gitlab::Graphql::Queries.all + end + + failed = queries.flat_map do |defn| + summary, errs = defn.validate(GitlabSchema) + + case summary + when :client_query + warn("SKIP #{defn.file}: client query") + else + warn("OK #{defn.file}") if errs.empty? + errs.each do |err| + warn(<<~MSG) + ERROR #{defn.file}: #{err.message} (at #{err.path.join('.')}) + MSG + end + end + + errs.empty? ? [] : [defn.file] + end + + if failed.present? + format_output( + "#{failed.count} GraphQL #{'query'.pluralize(failed.count)} out of #{queries.count} failed validation:", + *failed.map do |name| + known_failure = Gitlab::Graphql::Queries.known_failure?(name) + "- #{name}" + (known_failure ? ' (known failure)' : '') + end + ) + abort unless failed.all? { |name| Gitlab::Graphql::Queries.known_failure?(name) } + end + end + desc 'GitLab | GraphQL | Generate GraphQL docs' task compile_docs: [:environment, :enable_feature_flags] do renderer = Gitlab::Graphql::Docs::Renderer.new(GitlabSchema.graphql_definition, render_options) @@ -78,11 +116,11 @@ def render_options } end -def format_output(str) +def format_output(*strs) heading = '#' * 10 puts heading puts '#' - puts "# #{str}" + strs.each { |str| puts "# #{str}" } puts '#' puts heading end diff --git a/scripts/frontend/prettier.js b/scripts/frontend/prettier.js index 8e9ecc2ba85f32..f721e46f36bd37 100644 --- a/scripts/frontend/prettier.js +++ b/scripts/frontend/prettier.js @@ -7,7 +7,7 @@ const matchExtensions = ['js', 'vue', 'graphql']; // This will improve glob performance by excluding certain directories. // The .prettierignore file will also be respected, but after the glob has executed. -const globIgnore = ['**/node_modules/**', 'vendor/**', 'public/**']; +const globIgnore = ['**/node_modules/**', 'vendor/**', 'public/**', 'fixtures/**']; const readFileAsync = (file, options) => new Promise((resolve, reject) => { diff --git a/spec/lib/gitlab/graphql/queries_spec.rb b/spec/lib/gitlab/graphql/queries_spec.rb new file mode 100644 index 00000000000000..853abd9717ce03 --- /dev/null +++ b/spec/lib/gitlab/graphql/queries_spec.rb @@ -0,0 +1,301 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require "test_prof/recipes/rspec/let_it_be" + +RSpec.describe Gitlab::Graphql::Queries do + shared_examples 'a valid GraphQL query for the blog schema' do + it 'is valid' do + expect(subject.validate(schema).second).to be_empty + end + end + + shared_examples 'an invalid GraphQL query for the blog schema' do + it 'is invalid' do + expect(subject.validate(schema).second).to match errors + end + end + + # Toy schema to validate queries against + let_it_be(:schema) do + author = Class.new(GraphQL::Schema::Object) do + graphql_name 'Author' + field :name, GraphQL::STRING_TYPE, null: true + field :handle, GraphQL::STRING_TYPE, null: false + field :verified, GraphQL::BOOLEAN_TYPE, null: false + end + + post = Class.new(GraphQL::Schema::Object) do + graphql_name 'Post' + field :name, GraphQL::STRING_TYPE, null: false + field :title, GraphQL::STRING_TYPE, null: false + field :content, GraphQL::STRING_TYPE, null: true + field :author, author, null: false + end + author.field :posts, [post], null: false do + argument :blog_title, GraphQL::STRING_TYPE, required: false + end + + blog = Class.new(GraphQL::Schema::Object) do + graphql_name 'Blog' + field :title, GraphQL::STRING_TYPE, null: false + field :description, GraphQL::STRING_TYPE, null: false + field :main_author, author, null: false + field :posts, [post], null: false + field :post, post, null: true do + argument :slug, GraphQL::STRING_TYPE, required: true + end + end + + Class.new(GraphQL::Schema) do + query(Class.new(GraphQL::Schema::Object) do + graphql_name 'Query' + field :blog, blog, null: true do + argument :title, GraphQL::STRING_TYPE, required: true + end + field :post, post, null: true do + argument :slug, GraphQL::STRING_TYPE, required: true + end + end) + end + end + + let(:root) do + Rails.root / 'fixtures/lib/gitlab/graphql/queries' + end + + describe Gitlab::Graphql::Queries::Fragments do + subject { described_class.new(root) } + + it 'has the right home' do + expect(subject.home).to eq (root / 'app/assets/javascripts').to_s + end + + it 'has the right EE home' do + expect(subject.home_ee).to eq (root / 'ee/app/assets/javascripts').to_s + end + + it 'caches query definitions' do + fragment = subject.get('foo') + + expect(fragment).to be_a(::Gitlab::Graphql::Queries::Definition) + expect(subject.get('foo')).to be fragment + end + end + + describe '.all' do + it 'is the combination of finding queries in CE and EE' do + expect(described_class) + .to receive(:find).with(Rails.root / 'app/assets/javascripts').and_return([:ce]) + expect(described_class) + .to receive(:find).with(Rails.root / 'ee/app/assets/javascripts').and_return([:ee]) + + expect(described_class.all).to eq([:ce, :ee]) + end + end + + describe '.find' do + def definition_of(path) + be_a(::Gitlab::Graphql::Queries::Definition) + .and(have_attributes(file: path.to_s)) + end + + it 'find a single specific file' do + path = root / 'post_by_slug.graphql' + + expect(described_class.find(path)).to contain_exactly(definition_of(path)) + end + + it 'ignores files that do not exist' do + path = root / 'not_there.graphql' + + expect(described_class.find(path)).to be_empty + end + + it 'ignores fragments' do + path = root / 'author.fragment.graphql' + + expect(described_class.find(path)).to be_empty + end + + it 'ignores typedefs' do + path = root / 'typedefs.graphql' + + expect(described_class.find(path)).to be_empty + end + + it 'finds all query definitions under a root directory' do + found = described_class.find(root) + + expect(found).to include( + definition_of(root / 'post_by_slug.graphql'), + definition_of(root / 'post_by_slug.with_import.graphql'), + definition_of(root / 'post_by_slug.with_import.misspelled.graphql'), + definition_of(root / 'duplicate_imports.graphql'), + definition_of(root / 'deeply/nested/query.graphql') + ) + + expect(found).not_to include( + definition_of(root / 'typedefs.graphql'), + definition_of(root / 'author.fragment.graphql') + ) + end + end + + describe Gitlab::Graphql::Queries::Definition do + let(:fragments) { Gitlab::Graphql::Queries::Fragments.new(root, '.') } + + subject { described_class.new(root / path, fragments) } + + context 'a simple query' do + let(:path) { 'post_by_slug.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a query with an import' do + let(:path) { 'post_by_slug.with_import.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a query with duplicate imports' do + let(:path) { 'duplicate_imports.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a query importing from ee_else_ce' do + let(:path) { 'ee_else_ce.import.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + + it 'can resolve the ee fields' do + expect(subject.text(false)).not_to include('verified') + expect(subject.text(true)).to include('verified') + end + end + + context 'a query refering to parent directories' do + let(:path) { 'deeply/nested/query.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a query refering to parent directories, incorrectly' do + let(:path) { 'deeply/nested/bad_import.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + be_a(::Gitlab::Graphql::Queries::FileNotFound) + .and(have_attributes(message: include('deeply/author.fragment.graphql'))) + ) + end + end + end + + context 'a query with a broken import' do + let(:path) { 'post_by_slug.with_import.misspelled.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + be_a(::Gitlab::Graphql::Queries::FileNotFound) + .and(have_attributes(message: include('auther.fragment.graphql'))) + ) + end + end + end + + context 'a query which imports a file with a broken import' do + let(:path) { 'transitive_bad_import.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + be_a(::Gitlab::Graphql::Queries::FileNotFound) + .and(have_attributes(message: include('does-not-exist.graphql'))) + ) + end + end + end + + context 'a query containing a client directive' do + let(:path) { 'client.query.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + + it 'is tagged as a client query' do + expect(subject.validate(schema).first).to eq :client_query + end + end + + context 'a query containing a connection directive' do + let(:path) { 'connection.query.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a query which mentions an incorrect field' do + let(:path) { 'wrong_field.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + have_attributes(message: /'createdAt' doesn't exist/), + have_attributes(message: /'categories' doesn't exist/) + ) + end + end + end + + context 'a query which has a missing argument' do + let(:path) { 'missing_argument.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + have_attributes(message: include('blog')) + ) + end + end + end + + context 'a query which has a bad argument' do + let(:path) { 'bad_argument.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + have_attributes(message: include('Nullability mismatch on variable $bad')) + ) + end + end + end + + context 'a query which has a syntax error' do + let(:path) { 'syntax-error.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + have_attributes(message: include('Parse error')) + ) + end + end + end + + context 'a query which has an unused import' do + let(:path) { 'unused_import.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly( + have_attributes(message: include('AuthorF was defined, but not used')) + ) + end + end + end + end +end -- GitLab From 0f7c3b7070a2a53fd0657af265f816daf71652df Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 29 Dec 2020 15:51:59 +0000 Subject: [PATCH 02/10] Add new CI job to validate GraphQL queries --- .gitlab-ci.yml | 1 + .gitlab/ci/graphql-ci.yml | 7 +++++++ 2 files changed, 8 insertions(+) create mode 100644 .gitlab/ci/graphql-ci.yml diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 79ca127e42f6b9..7c471fdf8a0330 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -110,3 +110,4 @@ include: - local: .gitlab/ci/notify.gitlab-ci.yml - local: .gitlab/ci/dast.gitlab-ci.yml - local: .gitlab/ci/workhorse.gitlab-ci.yml + - local: .gitlab/ci/graphql-ci.yml diff --git a/.gitlab/ci/graphql-ci.yml b/.gitlab/ci/graphql-ci.yml new file mode 100644 index 00000000000000..ba948da4907cc7 --- /dev/null +++ b/.gitlab/ci/graphql-ci.yml @@ -0,0 +1,7 @@ +graphql-query-validate: + extends: + - .static-analysis-base + stage: test + needs: ["setup-test-env"] + script: + - bundle exec rake gitlab:graphql:validate -- GitLab From 6e515cdd3ccbf0772fbbc5a1ee70972dc9f004d7 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 4 Jan 2021 10:24:42 +0000 Subject: [PATCH 03/10] Improve code-quality in Queries.find --- lib/gitlab/graphql/queries.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/graphql/queries.rb b/lib/gitlab/graphql/queries.rb index 99a6f4a00546e1..7bb604f04eebe8 100644 --- a/lib/gitlab/graphql/queries.rb +++ b/lib/gitlab/graphql/queries.rb @@ -164,11 +164,7 @@ def self.find(root) definitions = [] ::Find.find(root.to_s) do |path| - next unless path.ends_with?('.graphql') - next if path.ends_with?('.fragment.graphql') - next if path.ends_with?('typedefs.graphql') - - definitions << Definition.new(path, fragments) + definitions << Definition.new(path, fragments) if query?(path) end definitions @@ -191,6 +187,12 @@ def self.known_failure?(path) @known_failures.fetch('filenames', []).any? { |known_failure| path.to_s.ends_with?(known_failure) } end + + def self.query?(path) + path.ends_with?('.graphql') && + !path.ends_with?('.fragment.graphql') && + !path.ends_with?('typedefs.graphql') + end end end end -- GitLab From bbc8aae6da21cbf0d64f778c0ff98bf9be80e6f3 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 4 Jan 2021 16:04:42 +0000 Subject: [PATCH 04/10] Consolidate graphql verification tasks This combines our schema and docs verification tasks with the new query validation task. Since the new task is not doc specific, the rules section for this has been renamed. --- .gitlab-ci.yml | 2 +- .gitlab/ci/docs.gitlab-ci.yml | 13 ------------- .gitlab/ci/graphql-ci.yml | 7 ------- .gitlab/ci/graphql.gitlab-ci.yml | 12 ++++++++++++ .gitlab/ci/rules.gitlab-ci.yml | 6 +++++- 5 files changed, 18 insertions(+), 22 deletions(-) delete mode 100644 .gitlab/ci/graphql-ci.yml create mode 100644 .gitlab/ci/graphql.gitlab-ci.yml diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 7c471fdf8a0330..b41d7bcd34ffc9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -110,4 +110,4 @@ include: - local: .gitlab/ci/notify.gitlab-ci.yml - local: .gitlab/ci/dast.gitlab-ci.yml - local: .gitlab/ci/workhorse.gitlab-ci.yml - - local: .gitlab/ci/graphql-ci.yml + - local: .gitlab/ci/graphql.gitlab-ci.yml diff --git a/.gitlab/ci/docs.gitlab-ci.yml b/.gitlab/ci/docs.gitlab-ci.yml index d6dc709a11a0c8..c9eb782935b40f 100644 --- a/.gitlab/ci/docs.gitlab-ci.yml +++ b/.gitlab/ci/docs.gitlab-ci.yml @@ -84,16 +84,3 @@ ui-docs-links lint: needs: [] script: - bundle exec haml-lint -i DocumentationLinks - -graphql-reference-verify: - extends: - - .default-retry - - .rails-cache - - .default-before_script - - .docs:rules:graphql-reference-verify - - .use-pg11 - stage: test - needs: ["setup-test-env"] - script: - - bundle exec rake gitlab:graphql:check_docs - - bundle exec rake gitlab:graphql:check_schema diff --git a/.gitlab/ci/graphql-ci.yml b/.gitlab/ci/graphql-ci.yml deleted file mode 100644 index ba948da4907cc7..00000000000000 --- a/.gitlab/ci/graphql-ci.yml +++ /dev/null @@ -1,7 +0,0 @@ -graphql-query-validate: - extends: - - .static-analysis-base - stage: test - needs: ["setup-test-env"] - script: - - bundle exec rake gitlab:graphql:validate diff --git a/.gitlab/ci/graphql.gitlab-ci.yml b/.gitlab/ci/graphql.gitlab-ci.yml new file mode 100644 index 00000000000000..e2ee21b62832e7 --- /dev/null +++ b/.gitlab/ci/graphql.gitlab-ci.yml @@ -0,0 +1,12 @@ +graphql-verify: + extends: + - .default-retry + - .rails-cache + - .default-before_script + - .graphql:rules:graphql-verify + stage: test + needs: ["setup-test-env"] + script: + - bundle exec rake gitlab:graphql:validate + - bundle exec rake gitlab:graphql:check_docs + - bundle exec rake gitlab:graphql:check_schema diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 43e7b7ab745153..9ea5538f643483 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -349,7 +349,11 @@ changes: *docs-patterns when: on_success -.docs:rules:graphql-reference-verify: +################## +# GraphQL rules # +################## + +.graphql:rules:graphql-verify: rules: - <<: *if-not-ee when: never -- GitLab From d1e2ccf548f631be937099580e85c64b09312913 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 4 Jan 2021 16:33:42 +0000 Subject: [PATCH 05/10] Disable DB setup in graphql-verification jobs --- .gitlab/ci/graphql.gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitlab/ci/graphql.gitlab-ci.yml b/.gitlab/ci/graphql.gitlab-ci.yml index e2ee21b62832e7..46eaf97cbe749b 100644 --- a/.gitlab/ci/graphql.gitlab-ci.yml +++ b/.gitlab/ci/graphql.gitlab-ci.yml @@ -1,4 +1,6 @@ graphql-verify: + variables: + SETUP_DB: "false" extends: - .default-retry - .rails-cache -- GitLab From 8d2087b377070c6eff45f66cfd7debd923a4b29f Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 4 Jan 2021 21:49:27 +0000 Subject: [PATCH 06/10] Remove TODO comments --- lib/gitlab/graphql/queries.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/gitlab/graphql/queries.rb b/lib/gitlab/graphql/queries.rb index 7bb604f04eebe8..7b7518a9c74295 100644 --- a/lib/gitlab/graphql/queries.rb +++ b/lib/gitlab/graphql/queries.rb @@ -6,7 +6,6 @@ module Gitlab module Graphql module Queries IMPORT_RE = /^#\s*import "(?[^"]+)"$/m.freeze - # TODO: validate both the EE and CE versions EE_ELSE_CE = /^ee_else_ce/.freeze HOME_RE = /^~/.freeze HOME_EE = %r{^ee/}.freeze @@ -125,7 +124,6 @@ def rel_dir(n_steps_up) path.to_s + '/' end - # TODO: move these warnings to the rake task def validate(schema) return [:client_query, []] if CLIENT_DIRECTIVE.match?(text) -- GitLab From f3c1ef8b64e43ca4cafe1d61240bf8e1a405fa73 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 5 Jan 2021 11:09:49 +0000 Subject: [PATCH 07/10] Apply 1 suggestion(s) to 1 file(s) --- .gitlab/ci/graphql.gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab/ci/graphql.gitlab-ci.yml b/.gitlab/ci/graphql.gitlab-ci.yml index 46eaf97cbe749b..4aff0ef630672c 100644 --- a/.gitlab/ci/graphql.gitlab-ci.yml +++ b/.gitlab/ci/graphql.gitlab-ci.yml @@ -7,7 +7,7 @@ graphql-verify: - .default-before_script - .graphql:rules:graphql-verify stage: test - needs: ["setup-test-env"] + needs: [] script: - bundle exec rake gitlab:graphql:validate - bundle exec rake gitlab:graphql:check_docs -- GitLab From f94230efa3f3ae90ff32c42b40d118d0d89349a2 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 5 Jan 2021 11:26:11 +0000 Subject: [PATCH 08/10] Hide private methods, and avoid boolean blindness --- lib/gitlab/graphql/queries.rb | 38 +++++++++++++------------ spec/lib/gitlab/graphql/queries_spec.rb | 4 +-- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/gitlab/graphql/queries.rb b/lib/gitlab/graphql/queries.rb index 7b7518a9c74295..d951ca5e704837 100644 --- a/lib/gitlab/graphql/queries.rb +++ b/lib/gitlab/graphql/queries.rb @@ -52,8 +52,8 @@ def initialize(path, fragments) @ee_else_ce = [] end - def text(ee = false) - qs = [query] + all_imports(ee).uniq.sort.map { |p| fragment(p).query } + def text(mode: :ce) + qs = [query] + all_imports(mode: mode).uniq.sort.map { |p| fragment(p).query } qs.join("\n\n").gsub(/\n\n+/, "\n\n") end @@ -77,13 +77,13 @@ def query @query = nil end - def all_imports(ee = false) + def all_imports(mode: :ce) return [] if query.nil? - home = ee ? @fragments.home_ee : @fragments.home + home = mode == :ee ? @fragments.home_ee : @fragments.home eithers = @ee_else_ce.map { |p| home + p } - (imports + eithers).flat_map { |p| [p] + @fragments.get(p).all_imports(ee) } + (imports + eithers).flat_map { |p| [p] + @fragments.get(p).all_imports(mode: mode) } end def all_errors @@ -94,6 +94,21 @@ def all_errors paths.map { |p| fragment(p).all_errors }.reduce(@errors.to_set) { |a, b| a | b } end + def validate(schema) + return [:client_query, []] if CLIENT_DIRECTIVE.match?(text) + + errs = all_errors.presence || schema.validate(text) + if @ee_else_ce.present? + errs += schema.validate(text(mode: :ee)) + end + + [:validated, errs] + rescue ::GraphQL::ParseError => e + [:validated, [WrappedError.new(e)]] + end + + private + def fragment(path) @fragments.get(path) end @@ -123,19 +138,6 @@ def rel_dir(n_steps_up) path.to_s + '/' end - - def validate(schema) - return [:client_query, []] if CLIENT_DIRECTIVE.match?(text) - - errs = all_errors.presence || schema.validate(text) - if @ee_else_ce.present? - errs += schema.validate(text(true)) - end - - [:validated, errs] - rescue ::GraphQL::ParseError => e - [:validated, [WrappedError.new(e)]] - end end class Fragments diff --git a/spec/lib/gitlab/graphql/queries_spec.rb b/spec/lib/gitlab/graphql/queries_spec.rb index 853abd9717ce03..0021fe5b974ee5 100644 --- a/spec/lib/gitlab/graphql/queries_spec.rb +++ b/spec/lib/gitlab/graphql/queries_spec.rb @@ -171,8 +171,8 @@ def definition_of(path) it_behaves_like 'a valid GraphQL query for the blog schema' it 'can resolve the ee fields' do - expect(subject.text(false)).not_to include('verified') - expect(subject.text(true)).to include('verified') + expect(subject.text(mode: :ce)).not_to include('verified') + expect(subject.text(mode: :ee)).to include('verified') end end -- GitLab From 540737a2192d9f10dbc9a0d07581302e54b59414 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 5 Jan 2021 12:01:34 +0000 Subject: [PATCH 09/10] Add new known failures --- config/known_invalid_graphql_queries.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/known_invalid_graphql_queries.yml b/config/known_invalid_graphql_queries.yml index cc655b83ab3f9d..e8c9eaec8df4f7 100644 --- a/config/known_invalid_graphql_queries.yml +++ b/config/known_invalid_graphql_queries.yml @@ -1,3 +1,5 @@ --- filenames: + - ee/app/assets/javascripts/on_demand_scans/graphql/dast_scan_create.mutation.graphql + - ee/app/assets/javascripts/oncall_schedules/graphql/mutations/update_oncall_schedule_rotation.mutation.graphql - ee/app/assets/javascripts/security_configuration/dast_profiles/graphql/dast_site_profiles_extended.query.graphql -- GitLab From 8412bdf4e42fec365e17e716fb2e4202799eb27f Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 5 Jan 2021 17:28:12 +0000 Subject: [PATCH 10/10] This uses more sophisticated client query processing Rather than skipping all queries that use the @client directive, we instead remove all such fields (and any arguments and fragments mentioned in the skipped sections) and then only skip the query if that then leaves us with an empty query. The query transformation is handled with a query printer. --- config/known_invalid_graphql_queries.yml | 1 - .../queries/client_unused_fragment.graphql | 7 ++ .../queries/mixed_client.query.graphql | 7 ++ .../mixed_client_invalid.query.graphql | 7 ++ .../mixed_client_skipped_argument.graphql | 11 +++ .../mixed_client_unused_fragment.graphql | 11 +++ .../graphql/queries/thingy.fragment.graphql | 3 + lib/gitlab/graphql/queries.rb | 94 ++++++++++++++++++- spec/lib/gitlab/graphql/queries_spec.rb | 42 +++++++++ 9 files changed, 179 insertions(+), 4 deletions(-) create mode 100644 fixtures/lib/gitlab/graphql/queries/client_unused_fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/mixed_client.query.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/mixed_client_invalid.query.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/mixed_client_skipped_argument.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/mixed_client_unused_fragment.graphql create mode 100644 fixtures/lib/gitlab/graphql/queries/thingy.fragment.graphql diff --git a/config/known_invalid_graphql_queries.yml b/config/known_invalid_graphql_queries.yml index e8c9eaec8df4f7..770366d76cf35f 100644 --- a/config/known_invalid_graphql_queries.yml +++ b/config/known_invalid_graphql_queries.yml @@ -2,4 +2,3 @@ filenames: - ee/app/assets/javascripts/on_demand_scans/graphql/dast_scan_create.mutation.graphql - ee/app/assets/javascripts/oncall_schedules/graphql/mutations/update_oncall_schedule_rotation.mutation.graphql - - ee/app/assets/javascripts/security_configuration/dast_profiles/graphql/dast_site_profiles_extended.query.graphql diff --git a/fixtures/lib/gitlab/graphql/queries/client_unused_fragment.graphql b/fixtures/lib/gitlab/graphql/queries/client_unused_fragment.graphql new file mode 100644 index 00000000000000..2beba1812c9407 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/client_unused_fragment.graphql @@ -0,0 +1,7 @@ +#import "./thingy.fragment.graphql" + +query($slug: String!, $foo: String) { + thingy(someArg: $foo) @client { + ...ThingyF + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/mixed_client.query.graphql b/fixtures/lib/gitlab/graphql/queries/mixed_client.query.graphql new file mode 100644 index 00000000000000..f98d070958c5cd --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/mixed_client.query.graphql @@ -0,0 +1,7 @@ +query { + thingy @client + post(slug: "validating-queries") { + title + otherThing @client + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/mixed_client_invalid.query.graphql b/fixtures/lib/gitlab/graphql/queries/mixed_client_invalid.query.graphql new file mode 100644 index 00000000000000..e97c133c5ca4a6 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/mixed_client_invalid.query.graphql @@ -0,0 +1,7 @@ +query { + thingy @client + post(slug: "validating-queries") { + titlz + otherThing @client + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/mixed_client_skipped_argument.graphql b/fixtures/lib/gitlab/graphql/queries/mixed_client_skipped_argument.graphql new file mode 100644 index 00000000000000..a54890085f19b8 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/mixed_client_skipped_argument.graphql @@ -0,0 +1,11 @@ +query($slug: String!, $foo: String) { + thingy(someArg: $foo) @client { + x + y + z + } + post(slug: $slug) { + title + otherThing @client + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/mixed_client_unused_fragment.graphql b/fixtures/lib/gitlab/graphql/queries/mixed_client_unused_fragment.graphql new file mode 100644 index 00000000000000..0f4e92319abad0 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/mixed_client_unused_fragment.graphql @@ -0,0 +1,11 @@ +#import "./thingy.fragment.graphql" + +query($slug: String!, $foo: String) { + thingy(someArg: $foo) @client { + ...ThingyF + } + post(slug: $slug) { + title + otherThing @client + } +} diff --git a/fixtures/lib/gitlab/graphql/queries/thingy.fragment.graphql b/fixtures/lib/gitlab/graphql/queries/thingy.fragment.graphql new file mode 100644 index 00000000000000..2f95d647eb3f92 --- /dev/null +++ b/fixtures/lib/gitlab/graphql/queries/thingy.fragment.graphql @@ -0,0 +1,3 @@ +fragment ThingyF on Thingy { + x y z +} diff --git a/lib/gitlab/graphql/queries.rb b/lib/gitlab/graphql/queries.rb index d951ca5e704837..de9717434909fc 100644 --- a/lib/gitlab/graphql/queries.rb +++ b/lib/gitlab/graphql/queries.rb @@ -12,7 +12,6 @@ module Queries DOTS_RE = %r{^(\.\./)+}.freeze DOT_RE = %r{^\./}.freeze IMPLICIT_ROOT = %r{^app/}.freeze - CLIENT_DIRECTIVE = /@client/.freeze CONN_DIRECTIVE = /@connection\(key: "\w+"\)/.freeze class WrappedError @@ -41,6 +40,87 @@ def path end end + # We need to re-write queries to remove all @client fields. Ideally we + # would do that as a source-to-source transformation of the AST, but doing it using a + # printer is much simpler. + class ClientFieldRedactor < GraphQL::Language::Printer + attr_reader :fields_printed, :skipped_arguments, :printed_arguments, :used_fragments + + def initialize(skips = true) + @skips = skips + @fields_printed = 0 + @in_operation = false + @skipped_arguments = [].to_set + @printed_arguments = [].to_set + @used_fragments = [].to_set + @skipped_fragments = [].to_set + @used_fragments = [].to_set + end + + def print_variable_identifier(variable_identifier) + @printed_arguments << variable_identifier.name + super + end + + def print_fragment_spread(fragment_spread, indent: "") + @used_fragments << fragment_spread.name + super + end + + def print_operation_definition(op, indent: "") + @in_operation = true + out = +"#{indent}#{op.operation_type}" + out << " #{op.name}" if op.name + + # Do these first, so that we detect any skipped arguments + dirs = print_directives(op.directives) + sels = print_selections(op.selections, indent: indent) + + # remove variable definitions only used in skipped (client) fields + vars = op.variables.reject do |v| + @skipped_arguments.include?(v.name) && !@printed_arguments.include?(v.name) + end + + if vars.any? + out << "(#{vars.map { |v| print_variable_definition(v) }.join(", ")})" + end + + out + dirs + sels + ensure + @in_operation = false + end + + def print_field(field, indent: '') + if skips? && field.directives.any? { |d| d.name == 'client' } + skipped = self.class.new(false) + + skipped.print_node(field) + @skipped_fragments |= skipped.used_fragments + @skipped_arguments |= skipped.printed_arguments + + return '' + end + + ret = super + + @fields_printed += 1 if @in_operation && ret != '' + + ret + end + + def print_fragment_definition(fragment_def, indent: "") + if skips? && @skipped_fragments.include?(fragment_def.name) && !@used_fragments.include?(fragment_def.name) + return '' + end + + super + end + + def skips? + @skips + end + end + class Definition attr_reader :file, :imports @@ -54,7 +134,15 @@ def initialize(path, fragments) def text(mode: :ce) qs = [query] + all_imports(mode: mode).uniq.sort.map { |p| fragment(p).query } - qs.join("\n\n").gsub(/\n\n+/, "\n\n") + t = qs.join("\n\n").gsub(/\n\n+/, "\n\n") + + return t unless /@client/.match?(t) + + doc = ::GraphQL.parse(t) + printer = ClientFieldRedactor.new + redacted = doc.dup.to_query_string(printer: printer) + + return redacted if printer.fields_printed > 0 end def query @@ -95,7 +183,7 @@ def all_errors end def validate(schema) - return [:client_query, []] if CLIENT_DIRECTIVE.match?(text) + return [:client_query, []] if query.present? && text.nil? errs = all_errors.presence || schema.validate(text) if @ee_else_ce.present? diff --git a/spec/lib/gitlab/graphql/queries_spec.rb b/spec/lib/gitlab/graphql/queries_spec.rb index 0021fe5b974ee5..6e08a87523f782 100644 --- a/spec/lib/gitlab/graphql/queries_spec.rb +++ b/spec/lib/gitlab/graphql/queries_spec.rb @@ -231,6 +231,48 @@ def definition_of(path) end end + context 'a mixed client query, valid' do + let(:path) { 'mixed_client.query.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + + it 'is not tagged as a client query' do + expect(subject.validate(schema).first).not_to eq :client_query + end + end + + context 'a mixed client query, with skipped argument' do + let(:path) { 'mixed_client_skipped_argument.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a mixed client query, with unused fragment' do + let(:path) { 'mixed_client_unused_fragment.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + end + + context 'a client query, with unused fragment' do + let(:path) { 'client_unused_fragment.graphql' } + + it_behaves_like 'a valid GraphQL query for the blog schema' + + it 'is tagged as a client query' do + expect(subject.validate(schema).first).to eq :client_query + end + end + + context 'a mixed client query, invalid' do + let(:path) { 'mixed_client_invalid.query.graphql' } + + it_behaves_like 'an invalid GraphQL query for the blog schema' do + let(:errors) do + contain_exactly(have_attributes(message: include('titlz'))) + end + end + end + context 'a query containing a connection directive' do let(:path) { 'connection.query.graphql' } -- GitLab