From 397efed8e82da68e2138e48ac68836622198228e Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Tue, 21 May 2019 21:59:18 +1200 Subject: Update docs on how to request approval for licenses/IP Squashed commit of the following: commit c07a6a46aba640fe0980b603f6bcaac89dc6153b Author: Jamie Hurewitz Date: Sun May 5 01:02:44 2019 +0000 Update licensing.md --- doc/development/licensing.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/development/licensing.md b/doc/development/licensing.md index 0e71cd47481..0550088ed5a 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -88,9 +88,11 @@ Definitions GitLab means GitLab Inc. and its affiliates and subsidiaries. -## Requesting Approval for Licenses +## Requesting Approval for Licenses or any other Intellectual Property -Libraries that are not listed in the [Acceptable Licenses][Acceptable-Licenses] or [Unacceptable Licenses][Unacceptable-Licenses] list can be submitted to the legal team for review. Please email `legal@gitlab.com` with the details. After a decision has been made, the original requestor is responsible for updating this document. +Libraries that are not already approved on the [Acceptable Licenses][Acceptable-Licenses] or that may be listed on the [Unacceptable Licenses][Unacceptable-Licenses] list may be submitted to the legal team for review and use on a case by case basis. Please email `legal@gitlab.com` with the details of how the software will be used, whether it will be modified or not, and how it will be distributed (if at all). After a decision has been made, the original requestor is responsible for updating this document, if applicable. Not all approvals will be approved for universal use and may continue to remain on the Unacceptable License list. + +All inquiries relating to patents should be directed to the Legal team. ## Notes -- cgit v1.2.1 From e339e453e4637d1b354d351e40f60eb0353fdc07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Wed, 29 May 2019 09:15:06 +0000 Subject: doc: add git mr command promotion --- doc/user/project/merge_requests/index.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index 09736c7fc7e..abd61970f7a 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -606,6 +606,9 @@ And to check out a particular merge request: git checkout origin/merge-requests/1 ``` +all the above can be done with [git-mr] script. + +[git-mr]: https://gitlab.com/glensc/git-mr [products]: https://about.gitlab.com/products/ "GitLab products page" [protected branches]: ../protected_branches.md [ci]: ../../../ci/README.md -- cgit v1.2.1 From fa89a6089e6a339d5cfdf6898d38e337a7a56515 Mon Sep 17 00:00:00 2001 From: jboyson1 Date: Thu, 30 May 2019 21:48:40 -0500 Subject: Fix broken floating point tests Update tests to use toBeCloseTo instead of toBe for floating point checks. More info here: https://jestjs.io/docs/en/expect#tobeclosetonumber-numdigits --- spec/javascripts/lib/utils/common_utils_spec.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 0cd077a6099..296ee85089f 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -852,20 +852,20 @@ describe('common_utils', () => { describe('roundOffFloat', () => { it('Rounds off decimal places of a float number with provided precision', () => { - expect(commonUtils.roundOffFloat(3.141592, 3)).toBe(3.142); + expect(commonUtils.roundOffFloat(3.141592, 3)).toBeCloseTo(3.142); }); it('Rounds off a float number to a whole number when provided precision is zero', () => { - expect(commonUtils.roundOffFloat(3.141592, 0)).toBe(3); - expect(commonUtils.roundOffFloat(3.5, 0)).toBe(4); + expect(commonUtils.roundOffFloat(3.141592, 0)).toBeCloseTo(3); + expect(commonUtils.roundOffFloat(3.5, 0)).toBeCloseTo(4); }); it('Rounds off float number to nearest 0, 10, 100, 1000 and so on when provided precision is below 0', () => { - expect(commonUtils.roundOffFloat(34567.14159, -1)).toBe(34570); - expect(commonUtils.roundOffFloat(34567.14159, -2)).toBe(34600); - expect(commonUtils.roundOffFloat(34567.14159, -3)).toBe(35000); - expect(commonUtils.roundOffFloat(34567.14159, -4)).toBe(30000); - expect(commonUtils.roundOffFloat(34567.14159, -5)).toBe(0); + expect(commonUtils.roundOffFloat(34567.14159, -1)).toBeCloseTo(34570); + expect(commonUtils.roundOffFloat(34567.14159, -2)).toBeCloseTo(34600); + expect(commonUtils.roundOffFloat(34567.14159, -3)).toBeCloseTo(35000); + expect(commonUtils.roundOffFloat(34567.14159, -4)).toBeCloseTo(30000); + expect(commonUtils.roundOffFloat(34567.14159, -5)).toBeCloseTo(0); }); }); -- cgit v1.2.1 From dbd6223211ec5046535544ea3fe0b8986f51a7e1 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Thu, 30 May 2019 10:23:34 +0100 Subject: Revert a default GIT_DEPTH for MR pipeline --- app/presenters/ci/build_runner_presenter.rb | 5 ++--- .../revert-git-depth-for-merge-request.yml | 5 +++++ spec/presenters/ci/build_runner_presenter_spec.rb | 22 +++------------------- 3 files changed, 10 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/revert-git-depth-for-merge-request.yml diff --git a/app/presenters/ci/build_runner_presenter.rb b/app/presenters/ci/build_runner_presenter.rb index ed3daf6585b..6d46e0bf18a 100644 --- a/app/presenters/ci/build_runner_presenter.rb +++ b/app/presenters/ci/build_runner_presenter.rb @@ -4,7 +4,6 @@ module Ci class BuildRunnerPresenter < SimpleDelegator include Gitlab::Utils::StrongMemoize - DEFAULT_GIT_DEPTH_MERGE_REQUEST = 10 RUNNER_REMOTE_TAG_PREFIX = 'refs/tags/'.freeze RUNNER_REMOTE_BRANCH_PREFIX = 'refs/remotes/origin/'.freeze @@ -28,7 +27,6 @@ module Ci def git_depth strong_memoize(:git_depth) do git_depth = variables&.find { |variable| variable[:key] == 'GIT_DEPTH' }&.dig(:value) - git_depth ||= DEFAULT_GIT_DEPTH_MERGE_REQUEST if merge_request_ref? git_depth.to_i end end @@ -39,12 +37,13 @@ module Ci if git_depth > 0 specs << refspec_for_branch(ref) if branch? || legacy_detached_merge_request_pipeline? specs << refspec_for_tag(ref) if tag? - specs << refspec_for_merge_request_ref if merge_request_ref? else specs << refspec_for_branch specs << refspec_for_tag end + specs << refspec_for_merge_request_ref if merge_request_ref? + specs end diff --git a/changelogs/unreleased/revert-git-depth-for-merge-request.yml b/changelogs/unreleased/revert-git-depth-for-merge-request.yml new file mode 100644 index 00000000000..3a258dff358 --- /dev/null +++ b/changelogs/unreleased/revert-git-depth-for-merge-request.yml @@ -0,0 +1,5 @@ +--- +title: Remove a default git depth in Pipelines for merge requests +merge_request: 28926 +author: +type: fixed diff --git a/spec/presenters/ci/build_runner_presenter_spec.rb b/spec/presenters/ci/build_runner_presenter_spec.rb index ad6cb012d0b..3430111ca9e 100644 --- a/spec/presenters/ci/build_runner_presenter_spec.rb +++ b/spec/presenters/ci/build_runner_presenter_spec.rb @@ -136,24 +136,6 @@ describe Ci::BuildRunnerPresenter do is_expected.to eq(1) end end - - context 'when pipeline is detached merge request pipeline' do - let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } - let(:pipeline) { merge_request.all_pipelines.first } - let(:build) { create(:ci_build, ref: pipeline.ref, pipeline: pipeline) } - - it 'returns the default git depth for pipelines for merge requests' do - is_expected.to eq(described_class::DEFAULT_GIT_DEPTH_MERGE_REQUEST) - end - - context 'when pipeline is legacy detached merge request pipeline' do - let(:merge_request) { create(:merge_request, :with_legacy_detached_merge_request_pipeline) } - - it 'behaves as branch pipeline' do - is_expected.to eq(0) - end - end - end end describe '#refspecs' do @@ -191,7 +173,9 @@ describe Ci::BuildRunnerPresenter do it 'returns the correct refspecs' do is_expected - .to contain_exactly('+refs/merge-requests/1/head:refs/merge-requests/1/head') + .to contain_exactly('+refs/heads/*:refs/remotes/origin/*', + '+refs/tags/*:refs/tags/*', + '+refs/merge-requests/1/head:refs/merge-requests/1/head') end context 'when pipeline is legacy detached merge request pipeline' do -- cgit v1.2.1 From 504ab29b82612d798f1d2e641ebe4f4242940c73 Mon Sep 17 00:00:00 2001 From: Jacopo Date: Tue, 28 May 2019 14:55:37 +0200 Subject: Enhance line-height of Activity feed UI Enhances line-height of Activity feed UI to 20px. --- app/assets/stylesheets/framework/variables.scss | 1 + app/assets/stylesheets/pages/events.scss | 6 +++--- .../unreleased/55253-activity-feed-ui-enhance-line-height.yml | 5 +++++ 3 files changed, 9 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/55253-activity-feed-ui-enhance-line-height.yml diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 28768bdf88f..0a3bd9bf4d1 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -381,6 +381,7 @@ $breadcrumb-min-height: 48px; $home-panel-title-row-height: 64px; $home-panel-avatar-mobile-size: 24px; $gl-line-height: 16px; +$gl-line-height-20: 20px; $gl-line-height-24: 24px; $gl-line-height-14: 14px; diff --git a/app/assets/stylesheets/pages/events.scss b/app/assets/stylesheets/pages/events.scss index e34628002ac..500f5816d38 100644 --- a/app/assets/stylesheets/pages/events.scss +++ b/app/assets/stylesheets/pages/events.scss @@ -8,7 +8,7 @@ border-bottom: 1px solid $white-normal; color: $gl-text-color-secondary; position: relative; - line-height: $gl-line-height; + line-height: $gl-line-height-20; .system-note-image { position: absolute; @@ -48,7 +48,7 @@ } .event-user-info { - margin-bottom: $gl-padding-8; + margin-bottom: $gl-padding-4; .author_name { a { @@ -67,7 +67,7 @@ } .event-body { - margin-top: $gl-padding-8; + margin-top: $gl-padding-4; margin-right: 174px; color: $gl-text-color; diff --git a/changelogs/unreleased/55253-activity-feed-ui-enhance-line-height.yml b/changelogs/unreleased/55253-activity-feed-ui-enhance-line-height.yml new file mode 100644 index 00000000000..f7dd8c59a7c --- /dev/null +++ b/changelogs/unreleased/55253-activity-feed-ui-enhance-line-height.yml @@ -0,0 +1,5 @@ +--- +title: Enhance line-height of Activity feed UI +merge_request: 28856 +author: Jacopo Beschi @jacopo-beschi +type: changed -- cgit v1.2.1 From b9cb49ad45e412d3330393d41b943c7b2957aefc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 3 Jun 2019 09:32:12 +0200 Subject: Move specs to ce directory --- spec/controllers/groups_controller_spec.rb | 22 ++++++++++++++++++++++ spec/controllers/projects_controller_spec.rb | 12 ++++++++++++ 2 files changed, 34 insertions(+) diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 1cd08200552..47d7e278183 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -141,6 +141,28 @@ describe GroupsController do end describe 'POST #create' do + it 'allows creating a group' do + sign_in(user) + + expect do + post :create, params: { group: { name: 'new_group', path: "new_group" } } + end.to change { Group.count }.by(1) + + expect(response).to have_gitlab_http_status(302) + end + + context 'authorization' do + it 'allows an admin to create a group' do + sign_in(create(:admin)) + + expect do + post :create, params: { group: { name: 'new_group', path: "new_group" } } + end.to change { Group.count }.by(1) + + expect(response).to have_gitlab_http_status(302) + end + end + context 'when creating subgroups', :nested_groups do [true, false].each do |can_create_group_status| context "and can_create_group is #{can_create_group_status}" do diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 7f1bbebd128..8d2412f97ef 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -292,6 +292,18 @@ describe ProjectsController do end describe 'GET edit' do + it 'allows an admin user to access the page' do + sign_in(create(:user, :admin)) + + get :edit, + params: { + namespace_id: project.namespace.path, + id: project.path + } + + expect(response).to have_gitlab_http_status(200) + end + it 'sets the badge API endpoint' do sign_in(user) project.add_maintainer(user) -- cgit v1.2.1 From 157b4e0dfa8dd6d79f5b844807c6a8d0aec41db9 Mon Sep 17 00:00:00 2001 From: Marcia Ramos Date: Mon, 3 Jun 2019 16:32:01 +0100 Subject: Fixes note, inline links --- doc/user/project/file_lock.md | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/doc/user/project/file_lock.md b/doc/user/project/file_lock.md index 541023692ea..3386eb9d0d4 100644 --- a/doc/user/project/file_lock.md +++ b/doc/user/project/file_lock.md @@ -1,10 +1,8 @@ # File Locking **[PREMIUM]** -> **Notes:** -> - [Introduced][ee-440] in [GitLab Premium][ee] 8.9. -> - This feature needs to have a license with the "File Lock" option enabled. If -> you are using Premium but you don't see the "Lock" button, -> ask your GitLab administrator. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/440) in [GitLab Premium](https://about.gitlab.com/pricing/) 8.9. +> - This feature needs to have a license with the "File Lock" option enabled. +> - If you are using Premium but you don't see the "Lock" button, ask your GitLab administrator. File Locking helps you avoid merge conflicts and better manage your binary files. Lock any file or directory, make your changes, and then unlock it so another @@ -37,7 +35,7 @@ lies under them is also locked. The user that locks a file or directory **is the only one** that can edit and push their changes back to the repository where the locked objects are located. -Locks can be created by any person who has [push access] to the repository; i.e., +Locks can be created by any person who has [push access](../../user/permissions.md) to the repository; i.e., Developer and higher level, and can be removed solely by their author and any user with Maintainer permissions and above. @@ -101,7 +99,3 @@ To view or manage every existing lock, navigate to the locks and [remove the ones you have permission for](#permissions-on-file-locking). ![Locked Files](img/file_lock_list.png) - -[ee-440]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/440 "File Lock" -[ee]: https://about.gitlab.com/pricing/ -[push access]: ../../user/permissions.md -- cgit v1.2.1 From ed503d51a39943b482e917028d589cc26ec01c95 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 3 Jun 2019 19:38:16 +0200 Subject: Expose IDs in GraphQL as a GlobalID This exposes all fields named `id` as GlobalIDs so they can be used across our entire GraphQL implementation. When the objects loaded are `ApplicationRecord`s. We'll use our existing batchloading to find them. Otherwise, we'll fall back to the default implementation of `GlobalID`: Calling the `.find` method on the class. --- app/graphql/gitlab_schema.rb | 25 ++++++++++ app/graphql/types/base_object.rb | 5 ++ .../unreleased/bvl-use-global-ids-graphql.yml | 5 ++ doc/development/api_graphql_styleguide.md | 21 ++++++-- lib/gitlab/graphql/loaders/batch_model_loader.rb | 2 +- spec/graphql/features/authorization_spec.rb | 2 +- spec/graphql/gitlab_schema_spec.rb | 58 ++++++++++++++++++++++ spec/requests/api/graphql/gitlab_schema_spec.rb | 13 ++++- spec/requests/api/graphql/group_query_spec.rb | 2 +- .../api/graphql/namespace/projects_spec.rb | 2 +- 10 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/bvl-use-global-ids-graphql.yml diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index f8ad6bee21b..2e5bdbd79c8 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -45,6 +45,31 @@ class GitlabSchema < GraphQL::Schema super(query_str, **kwargs) end + def id_from_object(object) + unless object.respond_to?(:to_global_id) + # This is an error in our schema and needs to be solved. So raise a + # more meaningfull error message + raise "#{object} does not implement `to_global_id`. "\ + "Include `GlobalID::Identification` into `#{object.class}" + end + + object.to_global_id + end + + def object_from_id(global_id) + gid = GlobalID.parse(global_id) + + unless gid + raise Gitlab::Graphql::Errors::ArgumentError, "#{global_id} is not a valid GitLab id." + end + + if gid.model_class < ApplicationRecord + Gitlab::Graphql::Loaders::BatchModelLoader.new(gid.model_class, gid.model_id).find + else + gid.find + end + end + private def max_query_complexity(ctx) diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb index 82b78abd573..e40059c46bb 100644 --- a/app/graphql/types/base_object.rb +++ b/app/graphql/types/base_object.rb @@ -6,5 +6,10 @@ module Types prepend Gitlab::Graphql::ExposePermissions field_class Types::BaseField + + # All graphql fields exposing an id, should expose a global id. + def id + GitlabSchema.id_from_object(object) + end end end diff --git a/changelogs/unreleased/bvl-use-global-ids-graphql.yml b/changelogs/unreleased/bvl-use-global-ids-graphql.yml new file mode 100644 index 00000000000..34cb65e6001 --- /dev/null +++ b/changelogs/unreleased/bvl-use-global-ids-graphql.yml @@ -0,0 +1,5 @@ +--- +title: Use global IDs when exposing GraphQL resources +merge_request: 29080 +author: +type: added diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index 8d2bfff3a5d..38270af682e 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -32,6 +32,21 @@ a new presenter specifically for GraphQL. The presenter is initialized using the object resolved by a field, and the context. +### Exposing Global ids + +When exposing an `id` field on a type, we will by default try to +expose a global id by calling `to_global_id` on the resource being +rendered. + +To override this behaviour, you can implement an `id` method on the +type for which you are exposing an id. Please make sure that when +exposing a `GraphQL::ID_TYPE` using a custom method that it is +globally unique. + +The records that are exposing a `full_path` as an `ID_TYPE` are one of +these exceptions. Since the full path is a unique identifier for a +`Project` or `Namespace`. + ### Connection Types GraphQL uses [cursor based @@ -79,14 +94,14 @@ look like this: { "cursor": "Nzc=", "node": { - "id": "77", + "id": "gid://gitlab/Pipeline/77", "status": "FAILED" } }, { "cursor": "Njc=", "node": { - "id": "67", + "id": "gid://gitlab/Pipeline/67", "status": "FAILED" } } @@ -330,7 +345,7 @@ argument :project_path, GraphQL::ID_TYPE, required: true, description: "The project the merge request to mutate is in" -argument :iid, GraphQL::ID_TYPE, +argument :iid, GraphQL::STRING_TYPE, required: true, description: "The iid of the merge request to mutate" diff --git a/lib/gitlab/graphql/loaders/batch_model_loader.rb b/lib/gitlab/graphql/loaders/batch_model_loader.rb index 5a0099dc6b1..50d3293fcbb 100644 --- a/lib/gitlab/graphql/loaders/batch_model_loader.rb +++ b/lib/gitlab/graphql/loaders/batch_model_loader.rb @@ -12,7 +12,7 @@ module Gitlab # rubocop: disable CodeReuse/ActiveRecord def find - BatchLoader.for({ model: model_class, id: model_id }).batch do |loader_info, loader| + BatchLoader.for({ model: model_class, id: model_id.to_i }).batch do |loader_info, loader| per_model = loader_info.group_by { |info| info[:model] } per_model.each do |model, info| ids = info.map { |i| i[:id] } diff --git a/spec/graphql/features/authorization_spec.rb b/spec/graphql/features/authorization_spec.rb index f5eb628a982..c427893f9cc 100644 --- a/spec/graphql/features/authorization_spec.rb +++ b/spec/graphql/features/authorization_spec.rb @@ -282,7 +282,7 @@ describe 'Gitlab::Graphql::Authorization' do issue_ids = issue_edges.map { |issue_edge| issue_edge['node']&.fetch('id') } expect(issue_edges.size).to eq(visible_issues.size) - expect(issue_ids).to eq(visible_issues.map { |i| i.id.to_s }) + expect(issue_ids).to eq(visible_issues.map { |i| i.to_global_id.to_s }) end it 'does not check access on fields that will not be rendered' do diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index e9149f4250f..4076c1f824b 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -107,6 +107,64 @@ describe GitlabSchema do end end + describe '.id_from_object' do + it 'returns a global id' do + expect(described_class.id_from_object(build(:project, id: 1))).to be_a(GlobalID) + end + + it "raises a meaningful error if a global id couldn't be generated" do + expect { described_class.id_from_object(build(:commit)) } + .to raise_error(RuntimeError, /include `GlobalID::Identification` into/i) + end + end + + describe '.object_from_id' do + context 'for subclasses of `ApplicationRecord`' do + it 'returns the correct record' do + user = create(:user) + + result = described_class.object_from_id(user.to_global_id.to_s) + + expect(result.__sync).to eq(user) + end + + it 'batchloads the queries' do + user1 = create(:user) + user2 = create(:user) + + expect do + [described_class.object_from_id(user1.to_global_id), + described_class.object_from_id(user2.to_global_id)].map(&:__sync) + end.not_to exceed_query_limit(1) + end + end + + context 'for other classes' do + # We cannot use an anonymous class here as `GlobalID` expects `.name` not + # to return `nil` + class TestGlobalId + include GlobalID::Identification + attr_accessor :id + + def initialize(id) + @id = id + end + end + + it 'falls back to a regular find' do + result = TestGlobalId.new(123) + + expect(TestGlobalId).to receive(:find).with("123").and_return(result) + + expect(described_class.object_from_id(result.to_global_id)).to eq(result) + end + end + + it 'raises the correct error on invalid input' do + expect { described_class.object_from_id("bogus id") }.to raise_error(Gitlab::Graphql::Errors::ArgumentError) + end + end + def field_instrumenters described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins] end diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index b6ca9246399..28676bb02f4 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'GitlabSchema configurations' do include GraphqlHelpers - let(:project) { create(:project) } + set(:project) { create(:project) } shared_examples 'imposing query limits' do describe '#max_complexity' do @@ -136,4 +136,15 @@ describe 'GitlabSchema configurations' do post_graphql(query, current_user: nil) end end + + context "global id's" do + it 'uses GlobalID to expose ids' do + post_graphql(graphql_query_for('project', { 'fullPath' => project.full_path }, %w(id)), + current_user: project.owner) + + parsed_id = GlobalID.parse(graphql_data['project']['id']) + + expect(parsed_id).to eq(project.to_global_id) + end + end end diff --git a/spec/requests/api/graphql/group_query_spec.rb b/spec/requests/api/graphql/group_query_spec.rb index db9f2ac9dd0..e0f1e4dbe9e 100644 --- a/spec/requests/api/graphql/group_query_spec.rb +++ b/spec/requests/api/graphql/group_query_spec.rb @@ -56,7 +56,7 @@ describe 'getting group information' do post_graphql(group_query(group1), current_user: user1) expect(response).to have_gitlab_http_status(200) - expect(graphql_data['group']['id']).to eq(group1.id.to_s) + expect(graphql_data['group']['id']).to eq(group1.to_global_id.to_s) expect(graphql_data['group']['name']).to eq(group1.name) expect(graphql_data['group']['path']).to eq(group1.path) expect(graphql_data['group']['description']).to eq(group1.description) diff --git a/spec/requests/api/graphql/namespace/projects_spec.rb b/spec/requests/api/graphql/namespace/projects_spec.rb index e05273da4bd..de1cd9586b6 100644 --- a/spec/requests/api/graphql/namespace/projects_spec.rb +++ b/spec/requests/api/graphql/namespace/projects_spec.rb @@ -60,7 +60,7 @@ describe 'getting projects', :nested_groups do expect(graphql_data['namespace']['projects']['edges'].size).to eq(1) project = graphql_data['namespace']['projects']['edges'][0]['node'] - expect(project['id']).to eq(public_project.id.to_s) + expect(project['id']).to eq(public_project.to_global_id.to_s) end end end -- cgit v1.2.1 From f16b13113ff580fbde78f8f6ba42a3f86c04ca12 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 3 Jun 2019 20:15:04 +0200 Subject: Fix incorrect instances of `GraphQL::ID_TYPE` Since the `GraphQL::ID_TYPE` usages should represent globally unique ids, this changes some fields for which this is not the case into strings. The `ID_TYPE` is a specialised, so this change should be backwards compatible. https://graphql-ruby.org/type_definitions/scalars.html --- app/graphql/mutations/merge_requests/base.rb | 2 +- app/graphql/resolvers/issues_resolver.rb | 4 ++-- app/graphql/resolvers/merge_requests_resolver.rb | 4 ++-- app/graphql/types/ci/pipeline_type.rb | 2 +- app/graphql/types/merge_request_type.rb | 2 +- spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/graphql/mutations/merge_requests/base.rb b/app/graphql/mutations/merge_requests/base.rb index 7d0cb777ad1..e85d16fc2c5 100644 --- a/app/graphql/mutations/merge_requests/base.rb +++ b/app/graphql/mutations/merge_requests/base.rb @@ -10,7 +10,7 @@ module Mutations required: true, description: "The project the merge request to mutate is in" - argument :iid, GraphQL::ID_TYPE, + argument :iid, GraphQL::STRING_TYPE, required: true, description: "The iid of the merge request to mutate" diff --git a/app/graphql/resolvers/issues_resolver.rb b/app/graphql/resolvers/issues_resolver.rb index 3ee3849f483..6988b451ec3 100644 --- a/app/graphql/resolvers/issues_resolver.rb +++ b/app/graphql/resolvers/issues_resolver.rb @@ -2,11 +2,11 @@ module Resolvers class IssuesResolver < BaseResolver - argument :iid, GraphQL::ID_TYPE, + argument :iid, GraphQL::STRING_TYPE, required: false, description: 'The IID of the issue, e.g., "1"' - argument :iids, [GraphQL::ID_TYPE], + argument :iids, [GraphQL::STRING_TYPE], required: false, description: 'The list of IIDs of issues, e.g., [1, 2]' argument :state, Types::IssuableStateEnum, diff --git a/app/graphql/resolvers/merge_requests_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb index 90795c797ac..b84e60066e1 100644 --- a/app/graphql/resolvers/merge_requests_resolver.rb +++ b/app/graphql/resolvers/merge_requests_resolver.rb @@ -2,11 +2,11 @@ module Resolvers class MergeRequestsResolver < BaseResolver - argument :iid, GraphQL::ID_TYPE, + argument :iid, GraphQL::STRING_TYPE, required: false, description: 'The IID of the merge request, e.g., "1"' - argument :iids, [GraphQL::ID_TYPE], + argument :iids, [GraphQL::STRING_TYPE], required: false, description: 'The list of IIDs of issues, e.g., [1, 2]' diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb index de7d6570a3e..cff81e5670b 100644 --- a/app/graphql/types/ci/pipeline_type.rb +++ b/app/graphql/types/ci/pipeline_type.rb @@ -10,7 +10,7 @@ module Types expose_permissions Types::PermissionTypes::Ci::Pipeline field :id, GraphQL::ID_TYPE, null: false - field :iid, GraphQL::ID_TYPE, null: false + field :iid, GraphQL::STRING_TYPE, null: false field :sha, GraphQL::STRING_TYPE, null: false field :before_sha, GraphQL::STRING_TYPE, null: true diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 120ffe0dfde..85ac3102442 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -11,7 +11,7 @@ module Types present_using MergeRequestPresenter field :id, GraphQL::ID_TYPE, null: false - field :iid, GraphQL::ID_TYPE, null: false + field :iid, GraphQL::STRING_TYPE, null: false field :title, GraphQL::STRING_TYPE, null: false field :description, GraphQL::STRING_TYPE, null: true field :state, MergeRequestStateEnum, null: false diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb index 8f427d71a32..d75f0df9fd3 100644 --- a/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb +++ b/spec/requests/api/graphql/mutations/merge_requests/set_wip_spec.rb @@ -11,7 +11,7 @@ describe 'Setting WIP status of a merge request' do let(:mutation) do variables = { project_path: project.full_path, - iid: merge_request.iid + iid: merge_request.iid.to_s } graphql_mutation(:merge_request_set_wip, variables.merge(input)) end -- cgit v1.2.1 From 6610f2fdcb4b641e9c1c87891e9de1ca28c1f518 Mon Sep 17 00:00:00 2001 From: Nathan Friend Date: Fri, 31 May 2019 09:58:23 -0300 Subject: Add auto SSL toggle to Pages domain settings page This commit adds an auto SSL toggle switch to the settings page of GitLab Pages domains. This toggle enable or disabled auto SSL management via Let's Encrypt. Toggling the button dynamically updates the form using client-side JavaScript. All changes are behind feature flags. --- .../pages/projects/pages_domains/edit/index.js | 3 + .../pages/projects/pages_domains/form.js | 43 +++++++ .../pages/projects/pages_domains/new/index.js | 3 + .../projects/pages_domains_controller.rb | 4 +- app/views/projects/pages_domains/_form.html.haml | 87 +++++++++++--- .../projects/pages_domains/_helper_text.html.haml | 9 ++ app/views/projects/pages_domains/edit.html.haml | 1 + app/views/projects/pages_domains/new.html.haml | 1 + .../unreleased/28996-create-mvc-ui-in-haml.yml | 5 + locale/gitlab.pot | 18 +++ spec/features/projects/pages_lets_encrypt_spec.rb | 131 +++++++++++++++++++++ spec/features/projects/pages_spec.rb | 21 +++- 12 files changed, 305 insertions(+), 21 deletions(-) create mode 100644 app/assets/javascripts/pages/projects/pages_domains/edit/index.js create mode 100644 app/assets/javascripts/pages/projects/pages_domains/form.js create mode 100644 app/assets/javascripts/pages/projects/pages_domains/new/index.js create mode 100644 app/views/projects/pages_domains/_helper_text.html.haml create mode 100644 changelogs/unreleased/28996-create-mvc-ui-in-haml.yml create mode 100644 spec/features/projects/pages_lets_encrypt_spec.rb diff --git a/app/assets/javascripts/pages/projects/pages_domains/edit/index.js b/app/assets/javascripts/pages/projects/pages_domains/edit/index.js new file mode 100644 index 00000000000..27e4433ad4d --- /dev/null +++ b/app/assets/javascripts/pages/projects/pages_domains/edit/index.js @@ -0,0 +1,3 @@ +import initForm from '~/pages/projects/pages_domains/form'; + +document.addEventListener('DOMContentLoaded', initForm); diff --git a/app/assets/javascripts/pages/projects/pages_domains/form.js b/app/assets/javascripts/pages/projects/pages_domains/form.js new file mode 100644 index 00000000000..1d0dbfe0406 --- /dev/null +++ b/app/assets/javascripts/pages/projects/pages_domains/form.js @@ -0,0 +1,43 @@ +import setupToggleButtons from '~/toggle_buttons'; + +export default () => { + const toggleContainer = document.querySelector('.js-auto-ssl-toggle-container'); + + if (toggleContainer) { + const onToggleButtonClicked = isAutoSslEnabled => { + Array.from(document.querySelectorAll('.js-shown-if-auto-ssl')).forEach(el => { + if (isAutoSslEnabled) { + el.classList.remove('d-none'); + } else { + el.classList.add('d-none'); + } + }); + + Array.from(document.querySelectorAll('.js-shown-unless-auto-ssl')).forEach(el => { + if (isAutoSslEnabled) { + el.classList.add('d-none'); + } else { + el.classList.remove('d-none'); + } + }); + + Array.from(document.querySelectorAll('.js-enabled-if-auto-ssl')).forEach(el => { + if (isAutoSslEnabled) { + el.removeAttribute('disabled'); + } else { + el.setAttribute('disabled', 'disabled'); + } + }); + + Array.from(document.querySelectorAll('.js-enabled-unless-auto-ssl')).forEach(el => { + if (isAutoSslEnabled) { + el.setAttribute('disabled', 'disabled'); + } else { + el.removeAttribute('disabled'); + } + }); + }; + + setupToggleButtons(toggleContainer, onToggleButtonClicked); + } +}; diff --git a/app/assets/javascripts/pages/projects/pages_domains/new/index.js b/app/assets/javascripts/pages/projects/pages_domains/new/index.js new file mode 100644 index 00000000000..27e4433ad4d --- /dev/null +++ b/app/assets/javascripts/pages/projects/pages_domains/new/index.js @@ -0,0 +1,3 @@ +import initForm from '~/pages/projects/pages_domains/form'; + +document.addEventListener('DOMContentLoaded', initForm); diff --git a/app/controllers/projects/pages_domains_controller.rb b/app/controllers/projects/pages_domains_controller.rb index 58b1bc54181..89f21d8dadb 100644 --- a/app/controllers/projects/pages_domains_controller.rb +++ b/app/controllers/projects/pages_domains_controller.rb @@ -65,11 +65,11 @@ class Projects::PagesDomainsController < Projects::ApplicationController private def create_params - params.require(:pages_domain).permit(:key, :certificate, :domain) + params.require(:pages_domain).permit(:key, :certificate, :domain, :auto_ssl_enabled) end def update_params - params.require(:pages_domain).permit(:key, :certificate) + params.require(:pages_domain).permit(:key, :certificate, :auto_ssl_enabled) end # rubocop: disable CodeReuse/ActiveRecord diff --git a/app/views/projects/pages_domains/_form.html.haml b/app/views/projects/pages_domains/_form.html.haml index 1e50a101c1e..33f2166480b 100644 --- a/app/views/projects/pages_domains/_form.html.haml +++ b/app/views/projects/pages_domains/_form.html.haml @@ -1,29 +1,80 @@ - if @domain.errors.any? - #error_explanation - .alert.alert-danger - - @domain.errors.full_messages.each do |msg| - %p= msg + .alert.alert-danger + - @domain.errors.full_messages.each do |msg| + = msg .form-group.row .col-sm-2.col-form-label = f.label :domain, _("Domain") .col-sm-10 - = f.text_field :domain, required: true, autocomplete: 'off', class: 'form-control', disabled: @domain.persisted? + = f.text_field :domain, required: true, autocomplete: "off", class: "form-control", disabled: @domain.persisted? - if Gitlab.config.pages.external_https - .form-group.row - .col-sm-2.col-form-label - = f.label :certificate, _("Certificate (PEM)") - .col-sm-10 - = f.text_area :certificate, rows: 5, class: 'form-control' - %span.help-inline= _("Upload a certificate for your domain with all intermediates") - - .form-group.row - .col-sm-2.col-form-label - = f.label :key, _("Key (PEM)") - .col-sm-10 - = f.text_area :key, rows: 5, class: 'form-control' - %span.help-inline= _("Upload a private key for your certificate") + + - auto_ssl_available = Feature.enabled?(:pages_auto_ssl) + - auto_ssl_enabled = @domain.auto_ssl_enabled? + - auto_ssl_available_and_enabled = auto_ssl_available && auto_ssl_enabled + + - if auto_ssl_available + .form-group.row + .col-sm-2.col-form-label + %label{ for: "pages_domain_auto_ssl_enabled_button" } + - lets_encrypt_link_url = "https://letsencrypt.org/" + - lets_encrypt_link_start = "".html_safe % { lets_encrypt_link_url: lets_encrypt_link_url } + - lets_encrypt_link_end = "".html_safe + = _("Automatic certificate management using %{lets_encrypt_link_start}Let's Encrypt%{lets_encrypt_link_end}").html_safe % { lets_encrypt_link_start: lets_encrypt_link_start, lets_encrypt_link_end: lets_encrypt_link_end } + + .col-sm-10.js-auto-ssl-toggle-container + %button{ type: "button", id: "pages_domain_auto_ssl_enabled_button", + class: "js-project-feature-toggle project-feature-toggle mt-2 #{"is-checked" if auto_ssl_available_and_enabled}", + "aria-label": _("Automatic certificate management using Let's Encrypt") } + = f.hidden_field :auto_ssl_enabled?, class: "js-project-feature-toggle-input" + %span.toggle-icon + = sprite_icon("status_success_borderless", size: 16, css_class: "toggle-icon-svg toggle-status-checked") + = sprite_icon("status_failed_borderless", size: 16, css_class: "toggle-icon-svg toggle-status-unchecked") + %p.text-secondary.mt-3 + - docs_link_url = help_page_path("user/project/pages/lets_encrypt_for_gitlab_pages.md", anchor: "lets-encrypt-for-gitlab-pages") + - docs_link_start = "".html_safe % { docs_link_url: docs_link_url } + - docs_link_end = "".html_safe + = _("Let's Encrypt is a free, automated, and open certificate authority (CA) that gives digital certificates in order to enable HTTPS (SSL/TLS) for websites. Learn more about Let's Encrypt configuration by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}.").html_safe % { docs_link_url: docs_link_url, docs_link_start: docs_link_start, docs_link_end: docs_link_end } + + .js-shown-if-auto-ssl{ class: ("d-none" unless auto_ssl_available_and_enabled) } + .form-group.row + .col-sm-2.col-form-label + = f.label :certificate, _("Certificate (PEM)") + .col-sm-10 + - if auto_ssl_available_and_enabled && !@domain.certificate.empty? + = f.text_area :certificate, + rows: 5, + class: "form-control", + disabled: true + %span.help-inline.text-muted= _("This certificate is automatically managed by Let's Encrypt") + - else + %p.text-secondary.form-control-plaintext= _("The certificate will be shown here once it has been obtained from Let's Encrypt. This process may take up to an hour to complete.") + + .js-shown-unless-auto-ssl{ class: ("d-none" if auto_ssl_available_and_enabled) } + .form-group.row + .col-sm-2.col-form-label + = f.label :certificate, _("Certificate (PEM)") + .col-sm-10 + = f.text_area :certificate, + rows: 5, + class: "form-control js-enabled-unless-auto-ssl", + value: (@domain.certificate unless auto_ssl_available_and_enabled), + disabled: auto_ssl_available_and_enabled + %span.help-inline.text-muted= _("Upload a certificate for your domain with all intermediates") + + .form-group.row + .col-sm-2.col-form-label + = f.label :key, _("Key (PEM)") + .col-sm-10 + = f.text_area :key, + rows: 5, + class: "form-control js-enabled-unless-auto-ssl", + value: (@domain.key unless auto_ssl_available_and_enabled), + disabled: auto_ssl_available_and_enabled + %span.help-inline.text-muted= _("Upload a private key for your certificate") + - else .nothing-here-block = _("Support for custom certificates is disabled. Ask your system's administrator to enable it.") diff --git a/app/views/projects/pages_domains/_helper_text.html.haml b/app/views/projects/pages_domains/_helper_text.html.haml new file mode 100644 index 00000000000..5a79fefabfc --- /dev/null +++ b/app/views/projects/pages_domains/_helper_text.html.haml @@ -0,0 +1,9 @@ +- docs_link_url = help_page_path("user/project/pages/getting_started_part_three.md", anchor: "adding-certificates-to-your-project") +- docs_link_start = "".html_safe % { docs_link_url: docs_link_url } +- docs_link_end = "".html_safe + +-# Hiding behind a feature flag to avoid any changes to this feature's implemention +-# when the :pages_auto_ssl feature flag is disabled. This check should be removed +-# once the :pages_auto_ssl feature flag is removed. +- if Feature.enabled?(:pages_auto_ssl) + %p= _("Learn more about adding certificates to your project by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}.").html_safe % { docs_link_url: docs_link_url, docs_link_start: docs_link_start, docs_link_end: docs_link_end } diff --git a/app/views/projects/pages_domains/edit.html.haml b/app/views/projects/pages_domains/edit.html.haml index e11387ae742..7c0777e5496 100644 --- a/app/views/projects/pages_domains/edit.html.haml +++ b/app/views/projects/pages_domains/edit.html.haml @@ -3,6 +3,7 @@ - page_title @domain.domain %h3.page-title = @domain.domain += render 'projects/pages_domains/helper_text' %hr.clearfix %div = form_for [@project.namespace.becomes(Namespace), @project, @domain], html: { class: 'fieldset-form' } do |f| diff --git a/app/views/projects/pages_domains/new.html.haml b/app/views/projects/pages_domains/new.html.haml index c7cefa87c76..e23ccb5d4c6 100644 --- a/app/views/projects/pages_domains/new.html.haml +++ b/app/views/projects/pages_domains/new.html.haml @@ -2,6 +2,7 @@ - page_title _('New Pages Domain') %h3.page-title = _("New Pages Domain") += render 'projects/pages_domains/helper_text' %hr.clearfix %div = form_for [@project.namespace.becomes(Namespace), @project, @domain], html: { class: 'fieldset-form' } do |f| diff --git a/changelogs/unreleased/28996-create-mvc-ui-in-haml.yml b/changelogs/unreleased/28996-create-mvc-ui-in-haml.yml new file mode 100644 index 00000000000..9c6897babb4 --- /dev/null +++ b/changelogs/unreleased/28996-create-mvc-ui-in-haml.yml @@ -0,0 +1,5 @@ +--- +title: Add auto SSL toggle option to Pages domain settings page +merge_request: 26438 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index afce09cd621..fc579d804b9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1329,6 +1329,12 @@ msgstr "" msgid "AutoDevOps|The Auto DevOps pipeline has been enabled and will be used if no alternative CI configuration file is found. %{more_information_link}" msgstr "" +msgid "Automatic certificate management using %{lets_encrypt_link_start}Let's Encrypt%{lets_encrypt_link_end}" +msgstr "" + +msgid "Automatic certificate management using Let's Encrypt" +msgstr "" + msgid "Automatically marked as default internal user" msgstr "" @@ -5803,6 +5809,9 @@ msgstr "" msgid "Learn more about Kubernetes" msgstr "" +msgid "Learn more about adding certificates to your project by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}." +msgstr "" + msgid "Learn more about signing commits" msgstr "" @@ -5830,6 +5839,9 @@ msgstr "" msgid "Let's Encrypt does not accept emails on example.com" msgstr "" +msgid "Let's Encrypt is a free, automated, and open certificate authority (CA) that gives digital certificates in order to enable HTTPS (SSL/TLS) for websites. Learn more about Let's Encrypt configuration by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}." +msgstr "" + msgid "Limited to showing %d event at most" msgid_plural "Limited to showing %d events at most" msgstr[0] "" @@ -9864,6 +9876,9 @@ msgstr "" msgid "The X509 Certificate to use when mutual TLS is required to communicate with the external authorization service. If left blank, the server certificate is still validated when accessing over HTTPS." msgstr "" +msgid "The certificate will be shown here once it has been obtained from Let's Encrypt. This process may take up to an hour to complete." +msgstr "" + msgid "The character highlighter helps you keep the subject line to %{titleLength} characters and wrap the body at %{bodyLength} so they are readable in git." msgstr "" @@ -10185,6 +10200,9 @@ msgstr "" msgid "This branch has changed since you started editing. Would you like to create a new branch?" msgstr "" +msgid "This certificate is automatically managed by Let's Encrypt" +msgstr "" + msgid "This commit is part of merge request %{link_to_merge_request}. Comments created here will be created in the context of that merge request." msgstr "" diff --git a/spec/features/projects/pages_lets_encrypt_spec.rb b/spec/features/projects/pages_lets_encrypt_spec.rb new file mode 100644 index 00000000000..baa217cbe58 --- /dev/null +++ b/spec/features/projects/pages_lets_encrypt_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe "Pages with Let's Encrypt", :https_pages_enabled do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:role) { :maintainer } + let(:certificate_pem) do + <<~PEM + -----BEGIN CERTIFICATE----- + MIICGzCCAYSgAwIBAgIBATANBgkqhkiG9w0BAQUFADAbMRkwFwYDVQQDExB0ZXN0 + LWNlcnRpZmljYXRlMB4XDTE2MDIxMjE0MzIwMFoXDTIwMDQxMjE0MzIwMFowGzEZ + MBcGA1UEAxMQdGVzdC1jZXJ0aWZpY2F0ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAw + gYkCgYEApL4J9L0ZxFJ1hI1LPIflAlAGvm6ZEvoT4qKU5Xf2JgU7/2geNR1qlNFa + SvCc08Knupp5yTgmvyK/Xi09U0N82vvp4Zvr/diSc4A/RA6Mta6egLySNT438kdT + nY2tR5feoTLwQpX0t4IMlwGQGT5h6Of2fKmDxzuwuyffcIHqLdsCAwEAAaNvMG0w + DAYDVR0TAQH/BAIwADAdBgNVHQ4EFgQUxl9WSxBprB0z0ibJs3rXEk0+95AwCwYD + VR0PBAQDAgXgMBEGCWCGSAGG+EIBAQQEAwIGQDAeBglghkgBhvhCAQ0EERYPeGNh + IGNlcnRpZmljYXRlMA0GCSqGSIb3DQEBBQUAA4GBAGC4T8SlFHK0yPSa+idGLQFQ + joZp2JHYvNlTPkRJ/J4TcXxBTJmArcQgTIuNoBtC+0A/SwdK4MfTCUY4vNWNdese + 5A4K65Nb7Oh1AdQieTBHNXXCdyFsva9/ScfQGEl7p55a52jOPs0StPd7g64uvjlg + YHi2yesCrOvVXt+lgPTd + -----END CERTIFICATE----- + PEM + end + + let(:certificate_key) do + <<~KEY + -----BEGIN PRIVATE KEY----- + MIICdgIBADANBgkqhkiG9w0BAQEFAASCAmAwggJcAgEAAoGBAKS+CfS9GcRSdYSN + SzyH5QJQBr5umRL6E+KilOV39iYFO/9oHjUdapTRWkrwnNPCp7qaeck4Jr8iv14t + PVNDfNr76eGb6/3YknOAP0QOjLWunoC8kjU+N/JHU52NrUeX3qEy8EKV9LeCDJcB + kBk+Yejn9nypg8c7sLsn33CB6i3bAgMBAAECgYA2D26w80T7WZvazYr86BNMePpd + j2mIAqx32KZHzt/lhh40J/SRtX9+Kl0Y7nBoRR5Ja9u/HkAIxNxLiUjwg9r6cpg/ + uITEF5nMt7lAk391BuI+7VOZZGbJDsq2ulPd6lO+C8Kq/PI/e4kXcIjeH6KwQsuR + 5vrXfBZ3sQfflaiN4QJBANBt8JY2LIGQF8o89qwUpRL5vbnKQ4IzZ5+TOl4RLR7O + AQpJ81tGuINghO7aunctb6rrcKJrxmEH1whzComybrMCQQDKV49nOBudRBAIgG4K + EnLzsRKISUHMZSJiYTYnablof8cKw1JaQduw7zgrUlLwnroSaAGX88+Jw1f5n2Lh + Vlg5AkBDdUGnrDLtYBCDEQYZHblrkc7ZAeCllDOWjxUV+uMqlCv8A4Ey6omvY57C + m6I8DkWVAQx8VPtozhvHjUw80rZHAkB55HWHAM3h13axKG0htCt7klhPsZHpx6MH + EPjGlXIT+aW2XiPmK3ZlCDcWIenE+lmtbOpI159Wpk8BGXs/s/xBAkEAlAY3ymgx + 63BDJEwvOb2IaP8lDDxNsXx9XJNVvQbv5n15vNsLHbjslHfAhAbxnLQ1fLhUPqSi + nNp/xedE1YxutQ== + -----END PRIVATE KEY----- + KEY + end + + before do + allow(Gitlab.config.pages).to receive(:enabled).and_return(true) + project.add_role(user, role) + sign_in(user) + project.namespace.update(owner: user) + allow_any_instance_of(Project).to receive(:pages_deployed?) { true } + end + + context 'when the page_auto_ssl feature flag is enabled' do + before do + stub_feature_flags(pages_auto_ssl: true) + end + + context 'when the auto SSL management is initially disabled' do + let(:domain) do + create(:pages_domain, auto_ssl_enabled: false, project: project) + end + + it 'enables auto SSL and dynamically updates the form accordingly', :js do + visit edit_project_pages_domain_path(project, domain) + + expect(domain.auto_ssl_enabled).to eq false + + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).to have_field 'Key (PEM)', type: 'textarea' + + find('.js-auto-ssl-toggle-container .project-feature-toggle').click + + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' + expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).not_to have_field 'Key (PEM)', type: 'textarea' + expect(page).to have_content "The certificate will be shown here once it has been obtained from Let's Encrypt. This process may take up to an hour to complete." + + click_on 'Save Changes' + + expect(domain.reload.auto_ssl_enabled).to eq true + end + end + + context 'when the auto SSL management is initially enabled' do + let(:domain) do + create(:pages_domain, auto_ssl_enabled: true, project: project) + end + + it 'disables auto SSL and dynamically updates the form accordingly', :js do + visit edit_project_pages_domain_path(project, domain) + + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea', disabled: true + expect(page).not_to have_field 'Key (PEM)', type: 'textarea' + + find('.js-auto-ssl-toggle-container .project-feature-toggle').click + + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).to have_field 'Key (PEM)', type: 'textarea' + + fill_in 'Certificate (PEM)', with: certificate_pem + fill_in 'Key (PEM)', with: certificate_key + + click_on 'Save Changes' + + expect(domain.reload.auto_ssl_enabled).to eq false + end + end + end + + context 'when the page_auto_ssl feature flag is disabled' do + let(:domain) do + create(:pages_domain, auto_ssl_enabled: false, project: project) + end + + before do + stub_feature_flags(pages_auto_ssl: false) + + visit edit_project_pages_domain_path(project, domain) + end + + it "does not render the Let's Encrypt field", :js do + expect(page).not_to have_selector '.js-auto-ssl-toggle-container' + end + end +end diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb index be05c74efdb..9bb0ba81ef5 100644 --- a/spec/features/projects/pages_spec.rb +++ b/spec/features/projects/pages_spec.rb @@ -1,6 +1,7 @@ +# frozen_string_literal: true require 'spec_helper' -describe 'Pages' do +shared_examples 'pages domain editing' do let(:project) { create(:project) } let(:user) { create(:user) } let(:role) { :maintainer } @@ -318,3 +319,21 @@ describe 'Pages' do end end end + +describe 'Pages' do + context 'when pages_auto_ssl feature flag is disabled' do + before do + stub_feature_flags(pages_auto_ssl: false) + end + + include_examples 'pages domain editing' + end + + context 'when pages_auto_ssl feature flag is enabled' do + before do + stub_feature_flags(pages_auto_ssl: true) + end + + include_examples 'pages domain editing' + end +end -- cgit v1.2.1 From c7d50ddf5504828296c97447b281be17282a056e Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Tue, 4 Jun 2019 23:25:33 +1200 Subject: Fix connection to Tiller error while uninstalling Both the `install-` and `uninstall-` pods loads the `values-content-configuration-` configmap into the pod (see `#volume_specification`). This configmap contains the cert necessary to connect to Tiller. The cert though is only valid for 30 minutes. So this fixes the bug where the configmap when uninstalling should be updated as well. --- .../62713-fix-uninstalling-cluster-apps.yml | 5 +++++ lib/gitlab/kubernetes/helm/api.rb | 1 + spec/lib/gitlab/kubernetes/helm/api_spec.rb | 24 ++++++++++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 changelogs/unreleased/62713-fix-uninstalling-cluster-apps.yml diff --git a/changelogs/unreleased/62713-fix-uninstalling-cluster-apps.yml b/changelogs/unreleased/62713-fix-uninstalling-cluster-apps.yml new file mode 100644 index 00000000000..45fa668ae85 --- /dev/null +++ b/changelogs/unreleased/62713-fix-uninstalling-cluster-apps.yml @@ -0,0 +1,5 @@ +--- +title: Fix connection to Tiller error while uninstalling +merge_request: 29131 +author: +type: fixed diff --git a/lib/gitlab/kubernetes/helm/api.rb b/lib/gitlab/kubernetes/helm/api.rb index ff1dadf9247..978cafae9ac 100644 --- a/lib/gitlab/kubernetes/helm/api.rb +++ b/lib/gitlab/kubernetes/helm/api.rb @@ -24,6 +24,7 @@ module Gitlab def uninstall(command) namespace.ensure_exists! + create_config_map(command) delete_pod!(command.pod_name) kubeclient.create_pod(command.pod_resource) diff --git a/spec/lib/gitlab/kubernetes/helm/api_spec.rb b/spec/lib/gitlab/kubernetes/helm/api_spec.rb index 24ce397ec3d..0de809833e6 100644 --- a/spec/lib/gitlab/kubernetes/helm/api_spec.rb +++ b/spec/lib/gitlab/kubernetes/helm/api_spec.rb @@ -36,6 +36,8 @@ describe Gitlab::Kubernetes::Helm::Api do describe '#uninstall' do before do allow(client).to receive(:create_pod).and_return(nil) + allow(client).to receive(:get_config_map).and_return(nil) + allow(client).to receive(:create_config_map).and_return(nil) allow(client).to receive(:delete_pod).and_return(nil) allow(namespace).to receive(:ensure_exists!).once end @@ -53,6 +55,28 @@ describe Gitlab::Kubernetes::Helm::Api do subject.uninstall(command) end + + context 'with a ConfigMap' do + let(:resource) { Gitlab::Kubernetes::ConfigMap.new(application_name, files).generate } + + it 'creates a ConfigMap on kubeclient' do + expect(client).to receive(:create_config_map).with(resource).once + + subject.install(command) + end + + context 'config map already exists' do + before do + expect(client).to receive(:get_config_map).with("values-content-configuration-#{application_name}", gitlab_namespace).and_return(resource) + end + + it 'updates the config map' do + expect(client).to receive(:update_config_map).with(resource).once + + subject.install(command) + end + end + end end describe '#install' do -- cgit v1.2.1 From 598fa4cdd8e698d4ed19ad6101989aa286c9131c Mon Sep 17 00:00:00 2001 From: mfluharty Date: Tue, 7 May 2019 13:37:29 -0600 Subject: Default masked to false for new variables Set the default value to false Adjust tests to expect false as the default Update documentation to make new default clear --- app/helpers/ci_variables_helper.rb | 2 +- app/views/ci/variables/_content.html.haml | 2 +- app/views/ci/variables/_variable_row.html.haml | 2 +- .../unreleased/11204-turn-off-mask-by-default.yml | 5 ++++ doc/ci/variables/README.md | 2 +- locale/gitlab.pot | 2 +- .../features/variable_list_shared_examples.rb | 32 +++++++++++++--------- 7 files changed, 29 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/11204-turn-off-mask-by-default.yml diff --git a/app/helpers/ci_variables_helper.rb b/app/helpers/ci_variables_helper.rb index 5bfdeb9e33c..e313015c937 100644 --- a/app/helpers/ci_variables_helper.rb +++ b/app/helpers/ci_variables_helper.rb @@ -17,7 +17,7 @@ module CiVariablesHelper if variable && !only_key_value variable.masked else - true + false end end diff --git a/app/views/ci/variables/_content.html.haml b/app/views/ci/variables/_content.html.haml index d07cbe4589c..0b5c1a806b2 100644 --- a/app/views/ci/variables/_content.html.haml +++ b/app/views/ci/variables/_content.html.haml @@ -1,3 +1,3 @@ -= _('Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they will be masked by default so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want.') += _('Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they can be masked so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want.') = _('You may also add variables that are made available to the running application by prepending the variable key with K8S_SECRET_.').html_safe = link_to _('More information'), help_page_path('ci/variables/README', anchor: 'variables') diff --git a/app/views/ci/variables/_variable_row.html.haml b/app/views/ci/variables/_variable_row.html.haml index ca2521e9bc6..ed4bd5ae19e 100644 --- a/app/views/ci/variables/_variable_row.html.haml +++ b/app/views/ci/variables/_variable_row.html.haml @@ -8,7 +8,7 @@ - value = variable&.value - is_protected_default = ci_variable_protected_by_default? - is_protected = ci_variable_protected?(variable, only_key_value) -- is_masked_default = true +- is_masked_default = false - is_masked = ci_variable_masked?(variable, only_key_value) - id_input_name = "#{form_field}[variables_attributes][][id]" diff --git a/changelogs/unreleased/11204-turn-off-mask-by-default.yml b/changelogs/unreleased/11204-turn-off-mask-by-default.yml new file mode 100644 index 00000000000..5c554e04d45 --- /dev/null +++ b/changelogs/unreleased/11204-turn-off-mask-by-default.yml @@ -0,0 +1,5 @@ +--- +title: Default masked to false for new variables +merge_request: 28186 +author: +type: changed diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 2157a6dc097..3b137297f40 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -86,7 +86,7 @@ Variable types can be set via the [UI](#via-the-ui) or the [API](../../api/proje > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/13784) in GitLab 11.10 -By default, variables will be created as masked variables. +Variables can be created as masked variables. This means that the value of the variable will be hidden in job logs, though it must match certain requirements to do so: diff --git a/locale/gitlab.pot b/locale/gitlab.pot index afce09cd621..c0edb043ea5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3877,7 +3877,7 @@ msgstr "" msgid "Enter the merge request title" msgstr "" -msgid "Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they will be masked by default so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want." +msgid "Environment variables are applied to environments via the runner. They can be protected by only exposing them to protected branches or tags. Additionally, they can be masked so they are hidden in job logs, though they must match certain regexp requirements to do so. You can use environment variables for passwords, secret keys, or whatever you want." msgstr "" msgid "Environment variables are configured by your administrator to be %{link_start}protected%{link_end} by default" diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 92a19dd22a2..01531864c1f 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -45,12 +45,12 @@ shared_examples 'variable list' do end end - it 'defaults to masked' do + it 'defaults to unmasked' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('key') find('.js-ci-variable-input-value').set('key_value') - expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') end click_button('Save variables') @@ -62,7 +62,7 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do expect(find('.js-ci-variable-input-key').value).to eq('key') expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') - expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') end end @@ -234,12 +234,14 @@ shared_examples 'variable list' do end it 'edits variable to be unmasked' do - page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do - expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('unmasked_key') + find('.js-ci-variable-input-value').set('unmasked_value') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') find('.ci-variable-masked-item .js-project-feature-toggle').click - expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') end click_button('Save variables') @@ -247,12 +249,6 @@ shared_examples 'variable list' do visit page_path - page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do - expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') - end - end - - it 'edits variable to be masked' do page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('true') @@ -268,6 +264,14 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') + end + end + + it 'edits variable to be masked' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('masked_key') + find('.js-ci-variable-input-value').set('masked_value') + expect(find('.js-ci-variable-input-masked', visible: false).value).to eq('false') find('.ci-variable-masked-item .js-project-feature-toggle').click @@ -348,10 +352,11 @@ shared_examples 'variable list' do end end - it 'shows validation error box about empty values' do + it 'shows validation error box about masking empty values' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('empty_value') find('.js-ci-variable-input-value').set('') + find('.ci-variable-masked-item .js-project-feature-toggle').click end click_button('Save variables') @@ -367,6 +372,7 @@ shared_examples 'variable list' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('unmaskable_value') find('.js-ci-variable-input-value').set('???') + find('.ci-variable-masked-item .js-project-feature-toggle').click end click_button('Save variables') -- cgit v1.2.1 From 59914a856be4e8b19da7a8a848183a572e563af2 Mon Sep 17 00:00:00 2001 From: Itzik Gan Baruch Date: Tue, 4 Jun 2019 15:53:14 +0000 Subject: Added a link to the click-through demo --- doc/ci/multi_project_pipelines.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/ci/multi_project_pipelines.md b/doc/ci/multi_project_pipelines.md index bcd92243d97..57825982ede 100644 --- a/doc/ci/multi_project_pipelines.md +++ b/doc/ci/multi_project_pipelines.md @@ -166,6 +166,10 @@ In this scenario, the `UPSTREAM_BRANCH` variable with a value related to the upstream pipeline will be passed to a `downstream` job, and will be available within the context of all downstream builds. +### Demos + +[A click-through demo of cross-project pipeline is available](https://about.gitlab.com/handbook/marketing/product-marketing/demo/#cross-project-pipeline-triggering-and-visualization-may-2019---1110), demonstrates how cross-functional dev teams use cross-pipeline triggering to trigger multiple pipelines for different microservices projects. + ### Limitations Because bridge jobs are a little different to regular jobs, it is not -- cgit v1.2.1 From 3f1f12645d3308f0532699f0f632f5b235a7ec34 Mon Sep 17 00:00:00 2001 From: Tim Rizzi Date: Tue, 4 Jun 2019 15:59:31 +0000 Subject: Update maven template with correct link --- lib/gitlab/ci/templates/Maven.gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/templates/Maven.gitlab-ci.yml b/lib/gitlab/ci/templates/Maven.gitlab-ci.yml index 08dc74e041a..13ab98d3a16 100644 --- a/lib/gitlab/ci/templates/Maven.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Maven.gitlab-ci.yml @@ -42,14 +42,14 @@ verify:jdk8: <<: *verify # To deploy packages from CI, create a ci_settings.xml file -# For deploying packages to GitLab's Maven Repository: See https://gitlab.com/help/user/project/packages/maven_repository.md#creating-maven-packages-with-gitlab-cicd for more details. +# For deploying packages to GitLab's Maven Repository: See https://docs.gitlab.com/ee/user/project/packages/maven_repository.html#creating-maven-packages-with-gitlab-cicd for more details. # Please note: The GitLab Maven Repository is currently only available in GitLab Premium / Ultimate. # For `master` branch run `mvn deploy` automatically. deploy:jdk8: stage: deploy script: - if [ ! -f ci_settings.xml ]; - then echo "CI settings missing\! If deploying to GitLab Maven Repository, please see https://gitlab.com/help/user/project/packages/maven_repository.md#creating-maven-packages-with-gitlab-cicd for instructions."; + then echo "CI settings missing\! If deploying to GitLab Maven Repository, please see https://docs.gitlab.com/ee/user/project/packages/maven_repository.html#creating-maven-packages-with-gitlab-cicd for instructions."; fi - 'mvn $MAVEN_CLI_OPTS deploy -s ci_settings.xml' only: -- cgit v1.2.1 From e02cd451b2d6a81f4847f26b29ed1eb6c74f8dab Mon Sep 17 00:00:00 2001 From: Jose Vargas Date: Thu, 30 May 2019 13:47:32 -0500 Subject: Add single_stat chart component to the monitoring bundle This merge requests just adds the component without integrating it to the dashboard, it adds tests as well --- .../monitoring/components/charts/single_stat.vue | 37 ++++++++++++++++++++++ .../monitoring/charts/single_stat_spec.js | 28 ++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 app/assets/javascripts/monitoring/components/charts/single_stat.vue create mode 100644 spec/javascripts/monitoring/charts/single_stat_spec.js diff --git a/app/assets/javascripts/monitoring/components/charts/single_stat.vue b/app/assets/javascripts/monitoring/components/charts/single_stat.vue new file mode 100644 index 00000000000..b03a6ca1806 --- /dev/null +++ b/app/assets/javascripts/monitoring/components/charts/single_stat.vue @@ -0,0 +1,37 @@ + + diff --git a/spec/javascripts/monitoring/charts/single_stat_spec.js b/spec/javascripts/monitoring/charts/single_stat_spec.js new file mode 100644 index 00000000000..12b73002f97 --- /dev/null +++ b/spec/javascripts/monitoring/charts/single_stat_spec.js @@ -0,0 +1,28 @@ +import { shallowMount } from '@vue/test-utils'; +import SingleStatChart from '~/monitoring/components/charts/single_stat.vue'; + +describe('Single Stat Chart component', () => { + let singleStatChart; + + beforeEach(() => { + singleStatChart = shallowMount(SingleStatChart, { + propsData: { + title: 'Time to render', + value: 1, + unit: 'sec', + }, + }); + }); + + afterEach(() => { + singleStatChart.destroy(); + }); + + describe('computed', () => { + describe('valueWithUnit', () => { + it('should interpolate the value and unit props', () => { + expect(singleStatChart.vm.valueWithUnit).toBe('1sec'); + }); + }); + }); +}); -- cgit v1.2.1 From e5ed663e7c0e5282abaa58f0bce4470aab6c3057 Mon Sep 17 00:00:00 2001 From: PatOnTheBack Date: Tue, 4 Jun 2019 20:23:11 +0000 Subject: Update code_quality.md to fix grammatical errors. --- doc/user/project/merge_requests/code_quality.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user/project/merge_requests/code_quality.md b/doc/user/project/merge_requests/code_quality.md index e6811b5df5e..705ff333579 100644 --- a/doc/user/project/merge_requests/code_quality.md +++ b/doc/user/project/merge_requests/code_quality.md @@ -19,7 +19,7 @@ in the merge request widget area: For instance, consider the following workflow: -1. Your backend team member starts a new implementation for making certain feature in your app faster +1. Your backend team member starts a new implementation for making a certain feature in your app faster 1. With Code Quality reports, they analyze how their implementation is impacting the code quality 1. The metrics show that their code degrade the quality in 10 points 1. You ask a co-worker to help them with this modification @@ -63,7 +63,7 @@ Example: NOTE: **Note:** Although the Code Climate spec supports more properties, those are ignored by GitLab. -For more information on how the Code Quality job should look like, check the +For more information on what the Code Quality job should look like, check the example on [analyzing a project's code quality](../../../ci/examples/code_quality.md). GitLab then checks this report, compares the metrics between the source and target -- cgit v1.2.1 From 977ba4cc150092107944f3a46734b2540d321dcf Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 4 Jun 2019 14:44:02 +1000 Subject: Ensure DB is writable before continuing jobs In the context of a Geo setup, some jobs can be running on a Geo secondary where the database is read-only and therefore we should guard against various jobs attempting to write. --- app/workers/pages_domain_verification_cron_worker.rb | 2 ++ app/workers/pages_domain_verification_worker.rb | 2 ++ spec/workers/pages_domain_verification_cron_worker_spec.rb | 7 +++++++ spec/workers/pages_domain_verification_worker_spec.rb | 7 +++++++ 4 files changed, 18 insertions(+) diff --git a/app/workers/pages_domain_verification_cron_worker.rb b/app/workers/pages_domain_verification_cron_worker.rb index 92d62a15aee..60703c83e9e 100644 --- a/app/workers/pages_domain_verification_cron_worker.rb +++ b/app/workers/pages_domain_verification_cron_worker.rb @@ -5,6 +5,8 @@ class PagesDomainVerificationCronWorker include CronjobQueue def perform + return if Gitlab::Database.read_only? + PagesDomain.needs_verification.find_each do |domain| PagesDomainVerificationWorker.perform_async(domain.id) end diff --git a/app/workers/pages_domain_verification_worker.rb b/app/workers/pages_domain_verification_worker.rb index b3319ff5a13..7817b2ee5fc 100644 --- a/app/workers/pages_domain_verification_worker.rb +++ b/app/workers/pages_domain_verification_worker.rb @@ -5,6 +5,8 @@ class PagesDomainVerificationWorker # rubocop: disable CodeReuse/ActiveRecord def perform(domain_id) + return if Gitlab::Database.read_only? + domain = PagesDomain.find_by(id: domain_id) return unless domain diff --git a/spec/workers/pages_domain_verification_cron_worker_spec.rb b/spec/workers/pages_domain_verification_cron_worker_spec.rb index 186824a444f..3fb86adee11 100644 --- a/spec/workers/pages_domain_verification_cron_worker_spec.rb +++ b/spec/workers/pages_domain_verification_cron_worker_spec.rb @@ -10,6 +10,13 @@ describe PagesDomainVerificationCronWorker do let!(:reverify) { create(:pages_domain, :reverify) } let!(:disabled) { create(:pages_domain, :disabled) } + it 'does nothing if the database is read-only' do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + expect(PagesDomainVerificationWorker).not_to receive(:perform_async).with(reverify.id) + + worker.perform + end + it 'enqueues a PagesDomainVerificationWorker for domains needing verification' do [reverify, disabled].each do |domain| expect(PagesDomainVerificationWorker).to receive(:perform_async).with(domain.id) diff --git a/spec/workers/pages_domain_verification_worker_spec.rb b/spec/workers/pages_domain_verification_worker_spec.rb index 2f23dcb487f..f51ac1f4323 100644 --- a/spec/workers/pages_domain_verification_worker_spec.rb +++ b/spec/workers/pages_domain_verification_worker_spec.rb @@ -8,6 +8,13 @@ describe PagesDomainVerificationWorker do let(:domain) { create(:pages_domain) } describe '#perform' do + it 'does nothing if the database is read-only' do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + expect(PagesDomain).not_to receive(:find_by).with(id: domain.id) + + worker.perform(domain.id) + end + it 'does nothing for a non-existent domain' do domain.destroy -- cgit v1.2.1 From cfaac7532210ef1ce03f335a3198bb7d2ad3979a Mon Sep 17 00:00:00 2001 From: drew cimino Date: Fri, 26 Apr 2019 11:37:20 -0400 Subject: && and || operators for CI Pipeline expressions. Refactored regex pattern matching to eagerly return tokens Packaged behind a default-enabled feature flag and added operator documentation. --- changelogs/unreleased/ci-variable-conjunction.yml | 5 + doc/ci/variables/README.md | 18 ++ lib/gitlab/ci/pipeline/expression/lexeme/and.rb | 27 +++ lib/gitlab/ci/pipeline/expression/lexeme/base.rb | 6 +- lib/gitlab/ci/pipeline/expression/lexeme/equals.rb | 9 +- .../ci/pipeline/expression/lexeme/matches.rb | 25 ++- .../ci/pipeline/expression/lexeme/not_equals.rb | 9 +- .../ci/pipeline/expression/lexeme/not_matches.rb | 9 +- .../ci/pipeline/expression/lexeme/operator.rb | 20 ++ lib/gitlab/ci/pipeline/expression/lexeme/or.rb | 27 +++ .../ci/pipeline/expression/lexeme/pattern.rb | 13 +- lib/gitlab/ci/pipeline/expression/lexer.rb | 19 +- lib/gitlab/ci/pipeline/expression/parser.rb | 70 +++++- lib/gitlab/ci/pipeline/expression/statement.rb | 26 +-- .../ci/pipeline/expression/lexeme/and_spec.rb | 77 +++++++ .../ci/pipeline/expression/lexeme/equals_spec.rb | 55 +++-- .../ci/pipeline/expression/lexeme/matches_spec.rb | 120 +++++++---- .../pipeline/expression/lexeme/not_equals_spec.rb | 60 ++++-- .../pipeline/expression/lexeme/not_matches_spec.rb | 120 +++++++---- .../ci/pipeline/expression/lexeme/or_spec.rb | 77 +++++++ .../ci/pipeline/expression/lexeme/pattern_spec.rb | 94 ++++++-- .../gitlab/ci/pipeline/expression/lexer_spec.rb | 50 +++++ .../gitlab/ci/pipeline/expression/parser_spec.rb | 48 ++++- .../ci/pipeline/expression/statement_spec.rb | 237 +++++++++++++-------- 24 files changed, 957 insertions(+), 264 deletions(-) create mode 100644 changelogs/unreleased/ci-variable-conjunction.yml create mode 100644 lib/gitlab/ci/pipeline/expression/lexeme/and.rb create mode 100644 lib/gitlab/ci/pipeline/expression/lexeme/or.rb create mode 100644 spec/lib/gitlab/ci/pipeline/expression/lexeme/and_spec.rb create mode 100644 spec/lib/gitlab/ci/pipeline/expression/lexeme/or_spec.rb diff --git a/changelogs/unreleased/ci-variable-conjunction.yml b/changelogs/unreleased/ci-variable-conjunction.yml new file mode 100644 index 00000000000..839c4285f3a --- /dev/null +++ b/changelogs/unreleased/ci-variable-conjunction.yml @@ -0,0 +1,5 @@ +--- +title: Add support for && and || to CI Pipeline Expressions. Change CI variable expression matching for Lexeme::Pattern to eagerly return tokens. +merge_request: 27925 +author: Martin Manelli +type: added diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 2157a6dc097..ac3066dbb2a 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -490,6 +490,7 @@ Below you can find supported syntax reference: 1. Equality matching using a string > Example: `$VARIABLE == "some value"` + > Example: `$VARIABLE != "some value"` _(added in 11.11)_ You can use equality operator `==` or `!=` to compare a variable content to a @@ -500,6 +501,7 @@ Below you can find supported syntax reference: 1. Checking for an undefined value > Example: `$VARIABLE == null` + > Example: `$VARIABLE != null` _(added in 11.11)_ It sometimes happens that you want to check whether a variable is defined @@ -510,6 +512,7 @@ Below you can find supported syntax reference: 1. Checking for an empty variable > Example: `$VARIABLE == ""` + > Example: `$VARIABLE != ""` _(added in 11.11)_ If you want to check whether a variable is defined, but is empty, you can @@ -519,6 +522,7 @@ Below you can find supported syntax reference: 1. Comparing two variables > Example: `$VARIABLE_1 == $VARIABLE_2` + > Example: `$VARIABLE_1 != $VARIABLE_2` _(added in 11.11)_ It is possible to compare two variables. This is going to compare values @@ -538,6 +542,7 @@ Below you can find supported syntax reference: 1. Pattern matching _(added in 11.0)_ > Example: `$VARIABLE =~ /^content.*/` + > Example: `$VARIABLE_1 !~ /^content.*/` _(added in 11.11)_ It is possible perform pattern matching against a variable and regular @@ -547,6 +552,19 @@ Below you can find supported syntax reference: Pattern matching is case-sensitive by default. Use `i` flag modifier, like `/pattern/i` to make a pattern case-insensitive. +1. Conjunction / Disjunction + + > Example: `$VARIABLE1 =~ /^content.*/ && $VARIABLE2 == "something"` + + > Example: `$VARIABLE1 =~ /^content.*/ && $VARIABLE2 =~ /thing$/ && $VARIABLE3` + + > Example: `$VARIABLE1 =~ /^content.*/ || $VARIABLE2 =~ /thing$/ && $VARIABLE3` + + It is possible to join multiple conditions using `&&` or `||`. Any of the otherwise + supported syntax may be used in a conjunctive or disjunctive statement. + Precedence of operators follows standard Ruby 2.5 operation + [precedence](https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html). + ## Debug tracing > Introduced in GitLab Runner 1.7. diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/and.rb b/lib/gitlab/ci/pipeline/expression/lexeme/and.rb new file mode 100644 index 00000000000..54a0e2ad9dd --- /dev/null +++ b/lib/gitlab/ci/pipeline/expression/lexeme/and.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Pipeline + module Expression + module Lexeme + class And < Lexeme::Operator + PATTERN = /&&/.freeze + + def evaluate(variables = {}) + @left.evaluate(variables) && @right.evaluate(variables) + end + + def self.build(_value, behind, ahead) + new(behind, ahead) + end + + def self.precedence + 11 # See: https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/base.rb b/lib/gitlab/ci/pipeline/expression/lexeme/base.rb index 70c774416f6..7ebd2e25398 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/base.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/base.rb @@ -15,10 +15,14 @@ module Gitlab end def self.scan(scanner) - if scanner.scan(self::PATTERN) + if scanner.scan(pattern) Expression::Token.new(scanner.matched, self) end end + + def self.pattern + self::PATTERN + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/equals.rb b/lib/gitlab/ci/pipeline/expression/lexeme/equals.rb index 668e85f5b9e..62f4c14f597 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/equals.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/equals.rb @@ -8,11 +8,6 @@ module Gitlab class Equals < Lexeme::Operator PATTERN = /==/.freeze - def initialize(left, right) - @left = left - @right = right - end - def evaluate(variables = {}) @left.evaluate(variables) == @right.evaluate(variables) end @@ -20,6 +15,10 @@ module Gitlab def self.build(_value, behind, ahead) new(behind, ahead) end + + def self.precedence + 10 # See: https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb index cd17bc4d78b..ecfab627226 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb @@ -8,21 +8,36 @@ module Gitlab class Matches < Lexeme::Operator PATTERN = /=~/.freeze - def initialize(left, right) - @left = left - @right = right - end - def evaluate(variables = {}) text = @left.evaluate(variables) regexp = @right.evaluate(variables) regexp.scan(text.to_s).any? + + if ci_variables_complex_expressions? + # return offset of first match, or nil if no matches + if match = regexp.scan(text.to_s).first + text.to_s.index(match) + end + else + # return true or false + regexp.scan(text.to_s).any? + end end def self.build(_value, behind, ahead) new(behind, ahead) end + + def self.precedence + 10 # See: https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html + end + + private + + def ci_variables_complex_expressions? + Feature.enabled?(:ci_variables_complex_expressions, default_enabled: true) + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/not_equals.rb b/lib/gitlab/ci/pipeline/expression/lexeme/not_equals.rb index 5fcc9406cc8..8166bcd5730 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/not_equals.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/not_equals.rb @@ -8,11 +8,6 @@ module Gitlab class NotEquals < Lexeme::Operator PATTERN = /!=/.freeze - def initialize(left, right) - @left = left - @right = right - end - def evaluate(variables = {}) @left.evaluate(variables) != @right.evaluate(variables) end @@ -20,6 +15,10 @@ module Gitlab def self.build(_value, behind, ahead) new(behind, ahead) end + + def self.precedence + 10 # See: https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb index 14544d33e25..831c27fa0ea 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb @@ -8,11 +8,6 @@ module Gitlab class NotMatches < Lexeme::Operator PATTERN = /\!~/.freeze - def initialize(left, right) - @left = left - @right = right - end - def evaluate(variables = {}) text = @left.evaluate(variables) regexp = @right.evaluate(variables) @@ -23,6 +18,10 @@ module Gitlab def self.build(_value, behind, ahead) new(behind, ahead) end + + def self.precedence + 10 # See: https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/operator.rb b/lib/gitlab/ci/pipeline/expression/lexeme/operator.rb index 3ebceb92eb7..3ddab7800c8 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/operator.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/operator.rb @@ -6,9 +6,29 @@ module Gitlab module Expression module Lexeme class Operator < Lexeme::Base + # This operator class is design to handle single operators that take two + # arguments. Expression::Parser was originally designed to read infix operators, + # and so the two operands are called "left" and "right" here. If we wish to + # implement an Operator that takes a greater or lesser number of arguments, a + # structural change or additional Operator superclass will likely be needed. + + OperatorError = Class.new(Expression::ExpressionError) + + def initialize(left, right) + raise OperatorError, 'Invalid left operand' unless left.respond_to? :evaluate + raise OperatorError, 'Invalid right operand' unless right.respond_to? :evaluate + + @left = left + @right = right + end + def self.type :operator end + + def self.precedence + raise NotImplementedError + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/or.rb b/lib/gitlab/ci/pipeline/expression/lexeme/or.rb new file mode 100644 index 00000000000..807876f905a --- /dev/null +++ b/lib/gitlab/ci/pipeline/expression/lexeme/or.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Pipeline + module Expression + module Lexeme + class Or < Lexeme::Operator + PATTERN = /\|\|/.freeze + + def evaluate(variables = {}) + @left.evaluate(variables) || @right.evaluate(variables) + end + + def self.build(_value, behind, ahead) + new(behind, ahead) + end + + def self.precedence + 12 # See: https://ruby-doc.org/core-2.5.0/doc/syntax/precedence_rdoc.html + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb index 2b719c9c6fc..e4cf360a1c1 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb @@ -8,10 +8,11 @@ module Gitlab require_dependency 're2' class Pattern < Lexeme::Value - PATTERN = %r{^/.+/[ismU]*$}.freeze + PATTERN = %r{^/.+/[ismU]*$}.freeze + NEW_PATTERN = %r{^\/([^\/]|\\/)+[^\\]\/[ismU]*}.freeze def initialize(regexp) - @value = regexp + @value = self.class.eager_matching_with_escape_characters? ? regexp.gsub(/\\\//, '/') : regexp unless Gitlab::UntrustedRegexp::RubySyntax.valid?(@value) raise Lexer::SyntaxError, 'Invalid regular expression!' @@ -24,9 +25,17 @@ module Gitlab raise Expression::RuntimeError, 'Invalid regular expression!' end + def self.pattern + eager_matching_with_escape_characters? ? NEW_PATTERN : PATTERN + end + def self.build(string) new(string) end + + def self.eager_matching_with_escape_characters? + Feature.enabled?(:ci_variables_complex_expressions, default_enabled: true) + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexer.rb b/lib/gitlab/ci/pipeline/expression/lexer.rb index e14edfae51d..22c210ae26b 100644 --- a/lib/gitlab/ci/pipeline/expression/lexer.rb +++ b/lib/gitlab/ci/pipeline/expression/lexer.rb @@ -20,6 +20,19 @@ module Gitlab Expression::Lexeme::NotMatches ].freeze + NEW_LEXEMES = [ + Expression::Lexeme::Variable, + Expression::Lexeme::String, + Expression::Lexeme::Pattern, + Expression::Lexeme::Null, + Expression::Lexeme::Equals, + Expression::Lexeme::Matches, + Expression::Lexeme::NotEquals, + Expression::Lexeme::NotMatches, + Expression::Lexeme::And, + Expression::Lexeme::Or + ].freeze + MAX_TOKENS = 100 def initialize(statement, max_tokens: MAX_TOKENS) @@ -45,7 +58,7 @@ module Gitlab return tokens if @scanner.eos? - lexeme = LEXEMES.find do |type| + lexeme = available_lexemes.find do |type| type.scan(@scanner).tap do |token| tokens.push(token) if token.present? end @@ -58,6 +71,10 @@ module Gitlab raise Lexer::SyntaxError, 'Too many tokens!' end + + def available_lexemes + Feature.enabled?(:ci_variables_complex_expressions, default_enabled: true) ? NEW_LEXEMES : LEXEMES + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/parser.rb b/lib/gitlab/ci/pipeline/expression/parser.rb index ed184309ab4..589bf32a4d7 100644 --- a/lib/gitlab/ci/pipeline/expression/parser.rb +++ b/lib/gitlab/ci/pipeline/expression/parser.rb @@ -5,17 +5,30 @@ module Gitlab module Pipeline module Expression class Parser + ParseError = Class.new(Expression::ExpressionError) + def initialize(tokens) @tokens = tokens.to_enum @nodes = [] end - ## - # This produces a reverse descent parse tree. - # - # It currently does not support precedence of operators. - # def tree + if Feature.enabled?(:ci_variables_complex_expressions, default_enabled: true) + rpn_parse_tree + else + reverse_descent_parse_tree + end + end + + def self.seed(statement) + new(Expression::Lexer.new(statement).tokens) + end + + private + + # This produces a reverse descent parse tree. + # It does not support precedence of operators. + def reverse_descent_parse_tree while token = @tokens.next case token.type when :operator @@ -32,8 +45,51 @@ module Gitlab @nodes.last || Lexeme::Null.new end - def self.seed(statement) - new(Expression::Lexer.new(statement).tokens) + def rpn_parse_tree + results = [] + + tokens_rpn.each do |token| + case token.type + when :value + results.push(token.build) + when :operator + right_operand = results.pop + left_operand = results.pop + + token.build(left_operand, right_operand).tap do |res| + results.push(res) + end + else + raise ParseError, 'Unprocessable token found in parse tree' + end + end + + raise ParseError, 'Unreachable nodes in parse tree' if results.count > 1 + raise ParseError, 'Empty parse tree' if results.count < 1 + + results.pop + end + + # Parse the expression into Reverse Polish Notation + # (See: Shunting-yard algorithm) + def tokens_rpn + output = [] + operators = [] + + @tokens.each do |token| + case token.type + when :value + output.push(token) + when :operator + if operators.any? && token.lexeme.precedence >= operators.last.lexeme.precedence + output.push(operators.pop) + end + + operators.push(token) + end + end + + output.concat(operators.reverse) end end end diff --git a/lib/gitlab/ci/pipeline/expression/statement.rb b/lib/gitlab/ci/pipeline/expression/statement.rb index ab5ae9caeea..0e81e1bd34c 100644 --- a/lib/gitlab/ci/pipeline/expression/statement.rb +++ b/lib/gitlab/ci/pipeline/expression/statement.rb @@ -7,27 +7,6 @@ module Gitlab class Statement StatementError = Class.new(Expression::ExpressionError) - GRAMMAR = [ - # presence matchers - %w[variable], - - # positive matchers - %w[variable equals string], - %w[variable equals variable], - %w[variable equals null], - %w[string equals variable], - %w[null equals variable], - %w[variable matches pattern], - - # negative matchers - %w[variable notequals string], - %w[variable notequals variable], - %w[variable notequals null], - %w[string notequals variable], - %w[null notequals variable], - %w[variable notmatches pattern] - ].freeze - def initialize(statement, variables = {}) @lexer = Expression::Lexer.new(statement) @variables = variables.with_indifferent_access @@ -36,10 +15,6 @@ module Gitlab def parse_tree raise StatementError if @lexer.lexemes.empty? - unless GRAMMAR.find { |syntax| syntax == @lexer.lexemes } - raise StatementError, 'Unknown pipeline expression!' - end - Expression::Parser.new(@lexer.tokens).tree end @@ -54,6 +29,7 @@ module Gitlab end def valid? + evaluate parse_tree.is_a?(Lexeme::Base) rescue Expression::ExpressionError false diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/and_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/and_spec.rb new file mode 100644 index 00000000000..006ce4d8078 --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/and_spec.rb @@ -0,0 +1,77 @@ +require 'fast_spec_helper' +require 'rspec-parameterized' + +describe Gitlab::Ci::Pipeline::Expression::Lexeme::And do + let(:left) { double('left', evaluate: nil) } + let(:right) { double('right', evaluate: nil) } + + describe '.build' do + it 'creates a new instance of the token' do + expect(described_class.build('&&', left, right)).to be_a(described_class) + end + + context 'with non-evaluable operands' do + let(:left) { double('left') } + let(:right) { double('right') } + + it 'raises an operator error' do + expect { described_class.build('&&', left, right) }.to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + end + + describe '.type' do + it 'is an operator' do + expect(described_class.type).to eq :operator + end + end + + describe '.precedence' do + it 'has a precedence' do + expect(described_class.precedence).to be_an Integer + end + end + + describe '#evaluate' do + let(:operator) { described_class.new(left, right) } + + subject { operator.evaluate } + + before do + allow(left).to receive(:evaluate).and_return(left_value) + allow(right).to receive(:evaluate).and_return(right_value) + end + + context 'when left and right are truthy' do + where(:left_value, :right_value) do + [true, 1, 'a'].permutation(2).to_a + end + + with_them do + it { is_expected.to be_truthy } + it { is_expected.to eq(right_value) } + end + end + + context 'when left or right is falsey' do + where(:left_value, :right_value) do + [true, false, nil].permutation(2).to_a + end + + with_them do + it { is_expected.to be_falsey } + end + end + + context 'when left and right are falsey' do + where(:left_value, :right_value) do + [false, nil].permutation(2).to_a + end + + with_them do + it { is_expected.to be_falsey } + it { is_expected.to eq(left_value) } + end + end + end +end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/equals_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/equals_spec.rb index 019a2ed184d..fcbd2863289 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/equals_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/equals_spec.rb @@ -5,9 +5,21 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Equals do let(:right) { double('right') } describe '.build' do - it 'creates a new instance of the token' do - expect(described_class.build('==', left, right)) - .to be_a(described_class) + context 'with non-evaluable operands' do + it 'creates a new instance of the token' do + expect { described_class.build('==', left, right) } + .to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + + context 'with evaluable operands' do + it 'creates a new instance of the token' do + allow(left).to receive(:evaluate).and_return('my-string') + allow(right).to receive(:evaluate).and_return('my-string') + + expect(described_class.build('==', left, right)) + .to be_a(described_class) + end end end @@ -17,23 +29,40 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Equals do end end + describe '.precedence' do + it 'has a precedence' do + expect(described_class.precedence).to be_an Integer + end + end + describe '#evaluate' do - it 'returns false when left and right are not equal' do - allow(left).to receive(:evaluate).and_return(1) - allow(right).to receive(:evaluate).and_return(2) + let(:operator) { described_class.new(left, right) } - operator = described_class.new(left, right) + subject { operator.evaluate } - expect(operator.evaluate(VARIABLE: 3)).to eq false + before do + allow(left).to receive(:evaluate).and_return(left_value) + allow(right).to receive(:evaluate).and_return(right_value) end - it 'returns true when left and right are equal' do - allow(left).to receive(:evaluate).and_return(1) - allow(right).to receive(:evaluate).and_return(1) + context 'when left and right are equal' do + where(:left_value, :right_value) do + [%w(string string)] + end + + with_them do + it { is_expected.to eq(true) } + end + end - operator = described_class.new(left, right) + context 'when left and right are not equal' do + where(:left_value, :right_value) do + ['one string', 'two string'].permutation(2).to_a + end - expect(operator.evaluate(VARIABLE: 3)).to eq true + with_them do + it { is_expected.to eq(false) } + end end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb index 49e5af52f4d..97da66d2bcc 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb @@ -6,9 +6,21 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Matches do let(:right) { double('right') } describe '.build' do - it 'creates a new instance of the token' do - expect(described_class.build('=~', left, right)) - .to be_a(described_class) + context 'with non-evaluable operands' do + it 'creates a new instance of the token' do + expect { described_class.build('=~', left, right) } + .to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + + context 'with evaluable operands' do + it 'creates a new instance of the token' do + allow(left).to receive(:evaluate).and_return('my-string') + allow(right).to receive(:evaluate).and_return('/my-string/') + + expect(described_class.build('=~', left, right)) + .to be_a(described_class) + end end end @@ -18,63 +30,93 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Matches do end end + describe '.precedence' do + it 'has a precedence' do + expect(described_class.precedence).to be_an Integer + end + end + describe '#evaluate' do - it 'returns false when left and right do not match' do - allow(left).to receive(:evaluate).and_return('my-string') - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('something')) + let(:operator) { described_class.new(left, right) } - operator = described_class.new(left, right) + subject { operator.evaluate } - expect(operator.evaluate).to eq false + before do + allow(left).to receive(:evaluate).and_return(left_value) + allow(right).to receive(:evaluate).and_return(right_value) end - it 'returns true when left and right match' do - allow(left).to receive(:evaluate).and_return('my-awesome-string') - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('awesome.string$')) + context 'when left and right do not match' do + let(:left_value) { 'my-string' } + let(:right_value) { Gitlab::UntrustedRegexp.new('something') } - operator = described_class.new(left, right) - - expect(operator.evaluate).to eq true + it { is_expected.to eq(nil) } end - it 'supports matching against a nil value' do - allow(left).to receive(:evaluate).and_return(nil) - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('pattern')) + context 'when left and right match' do + let(:left_value) { 'my-awesome-string' } + let(:right_value) { Gitlab::UntrustedRegexp.new('awesome.string$') } + + it { is_expected.to eq(3) } + end - operator = described_class.new(left, right) + context 'when left is nil' do + let(:left_value) { nil } + let(:right_value) { Gitlab::UntrustedRegexp.new('pattern') } - expect(operator.evaluate).to eq false + it { is_expected.to eq(nil) } end - it 'supports multiline strings' do - allow(left).to receive(:evaluate).and_return <<~TEXT - My awesome contents + context 'when left is a multiline string and matches right' do + let(:left_value) do + <<~TEXT + My awesome contents + + My-text-string! + TEXT + end + + let(:right_value) { Gitlab::UntrustedRegexp.new('text-string') } + + it { is_expected.to eq(24) } + end - My-text-string! - TEXT + context 'when left is a multiline string and does not match right' do + let(:left_value) do + <<~TEXT + My awesome contents - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('text-string')) + My-terrible-string! + TEXT + end - operator = described_class.new(left, right) + let(:right_value) { Gitlab::UntrustedRegexp.new('text-string') } - expect(operator.evaluate).to eq true + it { is_expected.to eq(nil) } end - it 'supports regexp flags' do - allow(left).to receive(:evaluate).and_return <<~TEXT - My AWESOME content - TEXT + context 'when a matching pattern uses regex flags' do + let(:left_value) do + <<~TEXT + My AWESOME content + TEXT + end + + let(:right_value) { Gitlab::UntrustedRegexp.new('(?i)awesome') } + + it { is_expected.to eq(3) } + end - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('(?i)awesome')) + context 'when a non-matching pattern uses regex flags' do + let(:left_value) do + <<~TEXT + My AWESOME content + TEXT + end - operator = described_class.new(left, right) + let(:right_value) { Gitlab::UntrustedRegexp.new('(?i)terrible') } - expect(operator.evaluate).to eq true + it { is_expected.to eq(nil) } end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_equals_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_equals_spec.rb index 9aa2f4efd67..38d30c9035a 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_equals_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_equals_spec.rb @@ -5,9 +5,21 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::NotEquals do let(:right) { double('right') } describe '.build' do - it 'creates a new instance of the token' do - expect(described_class.build('!=', left, right)) - .to be_a(described_class) + context 'with non-evaluable operands' do + it 'creates a new instance of the token' do + expect { described_class.build('!=', left, right) } + .to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + + context 'with evaluable operands' do + it 'creates a new instance of the token' do + allow(left).to receive(:evaluate).and_return('my-string') + allow(right).to receive(:evaluate).and_return('my-string') + + expect(described_class.build('!=', left, right)) + .to be_a(described_class) + end end end @@ -17,23 +29,45 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::NotEquals do end end + describe '.precedence' do + it 'has a precedence' do + expect(described_class.precedence).to be_an Integer + end + end + describe '#evaluate' do - it 'returns true when left and right are not equal' do - allow(left).to receive(:evaluate).and_return(1) - allow(right).to receive(:evaluate).and_return(2) + let(:operator) { described_class.new(left, right) } - operator = described_class.new(left, right) + subject { operator.evaluate } - expect(operator.evaluate(VARIABLE: 3)).to eq true + before do + allow(left).to receive(:evaluate).and_return(left_value) + allow(right).to receive(:evaluate).and_return(right_value) end - it 'returns false when left and right are equal' do - allow(left).to receive(:evaluate).and_return(1) - allow(right).to receive(:evaluate).and_return(1) + context 'when left and right are equal' do + using RSpec::Parameterized::TableSyntax + + where(:left_value, :right_value) do + 'string' | 'string' + 1 | 1 + '' | '' + nil | nil + end + + with_them do + it { is_expected.to eq(false) } + end + end - operator = described_class.new(left, right) + context 'when left and right are not equal' do + where(:left_value, :right_value) do + ['one string', 'two string', 1, 2, '', nil, false, true].permutation(2).to_a + end - expect(operator.evaluate(VARIABLE: 3)).to eq false + with_them do + it { is_expected.to eq(true) } + end end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb index fa3b9651fb4..99110ff8d88 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb @@ -6,9 +6,21 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::NotMatches do let(:right) { double('right') } describe '.build' do - it 'creates a new instance of the token' do - expect(described_class.build('!~', left, right)) - .to be_a(described_class) + context 'with non-evaluable operands' do + it 'creates a new instance of the token' do + expect { described_class.build('!~', left, right) } + .to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + + context 'with evaluable operands' do + it 'creates a new instance of the token' do + allow(left).to receive(:evaluate).and_return('my-string') + allow(right).to receive(:evaluate).and_return('my-string') + + expect(described_class.build('!~', left, right)) + .to be_a(described_class) + end end end @@ -18,63 +30,93 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::NotMatches do end end + describe '.precedence' do + it 'has a precedence' do + expect(described_class.precedence).to be_an Integer + end + end + describe '#evaluate' do - it 'returns true when left and right do not match' do - allow(left).to receive(:evaluate).and_return('my-string') - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('something')) + let(:operator) { described_class.new(left, right) } - operator = described_class.new(left, right) + subject { operator.evaluate } - expect(operator.evaluate).to eq true + before do + allow(left).to receive(:evaluate).and_return(left_value) + allow(right).to receive(:evaluate).and_return(right_value) end - it 'returns false when left and right match' do - allow(left).to receive(:evaluate).and_return('my-awesome-string') - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('awesome.string$')) + context 'when left and right do not match' do + let(:left_value) { 'my-string' } + let(:right_value) { Gitlab::UntrustedRegexp.new('something') } - operator = described_class.new(left, right) - - expect(operator.evaluate).to eq false + it { is_expected.to eq(true) } end - it 'supports matching against a nil value' do - allow(left).to receive(:evaluate).and_return(nil) - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('pattern')) + context 'when left and right match' do + let(:left_value) { 'my-awesome-string' } + let(:right_value) { Gitlab::UntrustedRegexp.new('awesome.string$') } + + it { is_expected.to eq(false) } + end - operator = described_class.new(left, right) + context 'when left is nil' do + let(:left_value) { nil } + let(:right_value) { Gitlab::UntrustedRegexp.new('pattern') } - expect(operator.evaluate).to eq true + it { is_expected.to eq(true) } end - it 'supports multiline strings' do - allow(left).to receive(:evaluate).and_return <<~TEXT - My awesome contents + context 'when left is a multiline string and matches right' do + let(:left_value) do + <<~TEXT + My awesome contents + + My-text-string! + TEXT + end + + let(:right_value) { Gitlab::UntrustedRegexp.new('text-string') } + + it { is_expected.to eq(false) } + end - My-text-string! - TEXT + context 'when left is a multiline string and does not match right' do + let(:left_value) do + <<~TEXT + My awesome contents - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('text-string')) + My-terrible-string! + TEXT + end - operator = described_class.new(left, right) + let(:right_value) { Gitlab::UntrustedRegexp.new('text-string') } - expect(operator.evaluate).to eq false + it { is_expected.to eq(true) } end - it 'supports regexp flags' do - allow(left).to receive(:evaluate).and_return <<~TEXT - My AWESOME content - TEXT + context 'when a matching pattern uses regex flags' do + let(:left_value) do + <<~TEXT + My AWESOME content + TEXT + end + + let(:right_value) { Gitlab::UntrustedRegexp.new('(?i)awesome') } + + it { is_expected.to eq(false) } + end - allow(right).to receive(:evaluate) - .and_return(Gitlab::UntrustedRegexp.new('(?i)awesome')) + context 'when a non-matching pattern uses regex flags' do + let(:left_value) do + <<~TEXT + My AWESOME content + TEXT + end - operator = described_class.new(left, right) + let(:right_value) { Gitlab::UntrustedRegexp.new('(?i)terrible') } - expect(operator.evaluate).to eq false + it { is_expected.to eq(true) } end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/or_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/or_spec.rb new file mode 100644 index 00000000000..d542eebc613 --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/or_spec.rb @@ -0,0 +1,77 @@ +require 'fast_spec_helper' +require 'rspec-parameterized' + +describe Gitlab::Ci::Pipeline::Expression::Lexeme::Or do + let(:left) { double('left', evaluate: nil) } + let(:right) { double('right', evaluate: nil) } + + describe '.build' do + it 'creates a new instance of the token' do + expect(described_class.build('||', left, right)).to be_a(described_class) + end + + context 'with non-evaluable operands' do + let(:left) { double('left') } + let(:right) { double('right') } + + it 'raises an operator error' do + expect { described_class.build('||', left, right) }.to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + end + + describe '.type' do + it 'is an operator' do + expect(described_class.type).to eq :operator + end + end + + describe '.precedence' do + it 'has a precedence' do + expect(described_class.precedence).to be_an Integer + end + end + + describe '#evaluate' do + let(:operator) { described_class.new(left, right) } + + subject { operator.evaluate } + + before do + allow(left).to receive(:evaluate).and_return(left_value) + allow(right).to receive(:evaluate).and_return(right_value) + end + + context 'when left and right are truthy' do + where(:left_value, :right_value) do + [true, 1, 'a'].permutation(2).to_a + end + + with_them do + it { is_expected.to be_truthy } + it { is_expected.to eq(left_value) } + end + end + + context 'when left or right is truthy' do + where(:left_value, :right_value) do + [true, false, 'a'].permutation(2).to_a + end + + with_them do + it { is_expected.to be_truthy } + end + end + + context 'when left and right are falsey' do + where(:left_value, :right_value) do + [false, nil].permutation(2).to_a + end + + with_them do + it { is_expected.to be_falsey } + it { is_expected.to eq(right_value) } + end + end + end +end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb index cff7f57ceff..30ea3f3e28e 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb @@ -1,4 +1,4 @@ -require 'fast_spec_helper' +require 'spec_helper' describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do describe '.build' do @@ -30,16 +30,6 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do .to eq Gitlab::UntrustedRegexp.new('pattern') end - it 'is a greedy scanner for regexp boundaries' do - scanner = StringScanner.new('/some .* / pattern/') - - token = described_class.scan(scanner) - - expect(token).not_to be_nil - expect(token.build.evaluate) - .to eq Gitlab::UntrustedRegexp.new('some .* / pattern') - end - it 'does not allow to use an empty pattern' do scanner = StringScanner.new(%(//)) @@ -68,12 +58,90 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do .to eq Gitlab::UntrustedRegexp.new('(?im)pattern') end - it 'does not support arbitrary flags' do + it 'ignores unsupported flags' do scanner = StringScanner.new('/pattern/x') token = described_class.scan(scanner) - expect(token).to be_nil + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('pattern') + end + + it 'is a eager scanner for regexp boundaries' do + scanner = StringScanner.new('/some .* / pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some .* ') + end + + it 'does not match on escaped regexp boundaries' do + scanner = StringScanner.new('/some .* \/ pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some .* / pattern') + end + + it 'recognizes \ as an escape character for /' do + scanner = StringScanner.new('/some numeric \/$ pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some numeric /$ pattern') + end + + it 'does not recognize \ as an escape character for $' do + scanner = StringScanner.new('/some numeric \$ pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some numeric \$ pattern') + end + + context 'with the ci_variables_complex_expressions feature flag disabled' do + before do + stub_feature_flags(ci_variables_complex_expressions: false) + end + + it 'is a greedy scanner for regexp boundaries' do + scanner = StringScanner.new('/some .* / pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some .* / pattern') + end + + it 'does not recognize the \ escape character for /' do + scanner = StringScanner.new('/some .* \/ pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some .* \/ pattern') + end + + it 'does not recognize the \ escape character for $' do + scanner = StringScanner.new('/some numeric \$ pattern/') + + token = described_class.scan(scanner) + + expect(token).not_to be_nil + expect(token.build.evaluate) + .to eq Gitlab::UntrustedRegexp.new('some numeric \$ pattern') + end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexer_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexer_spec.rb index 3f11b3f7673..d8db9c262a1 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexer_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexer_spec.rb @@ -58,6 +58,56 @@ describe Gitlab::Ci::Pipeline::Expression::Lexer do expect { lexer.tokens } .to raise_error described_class::SyntaxError end + + context 'with complex expressions' do + using RSpec::Parameterized::TableSyntax + + subject { described_class.new(expression).tokens.map(&:value) } + + where(:expression, :tokens) do + '$PRESENT_VARIABLE =~ /my var/ && $EMPTY_VARIABLE =~ /nope/' | ['$PRESENT_VARIABLE', '=~', '/my var/', '&&', '$EMPTY_VARIABLE', '=~', '/nope/'] + '$EMPTY_VARIABLE == "" && $PRESENT_VARIABLE' | ['$EMPTY_VARIABLE', '==', '""', '&&', '$PRESENT_VARIABLE'] + '$EMPTY_VARIABLE == "" && $PRESENT_VARIABLE != "nope"' | ['$EMPTY_VARIABLE', '==', '""', '&&', '$PRESENT_VARIABLE', '!=', '"nope"'] + '$PRESENT_VARIABLE && $EMPTY_VARIABLE' | ['$PRESENT_VARIABLE', '&&', '$EMPTY_VARIABLE'] + '$PRESENT_VARIABLE =~ /my var/ || $EMPTY_VARIABLE =~ /nope/' | ['$PRESENT_VARIABLE', '=~', '/my var/', '||', '$EMPTY_VARIABLE', '=~', '/nope/'] + '$EMPTY_VARIABLE == "" || $PRESENT_VARIABLE' | ['$EMPTY_VARIABLE', '==', '""', '||', '$PRESENT_VARIABLE'] + '$EMPTY_VARIABLE == "" || $PRESENT_VARIABLE != "nope"' | ['$EMPTY_VARIABLE', '==', '""', '||', '$PRESENT_VARIABLE', '!=', '"nope"'] + '$PRESENT_VARIABLE || $EMPTY_VARIABLE' | ['$PRESENT_VARIABLE', '||', '$EMPTY_VARIABLE'] + '$PRESENT_VARIABLE && null || $EMPTY_VARIABLE == ""' | ['$PRESENT_VARIABLE', '&&', 'null', '||', '$EMPTY_VARIABLE', '==', '""'] + end + + with_them do + it { is_expected.to eq(tokens) } + end + end + + context 'with the ci_variables_complex_expressions feature flag turned off' do + before do + stub_feature_flags(ci_variables_complex_expressions: false) + end + + it 'incorrectly tokenizes conjunctive match statements as one match statement' do + tokens = described_class.new('$PRESENT_VARIABLE =~ /my var/ && $EMPTY_VARIABLE =~ /nope/').tokens + + expect(tokens.map(&:value)).to eq(['$PRESENT_VARIABLE', '=~', '/my var/ && $EMPTY_VARIABLE =~ /nope/']) + end + + it 'incorrectly tokenizes disjunctive match statements as one statement' do + tokens = described_class.new('$PRESENT_VARIABLE =~ /my var/ || $EMPTY_VARIABLE =~ /nope/').tokens + + expect(tokens.map(&:value)).to eq(['$PRESENT_VARIABLE', '=~', '/my var/ || $EMPTY_VARIABLE =~ /nope/']) + end + + it 'raises an error about && operators' do + expect { described_class.new('$EMPTY_VARIABLE == "" && $PRESENT_VARIABLE').tokens } + .to raise_error(Gitlab::Ci::Pipeline::Expression::Lexer::SyntaxError).with_message('Unknown lexeme found!') + end + + it 'raises an error about || operators' do + expect { described_class.new('$EMPTY_VARIABLE == "" || $PRESENT_VARIABLE').tokens } + .to raise_error(Gitlab::Ci::Pipeline::Expression::Lexer::SyntaxError).with_message('Unknown lexeme found!') + end + end end describe '#lexemes' do diff --git a/spec/lib/gitlab/ci/pipeline/expression/parser_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/parser_spec.rb index 2b78b1dd4a7..e88ec5561b6 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/parser_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/parser_spec.rb @@ -2,25 +2,67 @@ require 'fast_spec_helper' describe Gitlab::Ci::Pipeline::Expression::Parser do describe '#tree' do - context 'when using operators' do + context 'when using two operators' do + it 'returns a reverse descent parse tree' do + expect(described_class.seed('$VAR1 == "123"').tree) + .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::Equals + end + end + + context 'when using three operators' do it 'returns a reverse descent parse tree' do expect(described_class.seed('$VAR1 == "123" == $VAR2').tree) .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::Equals end end - context 'when using a single token' do + context 'when using a single variable token' do it 'returns a single token instance' do expect(described_class.seed('$VAR').tree) .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::Variable end end + context 'when using a single string token' do + it 'returns a single token instance' do + expect(described_class.seed('"some value"').tree) + .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::String + end + end + context 'when expression is empty' do it 'returns a null token' do - expect(described_class.seed('').tree) + expect { described_class.seed('').tree } + .to raise_error Gitlab::Ci::Pipeline::Expression::Parser::ParseError + end + end + + context 'when expression is null' do + it 'returns a null token' do + expect(described_class.seed('null').tree) .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::Null end end + + context 'when two value tokens have no operator' do + it 'raises a parsing error' do + expect { described_class.seed('$VAR "text"').tree } + .to raise_error Gitlab::Ci::Pipeline::Expression::Parser::ParseError + end + end + + context 'when an operator has no left side' do + it 'raises an OperatorError' do + expect { described_class.seed('== "123"').tree } + .to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end + + context 'when an operator has no right side' do + it 'raises an OperatorError' do + expect { described_class.seed('$VAR ==').tree } + .to raise_error Gitlab::Ci::Pipeline::Expression::Lexeme::Operator::OperatorError + end + end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb index a9fd809409b..057e2f3fbe8 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb @@ -1,4 +1,6 @@ -require 'fast_spec_helper' +# TODO switch this back after the "ci_variables_complex_expressions" feature flag is removed +# require 'fast_spec_helper' +require 'spec_helper' require 'rspec-parameterized' describe Gitlab::Ci::Pipeline::Expression::Statement do @@ -7,8 +9,12 @@ describe Gitlab::Ci::Pipeline::Expression::Statement do end let(:variables) do - { 'PRESENT_VARIABLE' => 'my variable', - EMPTY_VARIABLE: '' } + { + 'PRESENT_VARIABLE' => 'my variable', + 'PATH_VARIABLE' => 'a/path/variable/value', + 'FULL_PATH_VARIABLE' => '/a/full/path/variable/value', + 'EMPTY_VARIABLE' => '' + } end describe '.new' do @@ -21,105 +27,158 @@ describe Gitlab::Ci::Pipeline::Expression::Statement do end end - describe '#parse_tree' do - context 'when expression is empty' do - let(:text) { '' } - - it 'raises an error' do - expect { subject.parse_tree } - .to raise_error described_class::StatementError - end - end + describe '#evaluate' do + using RSpec::Parameterized::TableSyntax - context 'when expression grammar is incorrect' do - table = [ - '$VAR "text"', # missing operator - '== "123"', # invalid left side - '"some string"', # only string provided - '$VAR ==', # invalid right side - 'null', # missing lexemes - '' # empty statement - ] - - table.each do |syntax| - context "when expression grammar is #{syntax.inspect}" do - let(:text) { syntax } - - it 'raises a statement error exception' do - expect { subject.parse_tree } - .to raise_error described_class::StatementError - end - - it 'is an invalid statement' do - expect(subject).not_to be_valid - end - end - end + where(:expression, :value) do + '$PRESENT_VARIABLE == "my variable"' | true + '"my variable" == $PRESENT_VARIABLE' | true + '$PRESENT_VARIABLE == null' | false + '$EMPTY_VARIABLE == null' | false + '"" == $EMPTY_VARIABLE' | true + '$EMPTY_VARIABLE' | '' + '$UNDEFINED_VARIABLE == null' | true + 'null == $UNDEFINED_VARIABLE' | true + '$PRESENT_VARIABLE' | 'my variable' + '$UNDEFINED_VARIABLE' | nil + "$PRESENT_VARIABLE =~ /var.*e$/" | 3 + '$PRESENT_VARIABLE =~ /va\r.*e$/' | nil + '$PRESENT_VARIABLE =~ /va\/r.*e$/' | nil + "$PRESENT_VARIABLE =~ /var.*e$/" | 3 + "$PRESENT_VARIABLE =~ /^var.*/" | nil + "$EMPTY_VARIABLE =~ /var.*/" | nil + "$UNDEFINED_VARIABLE =~ /var.*/" | nil + "$PRESENT_VARIABLE =~ /VAR.*/i" | 3 + '$PATH_VARIABLE =~ /path\/variable/' | 2 + '$FULL_PATH_VARIABLE =~ /^\/a\/full\/path\/variable\/value$/' | 0 + '$FULL_PATH_VARIABLE =~ /\\/path\\/variable\\/value$/' | 7 + '$PRESENT_VARIABLE != "my variable"' | false + '"my variable" != $PRESENT_VARIABLE' | false + '$PRESENT_VARIABLE != null' | true + '$EMPTY_VARIABLE != null' | true + '"" != $EMPTY_VARIABLE' | false + '$UNDEFINED_VARIABLE != null' | false + 'null != $UNDEFINED_VARIABLE' | false + "$PRESENT_VARIABLE !~ /var.*e$/" | false + "$PRESENT_VARIABLE !~ /^var.*/" | true + '$PRESENT_VARIABLE !~ /^v\ar.*/' | true + '$PRESENT_VARIABLE !~ /^v\/ar.*/' | true + "$EMPTY_VARIABLE !~ /var.*/" | true + "$UNDEFINED_VARIABLE !~ /var.*/" | true + "$PRESENT_VARIABLE !~ /VAR.*/i" | false + + '$PRESENT_VARIABLE && "string"' | 'string' + '$PRESENT_VARIABLE && $PRESENT_VARIABLE' | 'my variable' + '$PRESENT_VARIABLE && $EMPTY_VARIABLE' | '' + '$PRESENT_VARIABLE && null' | nil + '"string" && $PRESENT_VARIABLE' | 'my variable' + '$EMPTY_VARIABLE && $PRESENT_VARIABLE' | 'my variable' + 'null && $PRESENT_VARIABLE' | nil + '$EMPTY_VARIABLE && "string"' | 'string' + '$EMPTY_VARIABLE && $EMPTY_VARIABLE' | '' + '"string" && $EMPTY_VARIABLE' | '' + '"string" && null' | nil + 'null && "string"' | nil + '"string" && "string"' | 'string' + 'null && null' | nil + + '$PRESENT_VARIABLE =~ /my var/ && $EMPTY_VARIABLE =~ /nope/' | nil + '$EMPTY_VARIABLE == "" && $PRESENT_VARIABLE' | 'my variable' + '$EMPTY_VARIABLE == "" && $PRESENT_VARIABLE != "nope"' | true + '$PRESENT_VARIABLE && $EMPTY_VARIABLE' | '' + '$PRESENT_VARIABLE && $UNDEFINED_VARIABLE' | nil + '$UNDEFINED_VARIABLE && $EMPTY_VARIABLE' | nil + '$UNDEFINED_VARIABLE && $PRESENT_VARIABLE' | nil + + '$FULL_PATH_VARIABLE =~ /^\/a\/full\/path\/variable\/value$/ && $PATH_VARIABLE =~ /path\/variable/' | 2 + '$FULL_PATH_VARIABLE =~ /^\/a\/bad\/path\/variable\/value$/ && $PATH_VARIABLE =~ /path\/variable/' | nil + '$FULL_PATH_VARIABLE =~ /^\/a\/full\/path\/variable\/value$/ && $PATH_VARIABLE =~ /bad\/path\/variable/' | nil + '$FULL_PATH_VARIABLE =~ /^\/a\/bad\/path\/variable\/value$/ && $PATH_VARIABLE =~ /bad\/path\/variable/' | nil + + '$FULL_PATH_VARIABLE =~ /^\/a\/full\/path\/variable\/value$/ || $PATH_VARIABLE =~ /path\/variable/' | 0 + '$FULL_PATH_VARIABLE =~ /^\/a\/bad\/path\/variable\/value$/ || $PATH_VARIABLE =~ /path\/variable/' | 2 + '$FULL_PATH_VARIABLE =~ /^\/a\/full\/path\/variable\/value$/ || $PATH_VARIABLE =~ /bad\/path\/variable/' | 0 + '$FULL_PATH_VARIABLE =~ /^\/a\/bad\/path\/variable\/value$/ || $PATH_VARIABLE =~ /bad\/path\/variable/' | nil + + '$PRESENT_VARIABLE =~ /my var/ || $EMPTY_VARIABLE =~ /nope/' | 0 + '$EMPTY_VARIABLE == "" || $PRESENT_VARIABLE' | true + '$PRESENT_VARIABLE != "nope" || $EMPTY_VARIABLE == ""' | true + + '$PRESENT_VARIABLE && null || $EMPTY_VARIABLE == ""' | true + '$PRESENT_VARIABLE || $UNDEFINED_VARIABLE' | 'my variable' + '$UNDEFINED_VARIABLE || $PRESENT_VARIABLE' | 'my variable' + '$UNDEFINED_VARIABLE == null || $PRESENT_VARIABLE' | true + '$PRESENT_VARIABLE || $UNDEFINED_VARIABLE == null' | 'my variable' end - context 'when expression grammar is correct' do - context 'when using an operator' do - let(:text) { '$VAR == "value"' } - - it 'returns a reverse descent parse tree' do - expect(subject.parse_tree) - .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::Equals - end + with_them do + let(:text) { expression } - it 'is a valid statement' do - expect(subject).to be_valid - end + it "evaluates to `#{params[:value].inspect}`" do + expect(subject.evaluate).to eq(value) end - context 'when using a single token' do - let(:text) { '$PRESENT_VARIABLE' } + # This test is used to ensure that our parser + # returns exactly the same results as if we + # were evaluating using ruby's `eval` + context 'when using Ruby eval' do + let(:expression_ruby) do + expression + .gsub(/null/, 'nil') + .gsub(/\$([a-zA-Z_][a-zA-Z0-9_]*)/) { "variables['#{Regexp.last_match(1)}']" } + end - it 'returns a single token instance' do - expect(subject.parse_tree) - .to be_a Gitlab::Ci::Pipeline::Expression::Lexeme::Variable + it 'behaves exactly the same' do + expect(instance_eval(expression_ruby)).to eq(subject.evaluate) end end end - end - describe '#evaluate' do - using RSpec::Parameterized::TableSyntax + context 'with the ci_variables_complex_expressions feature flag disabled' do + before do + stub_feature_flags(ci_variables_complex_expressions: false) + end - where(:expression, :value) do - '$PRESENT_VARIABLE == "my variable"' | true - '"my variable" == $PRESENT_VARIABLE' | true - '$PRESENT_VARIABLE == null' | false - '$EMPTY_VARIABLE == null' | false - '"" == $EMPTY_VARIABLE' | true - '$EMPTY_VARIABLE' | '' - '$UNDEFINED_VARIABLE == null' | true - 'null == $UNDEFINED_VARIABLE' | true - '$PRESENT_VARIABLE' | 'my variable' - '$UNDEFINED_VARIABLE' | nil - "$PRESENT_VARIABLE =~ /var.*e$/" | true - "$PRESENT_VARIABLE =~ /^var.*/" | false - "$EMPTY_VARIABLE =~ /var.*/" | false - "$UNDEFINED_VARIABLE =~ /var.*/" | false - "$PRESENT_VARIABLE =~ /VAR.*/i" | true - '$PRESENT_VARIABLE != "my variable"' | false - '"my variable" != $PRESENT_VARIABLE' | false - '$PRESENT_VARIABLE != null' | true - '$EMPTY_VARIABLE != null' | true - '"" != $EMPTY_VARIABLE' | false - '$UNDEFINED_VARIABLE != null' | false - 'null != $UNDEFINED_VARIABLE' | false - "$PRESENT_VARIABLE !~ /var.*e$/" | false - "$PRESENT_VARIABLE !~ /^var.*/" | true - "$EMPTY_VARIABLE !~ /var.*/" | true - "$UNDEFINED_VARIABLE !~ /var.*/" | true - "$PRESENT_VARIABLE !~ /VAR.*/i" | false - end + where(:expression, :value) do + '$PRESENT_VARIABLE == "my variable"' | true + '"my variable" == $PRESENT_VARIABLE' | true + '$PRESENT_VARIABLE == null' | false + '$EMPTY_VARIABLE == null' | false + '"" == $EMPTY_VARIABLE' | true + '$EMPTY_VARIABLE' | '' + '$UNDEFINED_VARIABLE == null' | true + 'null == $UNDEFINED_VARIABLE' | true + '$PRESENT_VARIABLE' | 'my variable' + '$UNDEFINED_VARIABLE' | nil + "$PRESENT_VARIABLE =~ /var.*e$/" | true + "$PRESENT_VARIABLE =~ /^var.*/" | false + "$EMPTY_VARIABLE =~ /var.*/" | false + "$UNDEFINED_VARIABLE =~ /var.*/" | false + "$PRESENT_VARIABLE =~ /VAR.*/i" | true + '$PATH_VARIABLE =~ /path/variable/' | true + '$PATH_VARIABLE =~ /path\/variable/' | true + '$FULL_PATH_VARIABLE =~ /^/a/full/path/variable/value$/' | true + '$FULL_PATH_VARIABLE =~ /^\/a\/full\/path\/variable\/value$/' | true + '$PRESENT_VARIABLE != "my variable"' | false + '"my variable" != $PRESENT_VARIABLE' | false + '$PRESENT_VARIABLE != null' | true + '$EMPTY_VARIABLE != null' | true + '"" != $EMPTY_VARIABLE' | false + '$UNDEFINED_VARIABLE != null' | false + 'null != $UNDEFINED_VARIABLE' | false + "$PRESENT_VARIABLE !~ /var.*e$/" | false + "$PRESENT_VARIABLE !~ /^var.*/" | true + "$EMPTY_VARIABLE !~ /var.*/" | true + "$UNDEFINED_VARIABLE !~ /var.*/" | true + "$PRESENT_VARIABLE !~ /VAR.*/i" | false + end - with_them do - let(:text) { expression } + with_them do + let(:text) { expression } - it "evaluates to `#{params[:value].inspect}`" do - expect(subject.evaluate).to eq value + it "evaluates to `#{params[:value].inspect}`" do + expect(subject.evaluate).to eq value + end end end end @@ -137,6 +196,8 @@ describe Gitlab::Ci::Pipeline::Expression::Statement do '$INVALID = 1' | false "$PRESENT_VARIABLE =~ /var.*/" | true "$UNDEFINED_VARIABLE =~ /var.*/" | false + "$PRESENT_VARIABLE !~ /var.*/" | false + "$UNDEFINED_VARIABLE !~ /var.*/" | true end with_them do -- cgit v1.2.1 From cdf9c861f4cd4d18f449735c35105b31813e0798 Mon Sep 17 00:00:00 2001 From: Jamie Hurewitz Date: Wed, 5 Jun 2019 01:12:07 +0000 Subject: Apply suggestion to doc/development/licensing.md --- doc/development/licensing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/licensing.md b/doc/development/licensing.md index 0550088ed5a..0db90d2872f 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -90,7 +90,7 @@ GitLab means GitLab Inc. and its affiliates and subsidiaries. ## Requesting Approval for Licenses or any other Intellectual Property -Libraries that are not already approved on the [Acceptable Licenses][Acceptable-Licenses] or that may be listed on the [Unacceptable Licenses][Unacceptable-Licenses] list may be submitted to the legal team for review and use on a case by case basis. Please email `legal@gitlab.com` with the details of how the software will be used, whether it will be modified or not, and how it will be distributed (if at all). After a decision has been made, the original requestor is responsible for updating this document, if applicable. Not all approvals will be approved for universal use and may continue to remain on the Unacceptable License list. +Libraries that are not already approved and listed on the [Acceptable Licenses][Acceptable-Licenses] list or that may be listed on the [Unacceptable Licenses][Unacceptable-Licenses] list may be submitted to the legal team for review and use on a case-by-case basis. Please email `legal@gitlab.com` with the details of how the software will be used, whether or not it will be modified, and how it will be distributed (if at all). After a decision has been made, the original requestor is responsible for updating this document, if applicable. Not all approvals will be approved for universal use and may continue to remain on the Unacceptable License list. All inquiries relating to patents should be directed to the Legal team. -- cgit v1.2.1 From 2eecfd8f9d111c6518930b818a16daea8263b37f Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Tue, 4 Jun 2019 20:59:48 -0800 Subject: Use Redis for CacheMarkDownField on non AR models This allows using `CacheMarkdownField` for models that are not backed by ActiveRecord. When the including class inherits `ActiveRecord::Base` we include `Gitlab::MarkdownCache::ActiveRecord::Extension`. This will cause the markdown fields to be rendered and the generated HTML stored in a `_html` attribute on the record. We also store the version used for generating the markdown. All other classes that include this model will include the `Gitlab::MarkdownCache::Redis::Extension`. This add the `_html` attributes to that model and will generate the html in them. The generated HTML will be cached in redis under the key `markdown_cache::`. The class this included in must therefore respond to `id`. --- app/models/commit.rb | 11 +- app/models/concerns/cache_markdown_field.rb | 88 +--- .../54140-non-ar-cache-commit-markdown.yml | 5 + lib/banzai/commit_renderer.rb | 2 +- lib/gitlab/markdown_cache.rb | 12 + .../markdown_cache/active_record/extension.rb | 55 +++ lib/gitlab/markdown_cache/field_data.rb | 35 ++ lib/gitlab/markdown_cache/redis/extension.rb | 63 +++ lib/gitlab/markdown_cache/redis/store.rb | 56 +++ .../markdown/gitlab_flavored_markdown_spec.rb | 3 +- spec/helpers/markup_helper_spec.rb | 3 +- spec/lib/banzai/commit_renderer_spec.rb | 4 +- spec/lib/banzai/object_renderer_spec.rb | 28 +- spec/lib/banzai/renderer_spec.rb | 14 +- .../markdown_cache/active_record/extension_spec.rb | 177 ++++++++ spec/lib/gitlab/markdown_cache/field_data_spec.rb | 15 + .../gitlab/markdown_cache/redis/extension_spec.rb | 76 ++++ spec/lib/gitlab/markdown_cache/redis/store_spec.rb | 68 ++++ spec/models/concerns/cache_markdown_field_spec.rb | 446 +++++++-------------- spec/models/resource_label_event_spec.rb | 4 +- 20 files changed, 760 insertions(+), 405 deletions(-) create mode 100644 changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml create mode 100644 lib/gitlab/markdown_cache.rb create mode 100644 lib/gitlab/markdown_cache/active_record/extension.rb create mode 100644 lib/gitlab/markdown_cache/field_data.rb create mode 100644 lib/gitlab/markdown_cache/redis/extension.rb create mode 100644 lib/gitlab/markdown_cache/redis/store.rb create mode 100644 spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb create mode 100644 spec/lib/gitlab/markdown_cache/field_data_spec.rb create mode 100644 spec/lib/gitlab/markdown_cache/redis/extension_spec.rb create mode 100644 spec/lib/gitlab/markdown_cache/redis/store_spec.rb diff --git a/app/models/commit.rb b/app/models/commit.rb index f412d252e5c..fa0bf36ba49 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -13,6 +13,7 @@ class Commit include StaticModel include Presentable include ::Gitlab::Utils::StrongMemoize + include CacheMarkdownField attr_mentionable :safe_message, pipeline: :single_line @@ -37,13 +38,9 @@ class Commit # Used by GFM to match and present link extensions on node texts and hrefs. LINK_EXTENSION_PATTERN = /(patch)/.freeze - def banzai_render_context(field) - pipeline = field == :description ? :commit_description : :single_line - context = { pipeline: pipeline, project: self.project } - context[:author] = self.author if self.author - - context - end + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :full_title, pipeline: :single_line + cache_markdown_field :description, pipeline: :commit_description class << self def decorate(commits, project) diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index f90cd1ea690..42203a5f214 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -13,43 +13,9 @@ module CacheMarkdownField extend ActiveSupport::Concern - # Increment this number every time the renderer changes its output - CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 16 - # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze - # Knows about the relationship between markdown and html field names, and - # stores the rendering contexts for the latter - class FieldData - def initialize - @data = {} - end - - delegate :[], :[]=, to: :@data - - def markdown_fields - @data.keys - end - - def html_field(markdown_field) - "#{markdown_field}_html" - end - - def html_fields - markdown_fields.map { |field| html_field(field) } - end - - def html_fields_whitelisted - markdown_fields.each_with_object([]) do |field, fields| - if @data[field].fetch(:whitelisted, false) - fields << html_field(field) - end - end - end - end - def skip_project_check? false end @@ -85,24 +51,22 @@ module CacheMarkdownField end.to_h updates['cached_markdown_version'] = latest_cached_markdown_version - updates.each {|html_field, data| write_attribute(html_field, data) } + updates.each { |field, data| write_markdown_field(field, data) } end def refresh_markdown_cache! updates = refresh_markdown_cache - return unless persisted? && Gitlab::Database.read_write? - - update_columns(updates) + save_markdown(updates) end def cached_html_up_to_date?(markdown_field) - html_field = cached_markdown_fields.html_field(markdown_field) + return false if cached_html_for(markdown_field).nil? && __send__(markdown_field).present? # rubocop:disable GitlabSecurity/PublicSend - return false if cached_html_for(markdown_field).nil? && !__send__(markdown_field).nil? # rubocop:disable GitlabSecurity/PublicSend + html_field = cached_markdown_fields.html_field(markdown_field) - markdown_changed = attribute_changed?(markdown_field) || false - html_changed = attribute_changed?(html_field) || false + markdown_changed = markdown_field_changed?(markdown_field) + html_changed = markdown_field_changed?(html_field) latest_cached_markdown_version == cached_markdown_version && (html_changed || markdown_changed == html_changed) @@ -117,21 +81,21 @@ module CacheMarkdownField end def cached_html_for(markdown_field) - raise ArgumentError.new("Unknown field: #{field}") unless + raise ArgumentError.new("Unknown field: #{markdown_field}") unless cached_markdown_fields.markdown_fields.include?(markdown_field) __send__(cached_markdown_fields.html_field(markdown_field)) # rubocop:disable GitlabSecurity/PublicSend end def latest_cached_markdown_version - @latest_cached_markdown_version ||= (CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) | local_version + @latest_cached_markdown_version ||= (Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) | local_version end def local_version # because local_markdown_version is stored in application_settings which # uses cached_markdown_version too, we check explicitly to avoid # endless loop - return local_markdown_version if has_attribute?(:local_markdown_version) + return local_markdown_version if respond_to?(:has_attribute?) && has_attribute?(:local_markdown_version) settings = Gitlab::CurrentSettings.current_application_settings @@ -150,32 +114,14 @@ module CacheMarkdownField included do cattr_reader :cached_markdown_fields do - FieldData.new + Gitlab::MarkdownCache::FieldData.new end - # Always exclude _html fields from attributes (including serialization). - # They contain unredacted HTML, which would be a security issue - alias_method :attributes_before_markdown_cache, :attributes - def attributes - attrs = attributes_before_markdown_cache - html_fields = cached_markdown_fields.html_fields - whitelisted = cached_markdown_fields.html_fields_whitelisted - exclude_fields = html_fields - whitelisted - - exclude_fields.each do |field| - attrs.delete(field) - end - - if whitelisted.empty? - attrs.delete('cached_markdown_version') - end - - attrs + if self < ActiveRecord::Base + include Gitlab::MarkdownCache::ActiveRecord::Extension + else + prepend Gitlab::MarkdownCache::Redis::Extension end - - # Using before_update here conflicts with elasticsearch-model somehow - before_create :refresh_markdown_cache, if: :invalidated_markdown_cache? - before_update :refresh_markdown_cache, if: :invalidated_markdown_cache? end class_methods do @@ -193,10 +139,8 @@ module CacheMarkdownField # The HTML becomes invalid if any dependent fields change. For now, assume # author and project invalidate the cache in all circumstances. define_method(invalidation_method) do - changed_fields = changed_attributes.keys - invalidations = changed_fields & [markdown_field.to_s, *INVALIDATED_BY] - invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html") - + invalidations = changed_markdown_fields & [markdown_field.to_s, *INVALIDATED_BY] + invalidations.delete(markdown_field.to_s) if changed_markdown_fields.include?("#{markdown_field}_html") !invalidations.empty? || !cached_html_up_to_date?(markdown_field) end end diff --git a/changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml b/changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml new file mode 100644 index 00000000000..efda07380a4 --- /dev/null +++ b/changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml @@ -0,0 +1,5 @@ +--- +title: Use Redis for CacheMarkDownField on non AR models +merge_request: 29054 +author: +type: performance diff --git a/lib/banzai/commit_renderer.rb b/lib/banzai/commit_renderer.rb index f346151a3c1..2acc9d13f07 100644 --- a/lib/banzai/commit_renderer.rb +++ b/lib/banzai/commit_renderer.rb @@ -2,7 +2,7 @@ module Banzai module CommitRenderer - ATTRIBUTES = [:description, :title].freeze + ATTRIBUTES = [:description, :title, :full_title].freeze def self.render(commits, project, user = nil) obj_renderer = ObjectRenderer.new(user: user, default_project: project) diff --git a/lib/gitlab/markdown_cache.rb b/lib/gitlab/markdown_cache.rb new file mode 100644 index 00000000000..0354c710a3f --- /dev/null +++ b/lib/gitlab/markdown_cache.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + # Increment this number every time the renderer changes its output + CACHE_COMMONMARK_VERSION_START = 10 + CACHE_COMMONMARK_VERSION = 16 + + BaseError = Class.new(StandardError) + UnsupportedClassError = Class.new(BaseError) + end +end diff --git a/lib/gitlab/markdown_cache/active_record/extension.rb b/lib/gitlab/markdown_cache/active_record/extension.rb new file mode 100644 index 00000000000..bcc3432bd31 --- /dev/null +++ b/lib/gitlab/markdown_cache/active_record/extension.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + module ActiveRecord + module Extension + extend ActiveSupport::Concern + + included do + # Always exclude _html fields from attributes (including serialization). + # They contain unredacted HTML, which would be a security issue + alias_method :attributes_before_markdown_cache, :attributes + def attributes + attrs = attributes_before_markdown_cache + html_fields = cached_markdown_fields.html_fields + whitelisted = cached_markdown_fields.html_fields_whitelisted + exclude_fields = html_fields - whitelisted + + exclude_fields.each do |field| + attrs.delete(field) + end + + if whitelisted.empty? + attrs.delete('cached_markdown_version') + end + + attrs + end + + # Using before_update here conflicts with elasticsearch-model somehow + before_create :refresh_markdown_cache, if: :invalidated_markdown_cache? + before_update :refresh_markdown_cache, if: :invalidated_markdown_cache? + end + + def changed_markdown_fields + changed_attributes.keys.map(&:to_s) & cached_markdown_fields.markdown_fields.map(&:to_s) + end + + def write_markdown_field(field_name, value) + write_attribute(field_name, value) + end + + def markdown_field_changed?(field_name) + attribute_changed?(field_name) + end + + def save_markdown(updates) + return unless persisted? && Gitlab::Database.read_write? + + update_columns(updates) + end + end + end + end +end diff --git a/lib/gitlab/markdown_cache/field_data.rb b/lib/gitlab/markdown_cache/field_data.rb new file mode 100644 index 00000000000..14622c0f186 --- /dev/null +++ b/lib/gitlab/markdown_cache/field_data.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + # Knows about the relationship between markdown and html field names, and + # stores the rendering contexts for the latter + class FieldData + def initialize + @data = {} + end + + delegate :[], :[]=, to: :@data + + def markdown_fields + @data.keys + end + + def html_field(markdown_field) + "#{markdown_field}_html" + end + + def html_fields + @html_fields ||= markdown_fields.map { |field| html_field(field) } + end + + def html_fields_whitelisted + markdown_fields.each_with_object([]) do |field, fields| + if @data[field].fetch(:whitelisted, false) + fields << html_field(field) + end + end + end + end + end +end diff --git a/lib/gitlab/markdown_cache/redis/extension.rb b/lib/gitlab/markdown_cache/redis/extension.rb new file mode 100644 index 00000000000..97fc23343b4 --- /dev/null +++ b/lib/gitlab/markdown_cache/redis/extension.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + module Redis + module Extension + extend ActiveSupport::Concern + + attr_reader :cached_markdown_version + + class_methods do + def cache_markdown_field(markdown_field, context = {}) + super + + # define the `[field]_html` accessor + html_field = cached_markdown_fields.html_field(markdown_field) + define_method(html_field) do + load_cached_markdown unless markdown_data_loaded? + + instance_variable_get("@#{html_field}") + end + end + end + + private + + def save_markdown(updates) + markdown_store.save(updates) + end + + def write_markdown_field(field_name, value) + instance_variable_set("@#{field_name}", value) + end + + def markdown_field_changed?(field_name) + false + end + + def changed_markdown_fields + [] + end + + def cached_markdown + @cached_data ||= markdown_store.read + end + + def load_cached_markdown + cached_markdown.each do |field_name, value| + write_markdown_field(field_name, value) + end + end + + def markdown_data_loaded? + cached_markdown_version.present? || markdown_store.loaded? + end + + def markdown_store + @store ||= Gitlab::MarkdownCache::Redis::Store.new(self) + end + end + end + end +end diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb new file mode 100644 index 00000000000..a35eebc6a1b --- /dev/null +++ b/lib/gitlab/markdown_cache/redis/store.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Gitlab + module MarkdownCache + module Redis + class Store + EXPIRES_IN = 1.day + + def initialize(subject) + @subject = subject + @loaded = false + end + + def save(updates) + @loaded = false + + Gitlab::Redis::Cache.with do |r| + r.mapped_hmset(markdown_cache_key, updates) + r.expire(markdown_cache_key, EXPIRES_IN) + end + end + + def read + @loaded = true + + results = Gitlab::Redis::Cache.with do |r| + r.mapped_hmget(markdown_cache_key, *fields) + end + # The value read from redis is a string, so we're converting it back + # to an int. + results[:cached_markdown_version] = results[:cached_markdown_version].to_i + results + end + + def loaded? + @loaded + end + + private + + def fields + @fields ||= @subject.cached_markdown_fields.html_fields + [:cached_markdown_version] + end + + def markdown_cache_key + unless @subject.respond_to?(:id) + raise Gitlab::MarkdownCache::UnsupportedClassError, + "This class has no id to use for caching" + end + + "markdown_cache:#{@subject.class}:#{@subject.id}" + end + end + end + end +end diff --git a/spec/features/markdown/gitlab_flavored_markdown_spec.rb b/spec/features/markdown/gitlab_flavored_markdown_spec.rb index 6997ca48427..8fda3c7193e 100644 --- a/spec/features/markdown/gitlab_flavored_markdown_spec.rb +++ b/spec/features/markdown/gitlab_flavored_markdown_spec.rb @@ -20,8 +20,7 @@ describe "GitLab Flavored Markdown" do let(:commit) { project.commit } before do - allow_any_instance_of(Commit).to receive(:title) - .and_return("fix #{issue.to_reference}\n\nask #{fred.to_reference} for details") + create_commit("fix #{issue.to_reference}\n\nask #{fred.to_reference} for details", project, user, 'master') end it "renders title in commits#index" do diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index c3956ba08fd..597c8f836a9 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -78,7 +78,8 @@ describe MarkupHelper do let(:link) { '/commits/0a1b2c3d' } let(:issues) { create_list(:issue, 2, project: project) } - it 'handles references nested in links with all the text' do + # Clean the cache to make sure the title is re-rendered from the stubbed one + it 'handles references nested in links with all the text', :clean_gitlab_redis_cache do allow(commit).to receive(:title).and_return("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real") actual = helper.link_to_markdown_field(commit, :title, link) diff --git a/spec/lib/banzai/commit_renderer_spec.rb b/spec/lib/banzai/commit_renderer_spec.rb index 1f53657c59c..316dbf052c3 100644 --- a/spec/lib/banzai/commit_renderer_spec.rb +++ b/spec/lib/banzai/commit_renderer_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Banzai::CommitRenderer do - describe '.render' do + describe '.render', :clean_gitlab_redis_cache do it 'renders a commit description and title' do user = build(:user) project = create(:project, :repository) @@ -13,7 +13,7 @@ describe Banzai::CommitRenderer do described_class::ATTRIBUTES.each do |attr| expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original - expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, {}) + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, { skip_project_check: false }).and_call_original end described_class.render([project.commit], project, user) diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index 3b52f6666d0..7b855251a74 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -11,7 +11,7 @@ describe Banzai::ObjectRenderer do ) end - let(:object) { Note.new(note: 'hello', note_html: '

