diff options
| -rw-r--r-- | .gitlab/CODEOWNERS | 15 | ||||
| -rw-r--r-- | app/models/concerns/case_sensitivity.rb | 45 | ||||
| -rw-r--r-- | app/models/user.rb | 5 | ||||
| -rw-r--r-- | app/views/projects/blob/_blob.html.haml | 1 | ||||
| -rw-r--r-- | lib/gitlab/user_extractor.rb | 53 | ||||
| -rw-r--r-- | spec/lib/gitlab/user_extractor_spec.rb | 58 | ||||
| -rw-r--r-- | spec/models/concerns/case_sensitivity_spec.rb | 202 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 17 | ||||
| -rw-r--r-- | spec/support/helpers/test_env.rb | 3 |
9 files changed, 216 insertions, 183 deletions
diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS new file mode 100644 index 00000000000..5b6e5a719fa --- /dev/null +++ b/.gitlab/CODEOWNERS @@ -0,0 +1,15 @@ +# Backend Maintainers are the default for all ruby files +*.rb @ayufan @DouweM @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern +*.rake @ayufan @DouweM @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern + +# Technical writing team are the default reviewers for everything in `doc/` +/doc/ @axil @marcia + +# Frontend maintainers should see everything in `app/assets/` +app/assets/ @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann + +# Someone from the database team should review changes in `db/` +db/ @abrandl @NikolayS + +# Feature specific owners +/ee/lib/gitlab/code_owners/ @reprazent diff --git a/app/models/concerns/case_sensitivity.rb b/app/models/concerns/case_sensitivity.rb index 6e80365ee5b..c93b6589ee7 100644 --- a/app/models/concerns/case_sensitivity.rb +++ b/app/models/concerns/case_sensitivity.rb @@ -9,23 +9,46 @@ module CaseSensitivity # # Unlike other ActiveRecord methods this method only operates on a Hash. def iwhere(params) - criteria = self - cast_lower = Gitlab::Database.postgresql? + criteria = self params.each do |key, value| - column = ActiveRecord::Base.connection.quote_table_name(key) + criteria = case value + when Array + criteria.where(value_in(key, value)) + else + criteria.where(value_equal(key, value)) + end + end + + criteria + end - condition = - if cast_lower - "LOWER(#{column}) = LOWER(:value)" - else - "#{column} = :value" - end + private + + def value_equal(column, value) + lower_value = lower_value(value) + + lower_column(arel_table[column]).eq(lower_value).to_sql + end - criteria = criteria.where(condition, value: value) + def value_in(column, values) + lower_values = values.map do |value| + lower_value(value) end - criteria + lower_column(arel_table[column]).in(lower_values).to_sql + end + + def lower_value(value) + return value if Gitlab::Database.mysql? + + Arel::Nodes::NamedFunction.new('LOWER', [Arel::Nodes.build_quoted(value)]) + end + + def lower_column(column) + return column if Gitlab::Database.mysql? + + column.lower end end end diff --git a/app/models/user.rb b/app/models/user.rb index f21ca1c569f..0fcc952b5cd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -257,6 +257,7 @@ class User < ActiveRecord::Base scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) } scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :by_username, -> (usernames) { iwhere(username: usernames) } # Limits the users to those that have TODOs, optionally in the given state. # @@ -444,11 +445,11 @@ class User < ActiveRecord::Base end def find_by_username(username) - iwhere(username: username).take + by_username(username).take end def find_by_username!(username) - iwhere(username: username).take! + by_username(username).take! end def find_by_personal_access_token(token_string) diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index a4b1b496b69..cf273aab108 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -5,6 +5,7 @@ %ul.blob-commit-info = render 'projects/commits/commit', commit: @last_commit, project: @project, ref: @ref + = render_if_exists 'projects/blob/owners', blob: blob = render "projects/blob/auxiliary_viewer", blob: blob #blob-content-holder.blob-content-holder diff --git a/lib/gitlab/user_extractor.rb b/lib/gitlab/user_extractor.rb new file mode 100644 index 00000000000..3ede0a5b5e6 --- /dev/null +++ b/lib/gitlab/user_extractor.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# This class extracts all users found in a piece of text by the username or the +# email adress + +module Gitlab + class UserExtractor + # Not using `Devise.email_regexp` to filter out any chars that an email + # does not end with and not pinning the email to a start of end of a string. + EMAIL_REGEXP = /(?<email>([^@\s]+@[^@\s]+(?<!\W)))/ + USERNAME_REGEXP = User.reference_pattern + + def initialize(text) + @text = text + end + + def users + return User.none unless @text.present? + + @users ||= User.from("(#{union.to_sql}) users") + end + + def usernames + matches[:usernames] + end + + def emails + matches[:emails] + end + + def references + @references ||= matches.values.flatten + end + + def matches + @matches ||= { + emails: @text.scan(EMAIL_REGEXP).flatten.uniq, + usernames: @text.scan(USERNAME_REGEXP).flatten.uniq + } + end + + private + + def union + relations = [] + + relations << User.by_any_email(emails) if emails.any? + relations << User.by_username(usernames) if usernames.any? + + Gitlab::SQL::Union.new(relations) + end + end +end diff --git a/spec/lib/gitlab/user_extractor_spec.rb b/spec/lib/gitlab/user_extractor_spec.rb new file mode 100644 index 00000000000..fcc05ab3a0c --- /dev/null +++ b/spec/lib/gitlab/user_extractor_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::UserExtractor do + let(:text) do + <<~TXT + This is a long texth that mentions some users. + @user-1, @user-2 and user@gitlab.org take a walk in the park. + There they meet @user-4 that was out with other-user@gitlab.org. + @user-1 thought it was late, so went home straight away + TXT + end + subject(:extractor) { described_class.new(text) } + + describe '#users' do + it 'returns an empty relation when nil was passed' do + extractor = described_class.new(nil) + + expect(extractor.users).to be_empty + expect(extractor.users).to be_a(ActiveRecord::Relation) + end + + it 'returns the user case insensitive for usernames' do + user = create(:user, username: "USER-4") + + expect(extractor.users).to include(user) + end + + it 'returns users by primary email' do + user = create(:user, email: 'user@gitlab.org') + + expect(extractor.users).to include(user) + end + + it 'returns users by secondary email' do + user = create(:email, email: 'other-user@gitlab.org').user + + expect(extractor.users).to include(user) + end + end + + describe '#matches' do + it 'includes all mentioned email adresses' do + expect(extractor.matches[:emails]).to contain_exactly('user@gitlab.org', 'other-user@gitlab.org') + end + + it 'includes all mentioned usernames' do + expect(extractor.matches[:usernames]).to contain_exactly('user-1', 'user-2', 'user-4') + end + end + + describe '#references' do + it 'includes all user-references once' do + expect(extractor.references).to contain_exactly('user-1', 'user-2', 'user@gitlab.org', 'user-4', 'other-user@gitlab.org') + end + end +end diff --git a/spec/models/concerns/case_sensitivity_spec.rb b/spec/models/concerns/case_sensitivity_spec.rb index 5c0dfaeb4d3..1bf6c9b3404 100644 --- a/spec/models/concerns/case_sensitivity_spec.rb +++ b/spec/models/concerns/case_sensitivity_spec.rb @@ -3,186 +3,50 @@ require 'spec_helper' describe CaseSensitivity do describe '.iwhere' do let(:connection) { ActiveRecord::Base.connection } - let(:model) { Class.new { include CaseSensitivity } } - - describe 'using PostgreSQL' do - before do - allow(Gitlab::Database).to receive(:postgresql?).and_return(true) - allow(Gitlab::Database).to receive(:mysql?).and_return(false) - end - - describe 'with a single column/value pair' do - it 'returns the criteria for a column and a value' do - criteria = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('"foo"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo") = LOWER(:value)}, value: 'bar') - .and_return(criteria) - - expect(model.iwhere(foo: 'bar')).to eq(criteria) - end - - it 'returns the criteria for a column with a table, and a value' do - criteria = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('"foo"."bar"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo"."bar") = LOWER(:value)}, value: 'bar') - .and_return(criteria) - - expect(model.iwhere('foo.bar'.to_sym => 'bar')).to eq(criteria) - end - end - - describe 'with multiple column/value pairs' do - it 'returns the criteria for a column and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('"foo"') - - expect(connection).to receive(:quote_table_name) - .with(:bar) - .and_return('"bar"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo") = LOWER(:value)}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{LOWER("bar") = LOWER(:value)}, value: 'baz') - .and_return(final) - - got = model.iwhere(foo: 'bar', bar: 'baz') - - expect(got).to eq(final) - end - - it 'returns the criteria for a column with a table, and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('"foo"."bar"') - - expect(connection).to receive(:quote_table_name) - .with(:'foo.baz') - .and_return('"foo"."baz"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo"."bar") = LOWER(:value)}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{LOWER("foo"."baz") = LOWER(:value)}, value: 'baz') - .and_return(final) - - got = model.iwhere('foo.bar'.to_sym => 'bar', - 'foo.baz'.to_sym => 'baz') - - expect(got).to eq(final) - end + let(:model) do + Class.new(ActiveRecord::Base) do + include CaseSensitivity + self.table_name = 'namespaces' end end - describe 'using MySQL' do - before do - allow(Gitlab::Database).to receive(:postgresql?).and_return(false) - allow(Gitlab::Database).to receive(:mysql?).and_return(true) - end - - describe 'with a single column/value pair' do - it 'returns the criteria for a column and a value' do - criteria = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('`foo`') - - expect(model).to receive(:where) - .with(%q{`foo` = :value}, value: 'bar') - .and_return(criteria) + let!(:model_1) { model.create(path: 'mOdEl-1', name: 'mOdEl 1') } + let!(:model_2) { model.create(path: 'mOdEl-2', name: 'mOdEl 2') } - expect(model.iwhere(foo: 'bar')).to eq(criteria) - end + it 'finds a single instance by a single attribute regardless of case' do + expect(model.iwhere(path: 'MODEL-1')).to contain_exactly(model_1) + end - it 'returns the criteria for a column with a table, and a value' do - criteria = double(:criteria) + it 'finds multiple instances by a single attribute regardless of case' do + expect(model.iwhere(path: %w(MODEL-1 model-2))).to contain_exactly(model_1, model_2) + end - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('`foo`.`bar`') + it 'finds instances by multiple attributes' do + expect(model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1')) + .to contain_exactly(model_1) + end - expect(model).to receive(:where) - .with(%q{`foo`.`bar` = :value}, value: 'bar') - .and_return(criteria) + # Using `mysql` & `postgresql` metadata-tags here because both adapters build + # the query slightly differently + context 'for MySQL', :mysql do + it 'builds a simple query' do + query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql + expected_query = <<~QRY.strip + SELECT `namespaces`.* FROM `namespaces` WHERE (`namespaces`.`path` IN ('MODEL-1', 'model-2')) AND (`namespaces`.`name` = 'model 1') + QRY - expect(model.iwhere('foo.bar'.to_sym => 'bar')) - .to eq(criteria) - end + expect(query).to eq(expected_query) end + end - describe 'with multiple column/value pairs' do - it 'returns the criteria for a column and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('`foo`') - - expect(connection).to receive(:quote_table_name) - .with(:bar) - .and_return('`bar`') - - expect(model).to receive(:where) - .with(%q{`foo` = :value}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{`bar` = :value}, value: 'baz') - .and_return(final) - - got = model.iwhere(foo: 'bar', bar: 'baz') - - expect(got).to eq(final) - end - - it 'returns the criteria for a column with a table, and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('`foo`.`bar`') - - expect(connection).to receive(:quote_table_name) - .with(:'foo.baz') - .and_return('`foo`.`baz`') - - expect(model).to receive(:where) - .with(%q{`foo`.`bar` = :value}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{`foo`.`baz` = :value}, value: 'baz') - .and_return(final) - - got = model.iwhere('foo.bar'.to_sym => 'bar', - 'foo.baz'.to_sym => 'baz') + context 'for PostgreSQL', :postgresql do + it 'builds a query using LOWER' do + query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql + expected_query = <<~QRY.strip + SELECT \"namespaces\".* FROM \"namespaces\" WHERE (LOWER(\"namespaces\".\"path\") IN (LOWER('MODEL-1'), LOWER('model-2'))) AND (LOWER(\"namespaces\".\"name\") = LOWER('model 1')) + QRY - expect(got).to eq(final) - end + expect(query).to eq(expected_query) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fd99acb3bb2..2a7aff39240 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -405,6 +405,23 @@ describe User do end end end + + describe '.by_username' do + it 'finds users regardless of the case passed' do + user = create(:user, username: 'CaMeLcAsEd') + user2 = create(:user, username: 'UPPERCASE') + + expect(described_class.by_username(%w(CAMELCASED uppercase))) + .to contain_exactly(user, user2) + end + + it 'finds a single user regardless of the case passed' do + user = create(:user, username: 'CaMeLcAsEd') + + expect(described_class.by_username('CAMELCASED')) + .to contain_exactly(user) + end + end end describe "Respond to" do diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index 21103771d1f..3f8e3ae5190 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -52,7 +52,8 @@ module TestEnv 'add_images_and_changes' => '010d106', 'update-gitlab-shell-v-6-0-1' => '2f61d70', 'update-gitlab-shell-v-6-0-3' => 'de78448', - '2-mb-file' => 'bf12d25' + '2-mb-file' => 'bf12d25', + 'with-codeowners' => '219560e' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily |