From 5ced2d8d7d9107f031894c5b16908db8bf6b913f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 23 Aug 2017 15:09:20 +0200 Subject: Rename CI/CD job triggering policy class to Policy --- lib/gitlab/ci/config/entry/job.rb | 4 ++-- lib/gitlab/ci/config/entry/policy.rb | 18 ++++++++++++++++++ lib/gitlab/ci/config/entry/trigger.rb | 18 ------------------ 3 files changed, 20 insertions(+), 20 deletions(-) create mode 100644 lib/gitlab/ci/config/entry/policy.rb delete mode 100644 lib/gitlab/ci/config/entry/trigger.rb (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 32f5c6ab142..91aac6df4b1 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -59,10 +59,10 @@ module Gitlab entry :services, Entry::Services, description: 'Services that will be used to execute this job.' - entry :only, Entry::Trigger, + entry :only, Entry::Policy, description: 'Refs policy this job will be executed for.' - entry :except, Entry::Trigger, + entry :except, Entry::Policy, description: 'Refs policy this job will be executed for.' entry :variables, Entry::Variables, diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb new file mode 100644 index 00000000000..a08ab8a9d14 --- /dev/null +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -0,0 +1,18 @@ +module Gitlab + module Ci + class Config + module Entry + ## + # Entry that represents a trigger policy for the job. + # + class Policy < Node + include Validatable + + validations do + validates :config, array_of_strings_or_regexps: true + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/trigger.rb b/lib/gitlab/ci/config/entry/trigger.rb deleted file mode 100644 index 16b234e6c59..00000000000 --- a/lib/gitlab/ci/config/entry/trigger.rb +++ /dev/null @@ -1,18 +0,0 @@ -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents a trigger policy for the job. - # - class Trigger < Node - include Validatable - - validations do - validates :config, array_of_strings_or_regexps: true - end - end - end - end - end -end -- cgit v1.2.1 From 0d7d7c1057c80e930d56363f2efd0519e1462586 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 24 Aug 2017 14:54:27 +0200 Subject: Use aspect-oriented design in CI/CD config entries --- lib/gitlab/ci/config/entry/configurable.rb | 3 ++- lib/gitlab/ci/config/entry/node.rb | 11 ++++++----- lib/gitlab/ci/config/entry/validatable.rb | 11 +++++++++++ lib/gitlab/ci/config/entry/validator.rb | 2 +- 4 files changed, 20 insertions(+), 7 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/configurable.rb b/lib/gitlab/ci/config/entry/configurable.rb index e05aca9881b..68b6742385a 100644 --- a/lib/gitlab/ci/config/entry/configurable.rb +++ b/lib/gitlab/ci/config/entry/configurable.rb @@ -15,9 +15,10 @@ module Gitlab # module Configurable extend ActiveSupport::Concern - include Validatable included do + include Validatable + validations do validates :config, type: Hash end diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb index a6a914d79c1..2474684e07f 100644 --- a/lib/gitlab/ci/config/entry/node.rb +++ b/lib/gitlab/ci/config/entry/node.rb @@ -16,8 +16,9 @@ module Gitlab @metadata = metadata @entries = {} - @validator = self.class.validator.new(self) - @validator.validate(:new) + self.class.aspects.to_a.each do |aspect| + instance_exec(&aspect) + end end def [](key) @@ -47,7 +48,7 @@ module Gitlab end def errors - @validator.messages + descendants.flat_map(&:errors) + [] end def value @@ -79,8 +80,8 @@ module Gitlab def self.default end - def self.validator - Validator + def self.aspects + @aspects ||= [] end end end diff --git a/lib/gitlab/ci/config/entry/validatable.rb b/lib/gitlab/ci/config/entry/validatable.rb index f7f1b111571..5ced778d311 100644 --- a/lib/gitlab/ci/config/entry/validatable.rb +++ b/lib/gitlab/ci/config/entry/validatable.rb @@ -5,6 +5,17 @@ module Gitlab module Validatable extend ActiveSupport::Concern + def self.included(node) + node.aspects.append -> do + @validator = self.class.validator.new(self) + @validator.validate(:new) + end + end + + def errors + @validator.messages + descendants.flat_map(&:errors) + end + class_methods do def validator @validator ||= Class.new(Entry::Validator).tap do |validator| diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb index 55343005fe3..5ab54d7e218 100644 --- a/lib/gitlab/ci/config/entry/validator.rb +++ b/lib/gitlab/ci/config/entry/validator.rb @@ -30,7 +30,7 @@ module Gitlab def key_name if key.blank? - @entry.class.name.demodulize.underscore.humanize + @entry.class.name.to_s.demodulize.underscore.humanize else key end -- cgit v1.2.1 From 8c409fc40ba4bf2e7fe0c8458fd2b59c09bd123a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 09:49:18 +0200 Subject: Make it possible to define CI/CD config strategies --- lib/gitlab/ci/config/entry/policy.rb | 25 ++++++++++++++++++++---- lib/gitlab/ci/config/entry/simplifiable.rb | 31 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 lib/gitlab/ci/config/entry/simplifiable.rb (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index a08ab8a9d14..ac564a6c7d0 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -5,11 +5,28 @@ module Gitlab ## # Entry that represents a trigger policy for the job. # - class Policy < Node - include Validatable + class Policy < Simplifiable + strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } + strategy :ExpressionsPolicy, if: -> (config) { config.is_a?(Hash) } - validations do - validates :config, array_of_strings_or_regexps: true + class RefsPolicy < Entry::Node + include Entry::Validatable + + validations do + validates :config, array_of_strings_or_regexps: true + end + + def value + { refs: @config } + end + end + + class ExpressionsPolicy < Entry::Node + include Entry::Validatable + + validations do + validates :config, type: Hash + end end end end diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb new file mode 100644 index 00000000000..5319065b846 --- /dev/null +++ b/lib/gitlab/ci/config/entry/simplifiable.rb @@ -0,0 +1,31 @@ +module Gitlab + module Ci + class Config + module Entry + class Simplifiable < SimpleDelegator + EntryStrategy = Struct.new(:name, :condition) + + def initialize(config, **metadata) + strategy = self.class.strategies.find do |variant| + variant.condition.call(config) + end + + entry = self.class.const_get(strategy.name) + + super(entry.new(config, metadata)) + end + + def self.strategy(name, **opts) + EntryStrategy.new(name, opts.fetch(:if)).tap do |strategy| + (@strategies ||= []).append(strategy) + end + end + + def self.strategies + @strategies || [] + end + end + end + end + end +end -- cgit v1.2.1 From fcb4d1f80945cbcdfbdb73e102d046447f55e5b5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 10:27:00 +0200 Subject: Implement complex only/except policy CI/CD config --- lib/gitlab/ci/config/entry/policy.rb | 20 ++++++++++++++++++-- lib/gitlab/ci/config/entry/simplifiable.rb | 12 ++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index ac564a6c7d0..decacebee55 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -3,7 +3,7 @@ module Gitlab class Config module Entry ## - # Entry that represents a trigger policy for the job. + # Entry that represents an only/except trigger policy for the job. # class Policy < Simplifiable strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } @@ -23,9 +23,25 @@ module Gitlab class ExpressionsPolicy < Entry::Node include Entry::Validatable + include Entry::Attributable + + attributes :refs, :expressions validations do - validates :config, type: Hash + validates :config, presence: true + validates :config, allowed_keys: %i[refs expressions] + + with_options allow_nil: true do + validates :refs, array_of_strings_or_regexps: true + validates :expressions, type: Array + validates :expressions, presence: true + end + end + end + + class UnknownStrategy < Entry::Node + def errors + ['policy has to be either an array of conditions or a hash'] end end end diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb index 5319065b846..c26576fcbcd 100644 --- a/lib/gitlab/ci/config/entry/simplifiable.rb +++ b/lib/gitlab/ci/config/entry/simplifiable.rb @@ -10,7 +10,7 @@ module Gitlab variant.condition.call(config) end - entry = self.class.const_get(strategy.name) + entry = self.class.entry_class(strategy) super(entry.new(config, metadata)) end @@ -22,7 +22,15 @@ module Gitlab end def self.strategies - @strategies || [] + @strategies.to_a + end + + def self.entry_class(strategy) + if strategy.present? + self.const_get(strategy.name) + else + self::UnknownStrategy + end end end end -- cgit v1.2.1 From 9e203582b367a1b84035572261a79b62e22bfeaa Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Wed, 23 Aug 2017 01:51:53 +0900 Subject: Improve AutocompleteController#user.json performance --- lib/gitlab/sql/pattern.rb | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 lib/gitlab/sql/pattern.rb (limited to 'lib') diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb new file mode 100644 index 00000000000..47ea19994a2 --- /dev/null +++ b/lib/gitlab/sql/pattern.rb @@ -0,0 +1,29 @@ +module Gitlab + module SQL + class Pattern + MIN_CHARS_FOR_PARTIAL_MATCHING = 3 + + attr_reader :query + + def initialize(query) + @query = query + end + + def to_sql + if exact_matching? + query + else + "%#{query}%" + end + end + + def exact_matching? + !partial_matching? + end + + def partial_matching? + @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING + end + end + end +end -- cgit v1.2.1 From a061a2461a05200ab534d382b67e1b9099128478 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 11:42:40 +0200 Subject: Fix CI/CD trigger policy default value --- lib/gitlab/ci/config/entry/policy.rb | 3 +++ 1 file changed, 3 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index decacebee55..41a3737b34a 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -44,6 +44,9 @@ module Gitlab ['policy has to be either an array of conditions or a hash'] end end + + def self.default + end end end end -- cgit v1.2.1 From 946e8d3a93eb8c9e30d1f3baa3b5b28e6c06fbc1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 12:01:59 +0200 Subject: Use only/except policy that returns an array --- lib/gitlab/ci/config/entry/policy.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 41a3737b34a..495822e9f5a 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -15,10 +15,6 @@ module Gitlab validations do validates :config, array_of_strings_or_regexps: true end - - def value - { refs: @config } - end end class ExpressionsPolicy < Entry::Node -- cgit v1.2.1 From 99dddac55850fed68ea6f66363c3f083bd4866fa Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 13:00:45 +0200 Subject: Simplify ci config entry validator implementation --- lib/gitlab/ci/config/entry/validator.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb index 5ab54d7e218..83bca0d08bc 100644 --- a/lib/gitlab/ci/config/entry/validator.rb +++ b/lib/gitlab/ci/config/entry/validator.rb @@ -24,16 +24,11 @@ module Gitlab private def location - predecessors = ancestors.map(&:key).compact - predecessors.append(key_name).join(':') + ancestors.map(&:key).compact.append(key_name).join(':') end def key_name - if key.blank? - @entry.class.name.to_s.demodulize.underscore.humanize - else - key - end + key.presence || @entry.class.name.to_s.demodulize.underscore.humanize end end end -- cgit v1.2.1 From 7e6bc4dde29af635c4ec281beea20ca87ccfbe34 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 15:16:09 +0200 Subject: Improve reporting of a CI/CD entry config location --- lib/gitlab/ci/config/entry/attributable.rb | 2 ++ lib/gitlab/ci/config/entry/node.rb | 7 +++++++ lib/gitlab/ci/config/entry/policy.rb | 2 +- lib/gitlab/ci/config/entry/validator.rb | 11 ----------- 4 files changed, 10 insertions(+), 12 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/attributable.rb b/lib/gitlab/ci/config/entry/attributable.rb index 1c8b55ee4c4..24ff862a142 100644 --- a/lib/gitlab/ci/config/entry/attributable.rb +++ b/lib/gitlab/ci/config/entry/attributable.rb @@ -8,6 +8,8 @@ module Gitlab class_methods do def attributes(*attributes) attributes.flatten.each do |attribute| + raise ArgumentError if method_defined?(attribute) + define_method(attribute) do return unless config.is_a?(Hash) diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb index 2474684e07f..c868943c42e 100644 --- a/lib/gitlab/ci/config/entry/node.rb +++ b/lib/gitlab/ci/config/entry/node.rb @@ -71,6 +71,13 @@ module Gitlab true end + def location + name = @key.presence || self.class.name.to_s.demodulize + .underscore.humanize.downcase + + ancestors.map(&:key).append(name).compact.join(':') + end + def inspect val = leaf? ? config : descendants unspecified = specified? ? '' : '(unspecified) ' diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 495822e9f5a..05602f1b3c6 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -37,7 +37,7 @@ module Gitlab class UnknownStrategy < Entry::Node def errors - ['policy has to be either an array of conditions or a hash'] + ["#{location} has to be either an array of conditions or a hash"] end end diff --git a/lib/gitlab/ci/config/entry/validator.rb b/lib/gitlab/ci/config/entry/validator.rb index 83bca0d08bc..2df23a3edcd 100644 --- a/lib/gitlab/ci/config/entry/validator.rb +++ b/lib/gitlab/ci/config/entry/validator.rb @@ -8,7 +8,6 @@ module Gitlab def initialize(entry) super(entry) - @entry = entry end def messages @@ -20,16 +19,6 @@ module Gitlab def self.name 'Validator' end - - private - - def location - ancestors.map(&:key).compact.append(key_name).join(':') - end - - def key_name - key.presence || @entry.class.name.to_s.demodulize.underscore.humanize - end end end end -- cgit v1.2.1 From d1054bd3ddb48c15b6a3a53f8c57974208094106 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 25 Aug 2017 22:38:07 +0800 Subject: Resolve feedback on the MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13766 * Rename AfterImportService * Use constants * Move Git operations to Gitlab::Git::Repository * Use Regexp.union --- lib/gitlab/git/repository.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index eb3731ba35a..f6d30ad7534 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -232,6 +232,13 @@ module Gitlab branch_names + tag_names end + # Returns an Array of all ref names, except when it's matching pattern + # + # regexp - The pattern for ref names we don't want + def all_ref_names_except(regexp) + rugged.references.reject { |ref| ref.name =~ regexp }.map(&:name) + end + # Discovers the default branch based on the repository's available branches # # - If no branches are present, returns nil @@ -577,6 +584,10 @@ module Gitlab rugged.branches.delete(branch_name) end + def delete_refs(ref_names) + ref_names.each(&rugged.references.method(:delete)) + end + # Create a new branch named **ref+ based on **stat_point+, HEAD by default # # Examples: -- cgit v1.2.1 From 04108611716c0f1bb00092077ad5e31785675490 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 25 Aug 2017 18:26:55 +0200 Subject: Add specs for attributable aspect of ci config entry --- lib/gitlab/ci/config/entry/attributable.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/attributable.rb b/lib/gitlab/ci/config/entry/attributable.rb index 24ff862a142..3e87a09704e 100644 --- a/lib/gitlab/ci/config/entry/attributable.rb +++ b/lib/gitlab/ci/config/entry/attributable.rb @@ -8,7 +8,9 @@ module Gitlab class_methods do def attributes(*attributes) attributes.flatten.each do |attribute| - raise ArgumentError if method_defined?(attribute) + if method_defined?(attribute) + raise ArgumentError, 'Method already defined!' + end define_method(attribute) do return unless config.is_a?(Hash) -- cgit v1.2.1 From 866aab7f2a92f9929a5c5811d3d3c23c11184b26 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Sat, 26 Aug 2017 22:32:55 +0900 Subject: Fix escape characters was not sanitized --- lib/gitlab/sql/pattern.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 47ea19994a2..46c973d8a11 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -11,9 +11,9 @@ module Gitlab def to_sql if exact_matching? - query + sanitized_query else - "%#{query}%" + "%#{sanitized_query}%" end end @@ -24,6 +24,11 @@ module Gitlab def partial_matching? @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end + + def sanitized_query + # Note: ActiveRecord::Base.sanitize_sql_like is a protected method + ActiveRecord::Base.__send__(:sanitize_sql_like, query) + end end end end -- cgit v1.2.1 From 87b51c5981db3b1b9831b01ca6e74127d57dc2d9 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 29 Aug 2017 07:14:41 +0900 Subject: Move the logic to a concern --- lib/gitlab/sql/pattern.rb | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 46c973d8a11..26bfeeeee67 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -1,33 +1,26 @@ module Gitlab module SQL - class Pattern - MIN_CHARS_FOR_PARTIAL_MATCHING = 3 - - attr_reader :query + module Pattern + extend ActiveSupport::Concern - def initialize(query) - @query = query - end + MIN_CHARS_FOR_PARTIAL_MATCHING = 3 - def to_sql - if exact_matching? - sanitized_query - else - "%#{sanitized_query}%" + class_methods do + def to_pattern(query) + if exact_matching?(query) + sanitize_sql_like(query) + else + "%#{sanitize_sql_like(query)}%" + end end - end - def exact_matching? - !partial_matching? - end - - def partial_matching? - @query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING - end + def exact_matching?(query) + query.length < MIN_CHARS_FOR_PARTIAL_MATCHING + end - def sanitized_query - # Note: ActiveRecord::Base.sanitize_sql_like is a protected method - ActiveRecord::Base.__send__(:sanitize_sql_like, query) + def partial_matching?(query) + query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING + end end end end -- cgit v1.2.1 From 9954dafda58a0d5d856fdbbef01633e674638aa1 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 29 Aug 2017 16:23:12 +0800 Subject: Just use a block --- lib/gitlab/git/repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f6d30ad7534..adf15473eb6 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -585,7 +585,7 @@ module Gitlab end def delete_refs(ref_names) - ref_names.each(&rugged.references.method(:delete)) + ref_names.each { |ref| rugged.references.delete(ref) } end # Create a new branch named **ref+ based on **stat_point+, HEAD by default -- cgit v1.2.1 From 12633b46b6884dda4ffd87b14b4b52725acd6ec1 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Tue, 29 Aug 2017 18:00:03 +0900 Subject: Refactor --- lib/gitlab/sql/pattern.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb index 26bfeeeee67..b42bc67ccfc 100644 --- a/lib/gitlab/sql/pattern.rb +++ b/lib/gitlab/sql/pattern.rb @@ -7,17 +7,13 @@ module Gitlab class_methods do def to_pattern(query) - if exact_matching?(query) - sanitize_sql_like(query) - else + if partial_matching?(query) "%#{sanitize_sql_like(query)}%" + else + sanitize_sql_like(query) end end - def exact_matching?(query) - query.length < MIN_CHARS_FOR_PARTIAL_MATCHING - end - def partial_matching?(query) query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING end -- cgit v1.2.1 From 5eab624d3ce7500b3cc19230b5ce86125b88015b Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 18 Aug 2017 14:26:23 +0200 Subject: Improve migrations using triggers This adds a bunch of checks to migrations that may create or drop triggers. Dropping triggers/functions is done using "IF EXISTS" so we don't throw an error if the object in question has already been dropped. We now also raise a custom error (message) when the user does not have TRIGGER privileges. This should prevent the schema from entering an inconsistent state while also providing the user with enough information on how to solve the problem. The recommendation of using SUPERUSER permissions is a bit extreme but we require this anyway (Omnibus also configures users with this permission). Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36633 --- lib/gitlab/database.rb | 8 +++++++ lib/gitlab/database/grant.rb | 34 ++++++++++++++++++++++++++++++ lib/gitlab/database/migration_helpers.rb | 36 ++++++++++++++++++++++++++++---- 3 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 lib/gitlab/database/grant.rb (limited to 'lib') diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index e001d25e7b7..a6ec75da385 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -9,6 +9,14 @@ module Gitlab ActiveRecord::Base.configurations[Rails.env] end + def self.username + config['username'] || ENV['USER'] + end + + def self.database_name + config['database'] + end + def self.adapter_name config['adapter'] end diff --git a/lib/gitlab/database/grant.rb b/lib/gitlab/database/grant.rb new file mode 100644 index 00000000000..aee3981e79a --- /dev/null +++ b/lib/gitlab/database/grant.rb @@ -0,0 +1,34 @@ +module Gitlab + module Database + # Model that can be used for querying permissions of a SQL user. + class Grant < ActiveRecord::Base + self.table_name = + if Database.postgresql? + 'information_schema.role_table_grants' + else + 'mysql.user' + end + + def self.scope_to_current_user + if Database.postgresql? + where('grantee = user') + else + where("CONCAT(User, '@', Host) = current_user()") + end + end + + # Returns true if the current user can create and execute triggers on the + # given table. + def self.create_and_execute_trigger?(table) + priv = + if Database.postgresql? + where(privilege_type: 'TRIGGER', table_name: table) + else + where(Trigger_priv: 'Y') + end + + priv.scope_to_current_user.any? + end + end + end +end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 5e2c6cc5cad..fb14798efe6 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -358,6 +358,8 @@ module Gitlab raise 'rename_column_concurrently can not be run inside a transaction' end + check_trigger_permissions!(table) + old_col = column_for(table, old) new_type = type || old_col.type @@ -430,6 +432,8 @@ module Gitlab def cleanup_concurrent_column_rename(table, old, new) trigger_name = rename_trigger_name(table, old, new) + check_trigger_permissions!(table) + if Database.postgresql? remove_rename_triggers_for_postgresql(table, trigger_name) else @@ -485,14 +489,14 @@ module Gitlab # Removes the triggers used for renaming a PostgreSQL column concurrently. def remove_rename_triggers_for_postgresql(table, trigger) - execute("DROP TRIGGER #{trigger} ON #{table}") - execute("DROP FUNCTION #{trigger}()") + execute("DROP TRIGGER IF EXISTS #{trigger} ON #{table}") + execute("DROP FUNCTION IF EXISTS #{trigger}()") end # Removes the triggers used for renaming a MySQL column concurrently. def remove_rename_triggers_for_mysql(trigger) - execute("DROP TRIGGER #{trigger}_insert") - execute("DROP TRIGGER #{trigger}_update") + execute("DROP TRIGGER IF EXISTS #{trigger}_insert") + execute("DROP TRIGGER IF EXISTS #{trigger}_update") end # Returns the (base) name to use for triggers when renaming columns. @@ -625,6 +629,30 @@ module Gitlab conn.llen("queue:#{queue_name}") end end + + def check_trigger_permissions!(table) + unless Grant.create_and_execute_trigger?(table) + dbname = Database.database_name + user = Database.username + + raise <<-EOF +Your database user is not allowed to create, drop, or execute triggers on the +table #{table}. + +If you are using PostgreSQL you can solve this by logging in to the GitLab +database (#{dbname}) using a super user and running: + + ALTER #{user} WITH SUPERUSER + +For MySQL you instead need to run: + + GRANT ALL PRIVILEGES ON *.* TO #{user}@'%' + +Both queries will grant the user super user permissions, ensuring you don't run +into similar problems in the future (e.g. when new tables are created). + EOF + end + end end end end -- cgit v1.2.1 From ce1ce82045f168143ccc143f5200ea9da820d990 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Mon, 21 Aug 2017 17:42:03 -0500 Subject: Resolve new N+1 by adding preloads and metadata to issues end points --- lib/api/entities.rb | 22 ++++++++++++++++++++-- lib/api/issues.rb | 22 +++++++++++++++++++--- 2 files changed, 39 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e8dd61e493f..e18c69b7a3d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -320,7 +320,10 @@ module API end class IssueBasic < ProjectEntity - expose :label_names, as: :labels + expose :labels do |issue, options| + # Avoids an N+1 query since labels are preloaded + issue.labels.map(&:title).sort + end expose :milestone, using: Entities::Milestone expose :assignees, :author, using: Entities::UserBasic @@ -329,7 +332,22 @@ module API end expose :user_notes_count - expose :upvotes, :downvotes + expose :upvotes do |issue, options| + if options[:issuable_metadata] + # Avoids an N+1 query when metadata is included + options[:issuable_metadata][issue.id].upvotes + else + issue.upvotes + end + end + expose :downvotes do |issue, options| + if options[:issuable_metadata] + # Avoids an N+1 query when metadata is included + options[:issuable_metadata][issue.id].downvotes + else + issue.downvotes + end + end expose :due_date expose :confidential diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 4cec1145f3a..64a425eb96e 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -4,6 +4,8 @@ module API before { authenticate! } + helpers ::Gitlab::IssuableMetadata + helpers do def find_issues(args = {}) args = params.merge(args) @@ -13,6 +15,7 @@ module API args[:label_name] = args.delete(:labels) issues = IssuesFinder.new(current_user, args).execute + .preload(:assignees, :labels, :notes) issues.reorder(args[:order_by] => args[:sort]) end @@ -65,7 +68,11 @@ module API get do issues = find_issues - present paginate(issues), with: Entities::IssueBasic, current_user: current_user + options = { with: Entities::IssueBasic, + current_user: current_user } + options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + + present paginate(issues), options end end @@ -86,7 +93,11 @@ module API issues = find_issues(group_id: group.id) - present paginate(issues), with: Entities::IssueBasic, current_user: current_user + options = { with: Entities::IssueBasic, + current_user: current_user } + options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + + present paginate(issues), options end end @@ -109,7 +120,12 @@ module API issues = find_issues(project_id: project.id) - present paginate(issues), with: Entities::IssueBasic, current_user: current_user, project: user_project + options = { with: Entities::IssueBasic, + current_user: current_user, + project: user_project } + options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + + present paginate(issues), options end desc 'Get a single project issue' do -- cgit v1.2.1 From 749c389345cf382b740277db62f7d4b849902d60 Mon Sep 17 00:00:00 2001 From: Travis Miller Date: Mon, 28 Aug 2017 19:02:26 -0500 Subject: Add time stats to issue and merge request API end points --- lib/api/entities.rb | 22 +++++++++++++++++++++- lib/api/issues.rb | 28 +++++++++++++++++----------- lib/api/merge_requests.rb | 2 +- 3 files changed, 39 insertions(+), 13 deletions(-) (limited to 'lib') diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e18c69b7a3d..803b48dd88a 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -354,6 +354,10 @@ module API expose :web_url do |issue, options| Gitlab::UrlBuilder.build(issue) end + + expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |issue| + issue + end end class Issue < IssueBasic @@ -383,10 +387,22 @@ module API end class IssuableTimeStats < Grape::Entity + format_with(:time_tracking_formatter) do |time_spent| + Gitlab::TimeTrackingFormatter.output(time_spent) + end + expose :time_estimate expose :total_time_spent expose :human_time_estimate - expose :human_total_time_spent + + with_options(format_with: :time_tracking_formatter) do + expose :total_time_spent, as: :human_total_time_spent + end + + def total_time_spent + # Avoids an N+1 query since timelogs are preloaded + object.timelogs.map(&:time_spent).sum + end end class ExternalIssue < Grape::Entity @@ -436,6 +452,10 @@ module API expose :web_url do |merge_request, options| Gitlab::UrlBuilder.build(merge_request) end + + expose :time_stats, using: 'API::Entities::IssuableTimeStats' do |merge_request| + merge_request + end end class MergeRequest < MergeRequestBasic diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 64a425eb96e..c30e430ae85 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -15,7 +15,7 @@ module API args[:label_name] = args.delete(:labels) issues = IssuesFinder.new(current_user, args).execute - .preload(:assignees, :labels, :notes) + .preload(:assignees, :labels, :notes, :timelogs) issues.reorder(args[:order_by] => args[:sort]) end @@ -68,9 +68,11 @@ module API get do issues = find_issues - options = { with: Entities::IssueBasic, - current_user: current_user } - options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + options = { + with: Entities::IssueBasic, + current_user: current_user, + issuable_metadata: issuable_meta_data(issues, 'Issue') + } present paginate(issues), options end @@ -93,9 +95,11 @@ module API issues = find_issues(group_id: group.id) - options = { with: Entities::IssueBasic, - current_user: current_user } - options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + options = { + with: Entities::IssueBasic, + current_user: current_user, + issuable_metadata: issuable_meta_data(issues, 'Issue') + } present paginate(issues), options end @@ -120,10 +124,12 @@ module API issues = find_issues(project_id: project.id) - options = { with: Entities::IssueBasic, - current_user: current_user, - project: user_project } - options[:issuable_metadata] = issuable_meta_data(issues, 'Issue') + options = { + with: Entities::IssueBasic, + current_user: current_user, + project: user_project, + issuable_metadata: issuable_meta_data(issues, 'Issue') + } present paginate(issues), options end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 8810d4e441d..a3284afb745 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -21,7 +21,7 @@ module API return merge_requests if args[:view] == 'simple' merge_requests - .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels) + .preload(:notes, :author, :assignee, :milestone, :merge_request_diff, :labels, :timelogs) end params :merge_requests_params do -- cgit v1.2.1 From 67c042e4a5603a39494c3c7e407161348d7e85f3 Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Tue, 29 Aug 2017 16:49:43 +0200 Subject: Respect the default visibility level when creating a group --- lib/api/groups.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/api/groups.rb b/lib/api/groups.rb index e56427304a6..15266e9d4e5 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -7,7 +7,9 @@ module API helpers do params :optional_params_ce do optional :description, type: String, desc: 'The description of the group' - optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the group' + optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, + default: Gitlab::VisibilityLevel.string_level(Gitlab::CurrentSettings.current_application_settings.default_group_visibility), + desc: 'The visibility of the group' optional :lfs_enabled, type: Boolean, desc: 'Enable/disable LFS for the projects in this group' optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' optional :share_with_group_lock, type: Boolean, desc: 'Prevent sharing a project with another group within this group' -- cgit v1.2.1 From 9edaff0d3547ba9854e69655a4ae4d5095b8a25f Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Wed, 30 Aug 2017 10:11:24 +0200 Subject: Make rubocop happy --- lib/api/groups.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 15266e9d4e5..cf3cee0073c 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -7,9 +7,11 @@ module API helpers do params :optional_params_ce do optional :description, type: String, desc: 'The description of the group' - optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, - default: Gitlab::VisibilityLevel.string_level(Gitlab::CurrentSettings.current_application_settings.default_group_visibility), - desc: 'The visibility of the group' + optional :visibility, type: String, + values: Gitlab::VisibilityLevel.string_values, + default: Gitlab::VisibilityLevel.string_level( + Gitlab::CurrentSettings.current_application_settings.default_group_visibility), + desc: 'The visibility of the group' optional :lfs_enabled, type: Boolean, desc: 'Enable/disable LFS for the projects in this group' optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' optional :share_with_group_lock, type: Boolean, desc: 'Prevent sharing a project with another group within this group' -- cgit v1.2.1 From 808fb2549bf8b3dee58a4af81a4b6513a5016342 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 30 Aug 2017 13:20:55 +0200 Subject: Raise exception when simplifiable ci entry incomplete --- lib/gitlab/ci/config/entry/simplifiable.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb index c26576fcbcd..0a03027736f 100644 --- a/lib/gitlab/ci/config/entry/simplifiable.rb +++ b/lib/gitlab/ci/config/entry/simplifiable.rb @@ -6,6 +6,10 @@ module Gitlab EntryStrategy = Struct.new(:name, :condition) def initialize(config, **metadata) + unless self.class.const_defined?(:UnknownStrategy) + raise ArgumentError, 'UndefinedStrategy not available!' + end + strategy = self.class.strategies.find do |variant| variant.condition.call(config) end -- cgit v1.2.1 From 3474e109b73e0373a2663b5ae06db9aab61e1c02 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 30 Aug 2017 13:27:48 +0200 Subject: Simplify code for appending strategies in CI/CD config --- lib/gitlab/ci/config/entry/simplifiable.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/simplifiable.rb b/lib/gitlab/ci/config/entry/simplifiable.rb index 0a03027736f..12764629686 100644 --- a/lib/gitlab/ci/config/entry/simplifiable.rb +++ b/lib/gitlab/ci/config/entry/simplifiable.rb @@ -21,12 +21,12 @@ module Gitlab def self.strategy(name, **opts) EntryStrategy.new(name, opts.fetch(:if)).tap do |strategy| - (@strategies ||= []).append(strategy) + strategies.append(strategy) end end def self.strategies - @strategies.to_a + @strategies ||= [] end def self.entry_class(strategy) -- cgit v1.2.1 From 1bf87d25241eb5e1a0f147d350814a3a1cfa1343 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 30 Aug 2017 14:47:54 +0200 Subject: Remove unused expressions policy from ci/cd config --- lib/gitlab/ci/config/entry/policy.rb | 19 ------------------- 1 file changed, 19 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 05602f1b3c6..3cdae1cee4f 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -7,7 +7,6 @@ module Gitlab # class Policy < Simplifiable strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } - strategy :ExpressionsPolicy, if: -> (config) { config.is_a?(Hash) } class RefsPolicy < Entry::Node include Entry::Validatable @@ -17,24 +16,6 @@ module Gitlab end end - class ExpressionsPolicy < Entry::Node - include Entry::Validatable - include Entry::Attributable - - attributes :refs, :expressions - - validations do - validates :config, presence: true - validates :config, allowed_keys: %i[refs expressions] - - with_options allow_nil: true do - validates :refs, array_of_strings_or_regexps: true - validates :expressions, type: Array - validates :expressions, presence: true - end - end - end - class UnknownStrategy < Entry::Node def errors ["#{location} has to be either an array of conditions or a hash"] -- cgit v1.2.1 From b9d8946395ebe2b0d05e464e6a2af1181c7f1b90 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 30 Aug 2017 13:33:39 +0100 Subject: Don't use public_send in destroy_conditionally! helper As we only override in two places, we could just ask for the value rather than the method name. --- lib/api/branches.rb | 2 +- lib/api/helpers.rb | 6 ++++-- lib/api/tags.rb | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/api/branches.rb b/lib/api/branches.rb index b87f7cdbad1..a989394ad91 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -130,7 +130,7 @@ module API commit = user_project.repository.commit(branch.dereferenced_target) - destroy_conditionally!(commit, last_update_field: :authored_date) do + destroy_conditionally!(commit, last_updated: commit.authored_date) do result = DeleteBranchService.new(user_project, current_user) .execute(params[:branch]) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 84980864151..3d377fdb9eb 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -19,8 +19,10 @@ module API end end - def destroy_conditionally!(resource, last_update_field: :updated_at) - check_unmodified_since!(resource.public_send(last_update_field)) + def destroy_conditionally!(resource, last_updated: nil) + last_updated ||= resource.updated_at + + check_unmodified_since!(last_updated) status 204 if block_given? diff --git a/lib/api/tags.rb b/lib/api/tags.rb index 81b17935b81..912415e3a7f 100644 --- a/lib/api/tags.rb +++ b/lib/api/tags.rb @@ -70,7 +70,7 @@ module API commit = user_project.repository.commit(tag.dereferenced_target) - destroy_conditionally!(commit, last_update_field: :authored_date) do + destroy_conditionally!(commit, last_updated: commit.authored_date) do result = ::Tags::DestroyService.new(user_project, current_user) .execute(params[:tag_name]) -- cgit v1.2.1 From c5553ce772371295d2d7652cec899633042fae07 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 30 Aug 2017 21:15:06 +0800 Subject: Use `git update-ref --stdin -z` to delete refs --- lib/gitlab/git/repository.rb | 18 ++++++++++++++++-- lib/tasks/gitlab/cleanup.rake | 2 +- 2 files changed, 17 insertions(+), 3 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 8b39e92cc65..fb6504bdea0 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -17,6 +17,7 @@ module Gitlab NoRepository = Class.new(StandardError) InvalidBlobName = Class.new(StandardError) InvalidRef = Class.new(StandardError) + GitError = Class.new(StandardError) class << self # Unlike `new`, `create` takes the storage path, not the storage name @@ -598,8 +599,21 @@ module Gitlab rugged.branches.delete(branch_name) end - def delete_refs(ref_names) - ref_names.each { |ref| rugged.references.delete(ref) } + def delete_refs(*ref_names) + instructions = ref_names.map do |ref| + "delete #{ref}\x00\x00" + end + + command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z] + message, status = Gitlab::Popen.popen( + command, + path) do |stdin| + stdin.write(instructions.join) + end + + unless status.zero? + raise GitError.new("Could not delete refs #{ref_names}: #{message}") + end end # Create a new branch named **ref+ based on **stat_point+, HEAD by default diff --git a/lib/tasks/gitlab/cleanup.rake b/lib/tasks/gitlab/cleanup.rake index f76bef5f4bf..8ae1b6a626a 100644 --- a/lib/tasks/gitlab/cleanup.rake +++ b/lib/tasks/gitlab/cleanup.rake @@ -111,7 +111,7 @@ namespace :gitlab do next unless id > max_iid project.deployments.find(id).create_ref - rugged.references.delete(ref) + project.repository.delete_refs(ref) end end end -- cgit v1.2.1