hello

', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) } + let(:object) { Note.new(note: 'hello', note_html: '

hello

', cached_markdown_version: Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) } describe '#render' do context 'with cache' do @@ -60,24 +60,38 @@ describe Banzai::ObjectRenderer do end context 'without cache' do - let(:commit) { project.commit } + let(:cacheless_class) do + Class.new do + attr_accessor :title, :redacted_title_html, :project + + def banzai_render_context(field) + { project: project, pipeline: :single_line } + end + end + end + let(:cacheless_thing) do + cacheless_class.new.tap do |thing| + thing.title = "Merge branch 'branch-merged' into 'master'" + thing.project = project + end + end it 'renders and redacts an Array of objects' do - renderer.render([commit], :title) + renderer.render([cacheless_thing], :title) - expect(commit.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'") + expect(cacheless_thing.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'") end it 'calls Banzai::Redactor to perform redaction' do expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original - renderer.render([commit], :title) + renderer.render([cacheless_thing], :title) end it 'retrieves field content using Banzai::Renderer.cacheless_render_field' do - expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title, {}).and_call_original + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(cacheless_thing, :title, {}).and_call_original - renderer.render([commit], :title) + renderer.render([cacheless_thing], :title) end end end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index 650cecfc778..aa828e2f0e9 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -11,16 +11,24 @@ describe Banzai::Renderer do object end + def fake_cacheless_object + object = double('cacheless object') + + allow(object).to receive(:respond_to?).with(:cached_markdown_fields).and_return(false) + + object + end + describe '#render_field' do let(:renderer) { described_class } context 'without cache' do - let(:commit) { create(:project, :repository).commit } + let(:commit) { fake_cacheless_object } it 'returns cacheless render field' do - expect(renderer).to receive(:cacheless_render_field).with(commit, :title, {}) + expect(renderer).to receive(:cacheless_render_field).with(commit, :field, {}) - renderer.render_field(commit, :title) + renderer.render_field(commit, :field) end end diff --git a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb new file mode 100644 index 00000000000..6700b53e790 --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb @@ -0,0 +1,177 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::MarkdownCache::ActiveRecord::Extension do + class ARThingWithMarkdownFields < ActiveRecord::Base + self.table_name = 'issues' + include CacheMarkdownField + cache_markdown_field :title, whitelisted: true + cache_markdown_field :description, pipeline: :single_line + + attr_accessor :author, :project + end + + let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } + let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + + let(:markdown) { '`Foo`' } + let(:html) { '

Foo

' } + + let(:updated_markdown) { '`Bar`' } + let(:updated_html) { '

Bar

' } + + context 'an unchanged markdown field' do + let(:thing) { ARThingWithMarkdownFields.new(title: markdown) } + + before do + thing.title = thing.title + thing.save + end + + it { expect(thing.title).to eq(markdown) } + it { expect(thing.title_html).to eq(html) } + it { expect(thing.title_html_changed?).not_to be_truthy } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'a changed markdown field' do + let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + + before do + thing.title = updated_markdown + thing.save + end + + it { expect(thing.title_html).to eq(updated_html) } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'when a markdown field is set repeatedly to an empty string' do + it do + expect(thing).to receive(:refresh_markdown_cache).once + thing.title = '' + thing.save + thing.title = '' + thing.save + end + end + + context 'when a markdown field is set repeatedly to a string which renders as empty html' do + it do + expect(thing).to receive(:refresh_markdown_cache).once + thing.title = '[//]: # (This is also a comment.)' + thing.save + thing.title = '[//]: # (This is also a comment.)' + thing.save + end + end + + context 'a non-markdown field changed' do + let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + + before do + thing.state = 'closed' + thing.save + end + + it { expect(thing.state).to eq('closed') } + it { expect(thing.title).to eq(markdown) } + it { expect(thing.title_html).to eq(html) } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'version is out of date' do + let(:thing) { ARThingWithMarkdownFields.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + + before do + thing.save + end + + it { expect(thing.title_html).to eq(updated_html) } + it { expect(thing.cached_markdown_version).to eq(cache_version) } + end + + context 'when an invalidating field is changed' do + it 'invalidates the cache when project changes' do + thing.project = :new_project + allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + + thing.save + + expect(thing.title_html).to eq(updated_html) + expect(thing.description_html).to eq(updated_html) + expect(thing.cached_markdown_version).to eq(cache_version) + end + + it 'invalidates the cache when author changes' do + thing.author = :new_author + allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + + thing.save + + expect(thing.title_html).to eq(updated_html) + expect(thing.description_html).to eq(updated_html) + expect(thing.cached_markdown_version).to eq(cache_version) + end + end + + describe '.attributes' do + it 'excludes cache attributes that is blacklisted by default' do + expect(thing.attributes.keys.sort).not_to include(%w[description_html]) + end + end + + describe '#cached_html_up_to_date?' do + let(:thing) { ARThingWithMarkdownFields.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + subject { thing.cached_html_up_to_date?(:title) } + + it 'returns false if markdown has been changed but html has not' do + thing.title = "changed!" + + is_expected.to be_falsy + end + + it 'returns true if markdown has not been changed but html has' do + thing.title_html = updated_html + + is_expected.to be_truthy + end + + it 'returns true if markdown and html have both been changed' do + thing.title = updated_markdown + thing.title_html = updated_html + + is_expected.to be_truthy + end + + it 'returns false if the markdown field is set but the html is not' do + thing.title_html = nil + + is_expected.to be_falsy + end + end + + describe '#refresh_markdown_cache!' do + before do + thing.title = updated_markdown + end + + it 'skips saving if not persisted' do + expect(thing).to receive(:persisted?).and_return(false) + expect(thing).not_to receive(:update_columns) + + thing.refresh_markdown_cache! + end + + it 'saves the changes' do + expect(thing).to receive(:persisted?).and_return(true) + + expect(thing).to receive(:update_columns) + .with("title_html" => updated_html, + "description_html" => "", + "cached_markdown_version" => cache_version) + + thing.refresh_markdown_cache! + end + end +end diff --git a/spec/lib/gitlab/markdown_cache/field_data_spec.rb b/spec/lib/gitlab/markdown_cache/field_data_spec.rb new file mode 100644 index 00000000000..393bf85aa43 --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/field_data_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +describe Gitlab::MarkdownCache::FieldData do + subject(:field_data) { described_class.new } + + before do + field_data[:description] = { project: double('project in context') } + end + + it 'translates a markdown field name into a html field name' do + expect(field_data.html_field(:description)).to eq("description_html") + end +end diff --git a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb new file mode 100644 index 00000000000..d3d3cd6f03c --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::MarkdownCache::Redis::Extension, :clean_gitlab_redis_cache do + class ThingWithMarkdownFields + include CacheMarkdownField + + def initialize(title: nil, description: nil) + @title, @description = title, description + end + + attr_reader :title, :description + + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description + + def id + "test-markdown-cache" + end + end + + let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } + let(:thing) { ThingWithMarkdownFields.new(title: "`Hello`", description: "`World`") } + let(:expected_cache_key) { "markdown_cache:ThingWithMarkdownFields:test-markdown-cache" } + + it 'defines the html attributes' do + thing = ThingWithMarkdownFields.new + + expect(thing).to respond_to(:title_html, :description_html, :cached_markdown_version) + end + + it 'loads the markdown from the cache only once' do + expect(thing).to receive(:load_cached_markdown).once.and_call_original + + thing.title_html + thing.description_html + end + + it 'correctly loads the markdown if it was stored in redis' do + Gitlab::Redis::Cache.with do |r| + r.mapped_hmset(expected_cache_key, + title_html: 'hello', + description_html: 'world', + cached_markdown_version: cache_version) + end + + expect(thing.title_html).to eq('hello') + expect(thing.description_html).to eq('world') + expect(thing.cached_markdown_version).to eq(cache_version) + end + + describe "#refresh_markdown_cache!" do + it "stores the value in redis" do + expected_results = { "title_html" => "`Hello`", + "description_html" => "

World

", + "cached_markdown_version" => cache_version.to_s } + + thing.refresh_markdown_cache! + + results = Gitlab::Redis::Cache.with do |r| + r.mapped_hmget(expected_cache_key, + "title_html", "description_html", "cached_markdown_version") + end + + expect(results).to eq(expected_results) + end + + it "assigns the values" do + thing.refresh_markdown_cache! + + expect(thing.title_html).to eq('`Hello`') + expect(thing.description_html).to eq("

World

") + expect(thing.cached_markdown_version).to eq(cache_version) + end + end +end diff --git a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb new file mode 100644 index 00000000000..59c038cfb2f --- /dev/null +++ b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::MarkdownCache::Redis::Store, :clean_gitlab_redis_cache do + let(:storable_class) do + Class.new do + cattr_reader :cached_markdown_fields do + Gitlab::MarkdownCache::FieldData.new.tap do |field_data| + field_data[:field_1] = {} + field_data[:field_2] = {} + end + end + + attr_accessor :field_1, :field_2, :field_1_html, :field_2_html, :cached_markdown_version + + def id + 'test-redisbacked-store' + end + end + end + let(:storable) { storable_class.new } + let(:cache_key) { "markdown_cache:#{storable_class}:#{storable.id}" } + + subject(:store) { described_class.new(storable) } + + def read_values + Gitlab::Redis::Cache.with do |r| + r.mapped_hmget(cache_key, + :field_1_html, :field_2_html, :cached_markdown_version) + end + end + + def store_values(values) + Gitlab::Redis::Cache.with do |r| + r.mapped_hmset(cache_key, + values) + end + end + + describe '#save' do + it 'stores updates to html fields and version' do + values_to_store = { field_1_html: "hello", field_2_html: "world", cached_markdown_version: 1 } + + store.save(values_to_store) + + expect(read_values) + .to eq({ field_1_html: "hello", field_2_html: "world", cached_markdown_version: "1" }) + end + end + + describe '#read' do + it 'reads the html fields and version from redis if they were stored' do + stored_values = { field_1_html: "hello", field_2_html: "world", cached_markdown_version: 1 } + + store_values(stored_values) + + expect(store.read.symbolize_keys).to eq(stored_values) + end + + it 'is mared loaded after reading' do + expect(store).not_to be_loaded + + store.read + + expect(store).to be_loaded + end + end +end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 78637ff10c6..52f8b052ad4 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -2,383 +2,213 @@ require 'spec_helper' -describe CacheMarkdownField do - # The minimum necessary ActiveModel to test this concern - class ThingWithMarkdownFields - include ActiveModel::Model - include ActiveModel::Dirty - - include ActiveModel::Serialization - - class_attribute :attribute_names - self.attribute_names = [] - - def attributes - attribute_names.each_with_object({}) do |name, hsh| - hsh[name.to_s] = send(name) - end +describe CacheMarkdownField, :clean_gitlab_redis_cache do + let(:ar_class) do + Class.new(ActiveRecord::Base) do + self.table_name = 'issues' + include CacheMarkdownField + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description end + end - extend ActiveModel::Callbacks - define_model_callbacks :create, :update - - include CacheMarkdownField - cache_markdown_field :foo - cache_markdown_field :baz, pipeline: :single_line - cache_markdown_field :zoo, whitelisted: true + let(:other_class) do + Class.new do + include CacheMarkdownField - def self.add_attr(name) - self.attribute_names += [name] - define_attribute_methods(name) - attr_reader(name) - define_method("#{name}=") do |value| - write_attribute(name, value) + def initialize(args = {}) + @title, @description, @cached_markdown_version = args[:title], args[:description], args[:cached_markdown_version] + @title_html, @description_html = args[:title_html], args[:description_html] + @author, @project = args[:author], args[:project] end - end - add_attr :cached_markdown_version + attr_accessor :title, :description, :cached_markdown_version - [:foo, :foo_html, :bar, :baz, :baz_html, :zoo, :zoo_html].each do |name| - add_attr(name) - end - - def initialize(*) - super - - # Pretend new is load - clear_changes_information - end - - def read_attribute(name) - instance_variable_get("@#{name}") - end - - def write_attribute(name, value) - send("#{name}_will_change!") unless value == read_attribute(name) - instance_variable_set("@#{name}", value) - end + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description - def save - run_callbacks :update do - changes_applied + def id + "test-markdown-cache" end end - - def has_attribute?(attr_name) - attribute_names.include?(attr_name) - end - end - - def thing_subclass(new_attr) - Class.new(ThingWithMarkdownFields) { add_attr(new_attr) } end let(:markdown) { '`Foo`' } - let(:html) { '

Foo

' } + let(:html) { '

Foo

' } let(:updated_markdown) { '`Bar`' } - let(:updated_html) { '

Bar

' } - - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) } - let(:cache_version) { CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 } - - before do - stub_commonmark_sourcepos_disabled - end + let(:updated_html) { '

Bar

' } - describe '.attributes' do - it 'excludes cache attributes that is blacklisted by default' do - expect(thing.attributes.keys.sort).to eq(%w[bar baz cached_markdown_version foo zoo zoo_html]) - end - end - - context 'an unchanged markdown field' do - before do - thing.foo = thing.foo - thing.save - end + let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - it { expect(thing.foo).to eq(markdown) } - it { expect(thing.foo_html).to eq(html) } - it { expect(thing.foo_html_changed?).not_to be_truthy } - it { expect(thing.cached_markdown_version).to eq(cache_version) } + def thing_subclass(klass, extra_attribute) + Class.new(klass) { attr_accessor(extra_attribute) } end - context 'a changed markdown field' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) } + shared_examples 'a class with cached markdown fields' do + describe '#cached_html_up_to_date?' do + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } - before do - thing.foo = updated_markdown - thing.save - end + subject { thing.cached_html_up_to_date?(:title) } - it { expect(thing.foo_html).to eq(updated_html) } - it { expect(thing.cached_markdown_version).to eq(cache_version) } - end + it 'returns false when the version is absent' do + thing.cached_markdown_version = nil - context 'when a markdown field is set repeatedly to an empty string' do - it do - expect(thing).to receive(:refresh_markdown_cache).once - thing.foo = '' - thing.save - thing.foo = '' - thing.save - end - end - - context 'when a markdown field is set repeatedly to a string which renders as empty html' do - it do - expect(thing).to receive(:refresh_markdown_cache).once - thing.foo = '[//]: # (This is also a comment.)' - thing.save - thing.foo = '[//]: # (This is also a comment.)' - thing.save - end - end - - context 'when a markdown field and html field are both changed' do - it do - expect(thing).not_to receive(:refresh_markdown_cache) - thing.foo = '_look over there!_' - thing.foo_html = 'look over there!' - thing.save - end - end - - context 'a non-markdown field changed' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) } - - before do - thing.bar = 'OK' - thing.save - end - - it { expect(thing.bar).to eq('OK') } - it { expect(thing.foo).to eq(markdown) } - it { expect(thing.foo_html).to eq(html) } - it { expect(thing.cached_markdown_version).to eq(cache_version) } - end - - context 'version is out of date' do - let(:thing) { ThingWithMarkdownFields.new(foo: updated_markdown, foo_html: html, cached_markdown_version: nil) } - - before do - thing.save - end - - it { expect(thing.foo_html).to eq(updated_html) } - it { expect(thing.cached_markdown_version).to eq(cache_version) } - end - - describe '#cached_html_up_to_date?' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) } - - subject { thing.cached_html_up_to_date?(:foo) } - - it 'returns false when the version is absent' do - thing.cached_markdown_version = nil - - is_expected.to be_falsy - end - - it 'returns false when the cached version is too old' do - thing.cached_markdown_version = cache_version - 1 - - is_expected.to be_falsy - end - - it 'returns false when the cached version is in future' do - thing.cached_markdown_version = cache_version + 1 - - is_expected.to be_falsy - end - - it 'returns false when the local version was bumped' do - allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) - thing.cached_markdown_version = cache_version - - is_expected.to be_falsy - end + is_expected.to be_falsy + end - it 'returns true when the local version is default' do - thing.cached_markdown_version = cache_version + it 'returns false when the version is too early' do + thing.cached_markdown_version -= 1 - is_expected.to be_truthy - end + is_expected.to be_falsy + end - it 'returns true when the cached version is just right' do - allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) - thing.cached_markdown_version = cache_version + 2 + it 'returns false when the version is too late' do + thing.cached_markdown_version += 1 - is_expected.to be_truthy - end + is_expected.to be_falsy + end - it 'returns false if markdown has been changed but html has not' do - thing.foo = updated_html + it 'returns false when the local version was bumped' do + allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) + thing.cached_markdown_version = cache_version - is_expected.to be_falsy - end + is_expected.to be_falsy + end - it 'returns true if markdown has not been changed but html has' do - thing.foo_html = updated_html + it 'returns true when the local version is default' do + thing.cached_markdown_version = cache_version - is_expected.to be_truthy - end + is_expected.to be_truthy + end - it 'returns true if markdown and html have both been changed' do - thing.foo = updated_markdown - thing.foo_html = updated_html + it 'returns true when the cached version is just right' do + allow(Gitlab::CurrentSettings.current_application_settings).to receive(:local_markdown_version).and_return(2) + thing.cached_markdown_version = cache_version + 2 - is_expected.to be_truthy + is_expected.to be_truthy + end end - it 'returns false if the markdown field is set but the html is not' do - thing.foo_html = nil + describe '#latest_cached_markdown_version' do + let(:thing) { klass.new } + subject { thing.latest_cached_markdown_version } - is_expected.to be_falsy + it 'returns default version' do + thing.cached_markdown_version = nil + is_expected.to eq(cache_version) + end end - end - - describe '#latest_cached_markdown_version' do - subject { thing.latest_cached_markdown_version } - it 'returns default version' do - thing.cached_markdown_version = nil - is_expected.to eq(cache_version) - end - end + describe '#refresh_markdown_cache' do + let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) } - describe '#refresh_markdown_cache' do - before do - thing.foo = updated_markdown - end + before do + thing.description = updated_markdown + end - it 'fills all html fields' do - thing.refresh_markdown_cache + it 'fills all html fields' do + thing.refresh_markdown_cache - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + expect(thing.description_html).to eq(updated_html) + end - it 'does not save the result' do - expect(thing).not_to receive(:update_columns) + it 'does not save the result' do + expect(thing).not_to receive(:save_markdown) - thing.refresh_markdown_cache - end + thing.refresh_markdown_cache + end - it 'updates the markdown cache version' do - thing.cached_markdown_version = nil - thing.refresh_markdown_cache + it 'updates the markdown cache version' do + thing.cached_markdown_version = nil + thing.refresh_markdown_cache - expect(thing.cached_markdown_version).to eq(cache_version) + expect(thing.cached_markdown_version).to eq(cache_version) + end end - end - - describe '#refresh_markdown_cache!' do - let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) } - before do - thing.foo = updated_markdown - end + describe '#refresh_markdown_cache!' do + let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) } - it 'fills all html fields' do - thing.refresh_markdown_cache! + before do + thing.description = updated_markdown + end - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + it 'fills all html fields' do + thing.refresh_markdown_cache! - it 'skips saving if not persisted' do - expect(thing).to receive(:persisted?).and_return(false) - expect(thing).not_to receive(:update_columns) + expect(thing.description_html).to eq(updated_html) + end - thing.refresh_markdown_cache! - end + it 'saves the changes' do + expect(thing) + .to receive(:save_markdown) + .with("description_html" => updated_html, "title_html" => "", "cached_markdown_version" => cache_version) - it 'saves the changes using #update_columns' do - expect(thing).to receive(:persisted?).and_return(true) - expect(thing).to receive(:update_columns) - .with( - "foo_html" => updated_html, - "baz_html" => "", - "zoo_html" => "", - "cached_markdown_version" => cache_version - ) - - thing.refresh_markdown_cache! + thing.refresh_markdown_cache! + end end - end - describe '#banzai_render_context' do - subject(:context) { thing.banzai_render_context(:foo) } + describe '#banzai_render_context' do + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + subject(:context) { thing.banzai_render_context(:title) } - it 'sets project to nil if the object lacks a project' do - is_expected.to have_key(:project) - expect(context[:project]).to be_nil - end + it 'sets project to nil if the object lacks a project' do + is_expected.to have_key(:project) + expect(context[:project]).to be_nil + end - it 'excludes author if the object lacks an author' do - is_expected.not_to have_key(:author) - end + it 'excludes author if the object lacks an author' do + is_expected.not_to have_key(:author) + end - it 'raises if the context for an unrecognised field is requested' do - expect { thing.banzai_render_context(:not_found) }.to raise_error(ArgumentError) - end + it 'raises if the context for an unrecognised field is requested' do + expect { thing.banzai_render_context(:not_found) }.to raise_error(ArgumentError) + end - it 'includes the pipeline' do - baz = thing.banzai_render_context(:baz) + it 'includes the pipeline' do + title_context = thing.banzai_render_context(:title) - expect(baz[:pipeline]).to eq(:single_line) - end + expect(title_context[:pipeline]).to eq(:single_line) + end - it 'returns copies of the context template' do - template = thing.cached_markdown_fields[:baz] - copy = thing.banzai_render_context(:baz) + it 'returns copies of the context template' do + template = thing.cached_markdown_fields[:description] + copy = thing.banzai_render_context(:description) - expect(copy).not_to be(template) - end + expect(copy).not_to be(template) + end - context 'with a project' do - let(:project) { create(:project, group: create(:group)) } - let(:thing) { thing_subclass(:project).new(foo: markdown, foo_html: html, project: project) } + context 'with a project' do + let(:project) { build(:project, group: create(:group)) } + let(:thing) { thing_subclass(klass, :project).new(title: markdown, title_html: html, project: project) } - it 'sets the project in the context' do - is_expected.to have_key(:project) - expect(context[:project]).to eq(project) + it 'sets the project in the context' do + is_expected.to have_key(:project) + expect(context[:project]).to eq(project) + end end - it 'invalidates the cache when project changes' do - thing.project = :new_project - allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) - - thing.save + context 'with an author' do + let(:thing) { thing_subclass(klass, :author).new(title: markdown, title_html: html, author: :author_value) } - expect(thing.foo_html).to eq(updated_html) - expect(thing.baz_html).to eq(updated_html) - expect(thing.cached_markdown_version).to eq(cache_version) + it 'sets the author in the context' do + is_expected.to have_key(:author) + expect(context[:author]).to eq(:author_value) + end end end + end - context 'with an author' do - let(:thing) { thing_subclass(:author).new(foo: markdown, foo_html: html, author: :author_value) } - - it 'sets the author in the context' do - is_expected.to have_key(:author) - expect(context[:author]).to eq(:author_value) - end + context 'for Active record classes' do + let(:klass) { ar_class } - it 'invalidates the cache when author changes' do - thing.author = :new_author - allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html) + it_behaves_like 'a class with cached markdown fields' + end - thing.save + context 'for other classes' do + let(:klass) { other_class } - expect(thing.foo_html).to eq(updated_html) - expect(thing.baz_html).to eq(updated_html) - expect(thing.cached_markdown_version).to eq(cache_version) - end - end + it_behaves_like 'a class with cached markdown fields' end end diff --git a/spec/models/resource_label_event_spec.rb b/spec/models/resource_label_event_spec.rb index 7eeb2fae57d..cb52f154299 100644 --- a/spec/models/resource_label_event_spec.rb +++ b/spec/models/resource_label_event_spec.rb @@ -82,13 +82,13 @@ RSpec.describe ResourceLabelEvent, type: :model do end it 'returns true if markdown is outdated' do - subject.attributes = { cached_markdown_version: ((CacheMarkdownField::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 } + subject.attributes = { cached_markdown_version: ((Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 } expect(subject.outdated_markdown?).to be true end it 'returns false if label and reference are set' do - subject.attributes = { reference: 'whatever', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 } + subject.attributes = { reference: 'whatever', cached_markdown_version: Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } expect(subject.outdated_markdown?).to be false end -- cgit v1.2.1 From 454aa409e38b5b253c077e3f72d6c9addbf35339 Mon Sep 17 00:00:00 2001 From: Lukas 'Eipi' Eipert Date: Wed, 5 Jun 2019 06:27:53 +0000 Subject: Update dependency @gitlab/ui to ^3.11.0 --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 3599ce2c279..ab7268642ec 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "@babel/preset-env": "^7.4.4", "@gitlab/csslab": "^1.9.0", "@gitlab/svgs": "^1.63.0", - "@gitlab/ui": "^3.10.3", + "@gitlab/ui": "^3.11.0", "apollo-cache-inmemory": "^1.5.1", "apollo-client": "^2.5.1", "apollo-link": "^1.2.11", diff --git a/yarn.lock b/yarn.lock index 6906d6af89f..b6d31ea08e6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -698,10 +698,10 @@ resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.63.0.tgz#9dd544026d203e4ce6efed72b05db68f710c4d49" integrity sha512-YztrReFTg31B7v5wtUC5j15KHNcMebtW+kACytEU42XomMaIwk4USIbygqWlq0VRHA2VHJrHApfJHIjxiCCQcA== -"@gitlab/ui@^3.10.3": - version "3.10.3" - resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-3.10.3.tgz#dba2ddc726e203ab341d870cea2fe634f583c08d" - integrity sha512-Y48DKhOSC+Yw0X8PN+TyR8gITAq6jVHbiTsw+eZkCacs367L1u6w82lr7ba/Bl+4W6DhDWT34VCG0hYbGVrUJw== +"@gitlab/ui@^3.11.0": + version "3.11.0" + resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-3.11.0.tgz#7bba82c893f47abbfe7995281dc0ce95290dcc4e" + integrity sha512-55Qxyj2wZILznZJUTUxY1SUuw028IgmP6ZyLd5XF3xk91HWSyq5/zrlr/qRTFGL1cABhxoBLScmXsnOc2CIO0w== dependencies: "@babel/standalone" "^7.0.0" "@gitlab/vue-toasted" "^1.2.1" -- cgit v1.2.1 From 56d52340da0f8f15179b83f1206544a2590c22ff Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 5 Jun 2019 13:50:37 +0800 Subject: Use #cache_key of subject for generated redis key This commit also includes some changes in specs to use `Class.new` approach. --- lib/gitlab/markdown_cache/redis/store.rb | 2 +- .../markdown_cache/active_record/extension_spec.rb | 26 ++++++++++-------- .../gitlab/markdown_cache/redis/extension_spec.rb | 32 ++++++++++++---------- spec/lib/gitlab/markdown_cache/redis/store_spec.rb | 6 +++- spec/models/concerns/cache_markdown_field_spec.rb | 4 +++ 5 files changed, 42 insertions(+), 28 deletions(-) diff --git a/lib/gitlab/markdown_cache/redis/store.rb b/lib/gitlab/markdown_cache/redis/store.rb index a35eebc6a1b..2d4a89d9f1c 100644 --- a/lib/gitlab/markdown_cache/redis/store.rb +++ b/lib/gitlab/markdown_cache/redis/store.rb @@ -48,7 +48,7 @@ module Gitlab "This class has no id to use for caching" end - "markdown_cache:#{@subject.class}:#{@subject.id}" + "markdown_cache:#{@subject.cache_key}" end end end diff --git a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb index 6700b53e790..18052b1991c 100644 --- a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb +++ b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb @@ -2,17 +2,19 @@ require 'spec_helper' describe Gitlab::MarkdownCache::ActiveRecord::Extension do - class ARThingWithMarkdownFields < ActiveRecord::Base - self.table_name = 'issues' - include CacheMarkdownField - cache_markdown_field :title, whitelisted: true - cache_markdown_field :description, pipeline: :single_line + let(:klass) do + Class.new(ActiveRecord::Base) do + self.table_name = 'issues' + include CacheMarkdownField + cache_markdown_field :title, whitelisted: true + cache_markdown_field :description, pipeline: :single_line - attr_accessor :author, :project + attr_accessor :author, :project + end end let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } let(:markdown) { '`Foo`' } let(:html) { '

Foo

' } @@ -21,7 +23,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do let(:updated_html) { '

Bar

' } context 'an unchanged markdown field' do - let(:thing) { ARThingWithMarkdownFields.new(title: markdown) } + let(:thing) { klass.new(title: markdown) } before do thing.title = thing.title @@ -35,7 +37,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'a changed markdown field' do - let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } before do thing.title = updated_markdown @@ -67,7 +69,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'a non-markdown field changed' do - let(:thing) { ARThingWithMarkdownFields.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } + let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) } before do thing.state = 'closed' @@ -81,7 +83,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end context 'version is out of date' do - let(:thing) { ARThingWithMarkdownFields.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + let(:thing) { klass.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) } before do thing.save @@ -122,7 +124,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do end describe '#cached_html_up_to_date?' do - let(:thing) { ARThingWithMarkdownFields.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) } + let(:thing) { klass.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) } subject { thing.cached_html_up_to_date?(:title) } it 'returns false if markdown has been changed but html has not' do diff --git a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb index d3d3cd6f03c..24cfb399cc3 100644 --- a/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb +++ b/spec/lib/gitlab/markdown_cache/redis/extension_spec.rb @@ -2,30 +2,34 @@ require 'spec_helper' describe Gitlab::MarkdownCache::Redis::Extension, :clean_gitlab_redis_cache do - class ThingWithMarkdownFields - include CacheMarkdownField + let(:klass) do + Class.new do + include CacheMarkdownField - def initialize(title: nil, description: nil) - @title, @description = title, description - end + def initialize(title: nil, description: nil) + @title, @description = title, description + end + + attr_reader :title, :description - attr_reader :title, :description + cache_markdown_field :title, pipeline: :single_line + cache_markdown_field :description - cache_markdown_field :title, pipeline: :single_line - cache_markdown_field :description + def id + "test-markdown-cache" + end - def id - "test-markdown-cache" + def cache_key + "cache-key" + end end end let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 } - let(:thing) { ThingWithMarkdownFields.new(title: "`Hello`", description: "`World`") } - let(:expected_cache_key) { "markdown_cache:ThingWithMarkdownFields:test-markdown-cache" } + let(:thing) { klass.new(title: "`Hello`", description: "`World`") } + let(:expected_cache_key) { "markdown_cache:cache-key" } it 'defines the html attributes' do - thing = ThingWithMarkdownFields.new - expect(thing).to respond_to(:title_html, :description_html, :cached_markdown_version) end diff --git a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb index 59c038cfb2f..e7701cf1306 100644 --- a/spec/lib/gitlab/markdown_cache/redis/store_spec.rb +++ b/spec/lib/gitlab/markdown_cache/redis/store_spec.rb @@ -16,10 +16,14 @@ describe Gitlab::MarkdownCache::Redis::Store, :clean_gitlab_redis_cache do def id 'test-redisbacked-store' end + + def cache_key + "cache-key" + end end end let(:storable) { storable_class.new } - let(:cache_key) { "markdown_cache:#{storable_class}:#{storable.id}" } + let(:cache_key) { "markdown_cache:#{storable.cache_key}" } subject(:store) { described_class.new(storable) } diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 52f8b052ad4..9e6b5d56805 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -30,6 +30,10 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do def id "test-markdown-cache" end + + def cache_key + "cache-key" + end end end -- cgit v1.2.1 From 7aac4e5c3f290a4ae32ef0ca332a2ddac6bff774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Budai?= Date: Wed, 5 Jun 2019 07:27:08 +0000 Subject: Fix whitespace changes visibility when the related file was initially collapsed --- app/assets/javascripts/diffs/store/actions.js | 3 ++- changelogs/unreleased/50106-hide-whitespace-changes.yml | 5 +++++ spec/javascripts/diffs/store/actions_spec.js | 11 ++++++----- 3 files changed, 13 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/50106-hide-whitespace-changes.yml diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 35297b7c264..479afc50113 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -211,11 +211,12 @@ export const scrollToLineIfNeededParallel = (_, line) => { } }; -export const loadCollapsedDiff = ({ commit, getters }, file) => +export const loadCollapsedDiff = ({ commit, getters, state }, file) => axios .get(file.load_collapsed_diff_url, { params: { commit_id: getters.commitId, + w: state.showWhitespace ? '0' : '1', }, }) .then(res => { diff --git a/changelogs/unreleased/50106-hide-whitespace-changes.yml b/changelogs/unreleased/50106-hide-whitespace-changes.yml new file mode 100644 index 00000000000..e95953c8665 --- /dev/null +++ b/changelogs/unreleased/50106-hide-whitespace-changes.yml @@ -0,0 +1,5 @@ +--- +title: Fix whitespace changes visibility when the related file was initially collapsed +merge_request: 28950 +author: Ondřej Budai +type: fixed diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index 6309a8823d7..f129fbb57a3 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -396,6 +396,7 @@ describe('DiffsStoreActions', () => { }); describe('loadCollapsedDiff', () => { + const state = { showWhitespace: true }; it('should fetch data and call mutation with response and the give parameter', done => { const file = { hash: 123, load_collapsed_diff_url: '/load/collapsed/diff/url' }; const data = { hash: 123, parallelDiffLines: [{ lineCode: 1 }] }; @@ -403,7 +404,7 @@ describe('DiffsStoreActions', () => { const commit = jasmine.createSpy('commit'); mock.onGet(file.loadCollapsedDiffUrl).reply(200, data); - loadCollapsedDiff({ commit, getters: { commitId: null } }, file) + loadCollapsedDiff({ commit, getters: { commitId: null }, state }, file) .then(() => { expect(commit).toHaveBeenCalledWith(types.ADD_COLLAPSED_DIFFS, { file, data }); @@ -421,10 +422,10 @@ describe('DiffsStoreActions', () => { spyOn(axios, 'get').and.returnValue(Promise.resolve({ data: {} })); - loadCollapsedDiff({ commit() {}, getters }, file); + loadCollapsedDiff({ commit() {}, getters, state }, file); expect(axios.get).toHaveBeenCalledWith(file.load_collapsed_diff_url, { - params: { commit_id: null }, + params: { commit_id: null, w: '0' }, }); }); @@ -436,10 +437,10 @@ describe('DiffsStoreActions', () => { spyOn(axios, 'get').and.returnValue(Promise.resolve({ data: {} })); - loadCollapsedDiff({ commit() {}, getters }, file); + loadCollapsedDiff({ commit() {}, getters, state }, file); expect(axios.get).toHaveBeenCalledWith(file.load_collapsed_diff_url, { - params: { commit_id: '123' }, + params: { commit_id: '123', w: '0' }, }); }); }); -- cgit v1.2.1 From 4644a2daf5ec5e86e2b2989f04e99e4f081f6fef Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 4 Jun 2019 14:38:18 +0100 Subject: Add web_url to tree entry in GraphQL API --- .../repository/components/table/index.vue | 1 + .../repository/components/table/row.vue | 7 ++++- .../repository/queries/getFiles.graphql | 2 ++ app/graphql/types/tree/blob_type.rb | 4 +++ app/graphql/types/tree/tree_entry_type.rb | 4 +++ app/graphql/types/tree/tree_type.rb | 10 +++++-- app/presenters/blob_presenter.rb | 4 +++ app/presenters/tree_entry_presenter.rb | 9 +++++++ lib/gitlab/graphql/representation/tree_entry.rb | 31 ++++++++++++++++++++++ .../repository/components/table/row_spec.js | 12 +++++++++ spec/graphql/types/tree/blob_type_spec.rb | 2 +- spec/graphql/types/tree/tree_entry_type_spec.rb | 2 +- .../graphql/representation/tree_entry_spec.rb | 20 ++++++++++++++ spec/presenters/blob_presenter_spec.rb | 10 +++++++ spec/presenters/tree_entry_presenter_spec.rb | 16 +++++++++++ 15 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 app/presenters/tree_entry_presenter.rb create mode 100644 lib/gitlab/graphql/representation/tree_entry.rb create mode 100644 spec/lib/gitlab/graphql/representation/tree_entry_spec.rb create mode 100644 spec/presenters/tree_entry_presenter_spec.rb diff --git a/app/assets/javascripts/repository/components/table/index.vue b/app/assets/javascripts/repository/components/table/index.vue index cccde1bb278..d2198bcccfe 100644 --- a/app/assets/javascripts/repository/components/table/index.vue +++ b/app/assets/javascripts/repository/components/table/index.vue @@ -134,6 +134,7 @@ export default { :current-path="path" :path="entry.flatPath" :type="entry.type" + :url="entry.webUrl" /> diff --git a/app/assets/javascripts/repository/components/table/row.vue b/app/assets/javascripts/repository/components/table/row.vue index 9a264bef87e..764882a7936 100644 --- a/app/assets/javascripts/repository/components/table/row.vue +++ b/app/assets/javascripts/repository/components/table/row.vue @@ -21,6 +21,11 @@ export default { type: String, required: true, }, + url: { + type: String, + required: false, + default: null, + }, }, computed: { routerLinkTo() { @@ -59,7 +64,7 @@ export default { - + {{ fullPath }}