diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-10-27 21:11:02 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-10-27 21:11:02 +0300 |
commit | bd746eebdc82ea3731b38cd903a999569ff3b8be (patch) | |
tree | a5b9a018e89a20f53de13055bf975b62b8ccd595 /spec | |
parent | 0407f1573d1b3468f9fdcdd363996acc9d3052b1 (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'spec')
11 files changed, 873 insertions, 796 deletions
diff --git a/spec/features/admin/admin_dev_ops_reports_spec.rb b/spec/features/admin/admin_dev_ops_reports_spec.rb index bf32819cb52..f65862c568f 100644 --- a/spec/features/admin/admin_dev_ops_reports_spec.rb +++ b/spec/features/admin/admin_dev_ops_reports_spec.rb @@ -9,9 +9,9 @@ RSpec.describe 'DevOps Report page', :js do gitlab_enable_admin_mode_sign_in(admin) end - context 'with devops_adoption feature flag disabled' do + context 'without licensed feature devops adoption' do before do - stub_feature_flags(devops_adoption: false) + stub_licensed_features(devops_adoption: false) end it 'has dismissable intro callout' do diff --git a/spec/frontend/blob_edit/edit_blob_spec.js b/spec/frontend/blob_edit/edit_blob_spec.js index c031cae11df..dda46e97b85 100644 --- a/spec/frontend/blob_edit/edit_blob_spec.js +++ b/spec/frontend/blob_edit/edit_blob_spec.js @@ -1,3 +1,4 @@ +import MockAdapter from 'axios-mock-adapter'; import { Emitter } from 'monaco-editor'; import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import waitForPromises from 'helpers/wait_for_promises'; @@ -8,6 +9,7 @@ import { EditorMarkdownExtension } from '~/editor/extensions/source_editor_markd import { EditorMarkdownPreviewExtension } from '~/editor/extensions/source_editor_markdown_livepreview_ext'; import { ToolbarExtension } from '~/editor/extensions/source_editor_toolbar_ext'; import SourceEditor from '~/editor/source_editor'; +import axios from '~/lib/utils/axios_utils'; jest.mock('~/editor/source_editor'); jest.mock('~/editor/extensions/source_editor_extension_base'); @@ -32,6 +34,7 @@ const markdownExtensions = [ describe('Blob Editing', () => { let blobInstance; + let mock; const useMock = jest.fn(() => markdownExtensions); const unuseMock = jest.fn(); const emitter = new Emitter(); @@ -44,7 +47,10 @@ describe('Blob Editing', () => { onDidChangeModelLanguage: emitter.event, }; beforeEach(() => { + mock = new MockAdapter(axios); setHTMLFixture(` + <div class="js-edit-mode-pane"></div> + <div class="js-edit-mode"><a href="#write">Write</a><a href="#preview">Preview</a></div> <form class="js-edit-blob-form"> <div id="file_path"></div> <div id="editor"></div> @@ -54,6 +60,7 @@ describe('Blob Editing', () => { jest.spyOn(SourceEditor.prototype, 'createInstance').mockReturnValue(mockInstance); }); afterEach(() => { + mock.restore(); jest.clearAllMocks(); unuseMock.mockClear(); useMock.mockClear(); @@ -108,6 +115,47 @@ describe('Blob Editing', () => { }); }); + describe('correctly handles toggling the live-preview panel for different file types', () => { + it.each` + desc | isMarkdown | isPreviewOpened | tabToClick | shouldOpenPreview | shouldClosePreview | expectedDesc + ${'not markdown with preview closed'} | ${false} | ${false} | ${'#write'} | ${false} | ${false} | ${'not toggle preview'} + ${'not markdown with preview closed'} | ${false} | ${false} | ${'#preview'} | ${false} | ${false} | ${'not toggle preview'} + ${'markdown with preview closed'} | ${true} | ${false} | ${'#write'} | ${false} | ${false} | ${'not toggle preview'} + ${'markdown with preview closed'} | ${true} | ${false} | ${'#preview'} | ${true} | ${false} | ${'open preview'} + ${'markdown with preview opened'} | ${true} | ${true} | ${'#write'} | ${false} | ${true} | ${'close preview'} + ${'markdown with preview opened'} | ${true} | ${true} | ${'#preview'} | ${false} | ${false} | ${'not toggle preview'} + `( + 'when $desc, clicking $tabToClick should $expectedDesc', + async ({ + isMarkdown, + isPreviewOpened, + tabToClick, + shouldOpenPreview, + shouldClosePreview, + }) => { + const fire = jest.fn(); + SourceEditor.prototype.createInstance = jest.fn().mockReturnValue({ + ...mockInstance, + markdownPreview: { + eventEmitter: { + fire, + }, + }, + }); + await initEditor(isMarkdown); + blobInstance.markdownLivePreviewOpened = isPreviewOpened; + const elToClick = document.querySelector(`a[href='${tabToClick}']`); + elToClick.dispatchEvent(new Event('click')); + + if (shouldOpenPreview || shouldClosePreview) { + expect(fire).toHaveBeenCalled(); + } else { + expect(fire).not.toHaveBeenCalled(); + } + }, + ); + }); + it('adds trailing newline to the blob content on submit', async () => { const form = document.querySelector('.js-edit-blob-form'); const fileContentEl = document.getElementById('file-content'); diff --git a/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js b/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js index 1ff351b6554..19ebe0e3cb7 100644 --- a/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js +++ b/spec/frontend/editor/source_editor_markdown_livepreview_ext_spec.js @@ -81,9 +81,18 @@ describe('Markdown Live Preview Extension for Source Editor', () => { }, path: previewMarkdownPath, actionShowPreviewCondition: expect.any(Object), + eventEmitter: expect.any(Object), }); }); + it('support external preview trigger via emitter event', () => { + expect(panelSpy).not.toHaveBeenCalled(); + + instance.markdownPreview.eventEmitter.fire(); + + expect(panelSpy).toHaveBeenCalled(); + }); + describe('onDidLayoutChange', () => { const emitter = new Emitter(); let layoutSpy; diff --git a/spec/frontend/invite_members/components/invite_members_modal_spec.js b/spec/frontend/invite_members/components/invite_members_modal_spec.js index e9e1fbad07b..47be1933ed7 100644 --- a/spec/frontend/invite_members/components/invite_members_modal_spec.js +++ b/spec/frontend/invite_members/components/invite_members_modal_spec.js @@ -10,12 +10,12 @@ import InviteMembersModal from '~/invite_members/components/invite_members_modal import InviteModalBase from '~/invite_members/components/invite_modal_base.vue'; import ModalConfetti from '~/invite_members/components/confetti.vue'; import MembersTokenSelect from '~/invite_members/components/members_token_select.vue'; +import UserLimitNotification from '~/invite_members/components/user_limit_notification.vue'; import { INVITE_MEMBERS_FOR_TASK, MEMBERS_MODAL_CELEBRATE_INTRO, MEMBERS_MODAL_CELEBRATE_TITLE, MEMBERS_PLACEHOLDER, - MEMBERS_PLACEHOLDER_DISABLED, MEMBERS_TO_PROJECT_CELEBRATE_INTRO_TEXT, LEARN_GITLAB, EXPANDED_ERRORS, @@ -31,8 +31,6 @@ import { propsData, inviteSource, newProjectPath, - freeUsersLimit, - membersCount, user1, user2, user3, @@ -99,6 +97,7 @@ describe('InviteMembersModal', () => { const findIntroText = () => wrapper.findByTestId('modal-base-intro-text').text(); const findMemberErrorAlert = () => wrapper.findByTestId('alert-member-error'); const findMoreInviteErrorsButton = () => wrapper.findByTestId('accordion-button'); + const findUserLimitAlert = () => wrapper.findComponent(UserLimitNotification); const findAccordion = () => wrapper.findComponent(GlCollapse); const findErrorsIcon = () => wrapper.findComponent(GlIcon); const findMemberErrorMessage = (element) => @@ -112,7 +111,7 @@ describe('InviteMembersModal', () => { const findMembersFormGroup = () => wrapper.findByTestId('members-form-group'); const membersFormGroupInvalidFeedback = () => findMembersFormGroup().attributes('invalid-feedback'); - const membersFormGroupText = () => findMembersFormGroup().text(); + const membersFormGroupDescription = () => findMembersFormGroup().attributes('description'); const findMembersSelect = () => wrapper.findComponent(MembersTokenSelect); const findTasksToBeDone = () => wrapper.findByTestId('invite-members-modal-tasks-to-be-done'); const findTasks = () => wrapper.findByTestId('invite-members-modal-tasks'); @@ -299,19 +298,8 @@ describe('InviteMembersModal', () => { describe('members form group description', () => { it('renders correct description', () => { - createInviteMembersToProjectWrapper({ freeUsersLimit, membersCount }, { GlFormGroup }); - expect(membersFormGroupText()).toContain(MEMBERS_PLACEHOLDER); - }); - - describe('when reached user limit', () => { - it('renders correct description', () => { - createInviteMembersToProjectWrapper( - { freeUsersLimit, membersCount: 5 }, - { GlFormGroup }, - ); - - expect(membersFormGroupText()).toContain(MEMBERS_PLACEHOLDER_DISABLED); - }); + createInviteMembersToProjectWrapper({ GlFormGroup }); + expect(membersFormGroupDescription()).toContain(MEMBERS_PLACEHOLDER); }); }); }); @@ -339,23 +327,10 @@ describe('InviteMembersModal', () => { describe('members form group description', () => { it('renders correct description', async () => { - createInviteMembersToProjectWrapper({ freeUsersLimit, membersCount }, { GlFormGroup }); + createInviteMembersToProjectWrapper({ GlFormGroup }); await triggerOpenModal({ mode: 'celebrate' }); - expect(membersFormGroupText()).toContain(MEMBERS_PLACEHOLDER); - }); - - describe('when reached user limit', () => { - it('renders correct description', async () => { - createInviteMembersToProjectWrapper( - { freeUsersLimit, membersCount: 5 }, - { GlFormGroup }, - ); - - await triggerOpenModal({ mode: 'celebrate' }); - - expect(membersFormGroupText()).toContain(MEMBERS_PLACEHOLDER_DISABLED); - }); + expect(membersFormGroupDescription()).toContain(MEMBERS_PLACEHOLDER); }); }); }); @@ -370,20 +345,39 @@ describe('InviteMembersModal', () => { describe('members form group description', () => { it('renders correct description', () => { - createInviteMembersToGroupWrapper({ freeUsersLimit, membersCount }, { GlFormGroup }); - expect(membersFormGroupText()).toContain(MEMBERS_PLACEHOLDER); - }); - - describe('when reached user limit', () => { - it('renders correct description', () => { - createInviteMembersToGroupWrapper({ freeUsersLimit, membersCount: 5 }, { GlFormGroup }); - expect(membersFormGroupText()).toContain(MEMBERS_PLACEHOLDER_DISABLED); - }); + createInviteMembersToGroupWrapper({ GlFormGroup }); + expect(membersFormGroupDescription()).toContain(MEMBERS_PLACEHOLDER); }); }); }); }); + describe('rendering the user limit notification', () => { + it('shows the user limit notification alert when reached limit', () => { + const usersLimitDataset = { reachedLimit: true }; + + createInviteMembersToProjectWrapper(usersLimitDataset); + + expect(findUserLimitAlert().exists()).toBe(true); + }); + + it('shows the user limit notification alert when close to dashboard limit', () => { + const usersLimitDataset = { closeToDashboardLimit: true }; + + createInviteMembersToProjectWrapper(usersLimitDataset); + + expect(findUserLimitAlert().exists()).toBe(true); + }); + + it('does not show the user limit notification alert', () => { + const usersLimitDataset = {}; + + createInviteMembersToProjectWrapper(usersLimitDataset); + + expect(findUserLimitAlert().exists()).toBe(false); + }); + }); + describe('submitting the invite form', () => { const mockInvitationsApi = (code, data) => { mock.onPost(GROUPS_INVITATIONS_PATH).reply(code, data); diff --git a/spec/frontend/invite_members/components/invite_modal_base_spec.js b/spec/frontend/invite_members/components/invite_modal_base_spec.js index b55eeb72471..aeead8809fd 100644 --- a/spec/frontend/invite_members/components/invite_modal_base_spec.js +++ b/spec/frontend/invite_members/components/invite_modal_base_spec.js @@ -18,10 +18,7 @@ import { CANCEL_BUTTON_TEXT, INVITE_BUTTON_TEXT_DISABLED, INVITE_BUTTON_TEXT, - CANCEL_BUTTON_TEXT_DISABLED, ON_SHOW_TRACK_LABEL, - ON_CLOSE_TRACK_LABEL, - ON_SUBMIT_TRACK_LABEL, } from '~/invite_members/constants'; import { propsData, membersPath, purchasePath } from '../mock_data/modal_base'; @@ -131,7 +128,9 @@ describe('InviteModalBase', () => { it('renders description', () => { createComponent({}, { GlFormGroup }); - expect(findMembersFormGroup().text()).toContain(propsData.formGroupDescription); + expect(findMembersFormGroup().attributes('description')).toContain( + propsData.formGroupDescription, + ); }); describe('when users limit is reached', () => { @@ -145,30 +144,13 @@ describe('InviteModalBase', () => { beforeEach(() => { createComponent( - { usersLimitDataset: { membersPath, purchasePath }, reachedLimit: true }, + { usersLimitDataset: { membersPath, purchasePath, reachedLimit: true } }, { GlModal, GlFormGroup }, ); }); - it('renders correct blocks', () => { - expect(findIcon().exists()).toBe(true); - expect(findDisabledInput().exists()).toBe(true); - expect(findDropdown().exists()).toBe(false); - expect(findDatepicker().exists()).toBe(false); - }); - - it('renders correct buttons', () => { - const cancelButton = findCancelButton(); - const actionButton = findActionButton(); - - expect(cancelButton.attributes('href')).toBe(purchasePath); - expect(cancelButton.text()).toBe(CANCEL_BUTTON_TEXT_DISABLED); - expect(actionButton.attributes('href')).toBe(membersPath); - expect(actionButton.text()).toBe(INVITE_BUTTON_TEXT_DISABLED); - }); - it('tracks actions', () => { - createComponent({ reachedLimit: true }, { GlFormGroup, GlModal }); + createComponent({ usersLimitDataset: { reachedLimit: true } }, { GlFormGroup, GlModal }); trackingSpy = mockTracking(undefined, wrapper.element, jest.spyOn); const modal = wrapper.findComponent(GlModal); @@ -176,37 +158,20 @@ describe('InviteModalBase', () => { modal.vm.$emit('shown'); expectTracking('render', ON_SHOW_TRACK_LABEL); - modal.vm.$emit('cancel', { preventDefault: jest.fn() }); - expectTracking('click_button', ON_CLOSE_TRACK_LABEL); - - modal.vm.$emit('primary', { preventDefault: jest.fn() }); - expectTracking('click_button', ON_SUBMIT_TRACK_LABEL); - unmockTracking(); }); - - describe('when free user namespace', () => { - it('hides cancel button', () => { - createComponent( - { - usersLimitDataset: { membersPath, purchasePath, userNamespace: true }, - reachedLimit: true, - }, - { GlModal, GlFormGroup }, - ); - - expect(findCancelButton().exists()).toBe(false); - }); - }); }); describe('when user limit is close on a personal namespace', () => { beforeEach(() => { createComponent( { - closeToLimit: true, - reachedLimit: false, - usersLimitDataset: { membersPath, userNamespace: true }, + usersLimitDataset: { + membersPath, + userNamespace: true, + closeToDashboardLimit: true, + reachedLimit: false, + }, }, { GlModal, GlFormGroup }, ); diff --git a/spec/frontend/invite_members/components/user_limit_notification_spec.js b/spec/frontend/invite_members/components/user_limit_notification_spec.js index 1ff2e86412f..2a780490468 100644 --- a/spec/frontend/invite_members/components/user_limit_notification_spec.js +++ b/spec/frontend/invite_members/components/user_limit_notification_spec.js @@ -1,8 +1,8 @@ import { GlAlert, GlSprintf } from '@gitlab/ui'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import UserLimitNotification from '~/invite_members/components/user_limit_notification.vue'; -import { REACHED_LIMIT_UPGRADE_SUGGESTION_MESSAGE } from '~/invite_members/constants'; -import { freeUsersLimit, membersCount } from '../mock_data/member_modal'; +import { REACHED_LIMIT_VARIANT, CLOSE_TO_LIMIT_VARIANT } from '~/invite_members/constants'; +import { freeUsersLimit, remainingSeats } from '../mock_data/member_modal'; const WARNING_ALERT_TITLE = 'You only have space for 2 more members in name'; @@ -10,20 +10,16 @@ describe('UserLimitNotification', () => { let wrapper; const findAlert = () => wrapper.findComponent(GlAlert); + const findTrialLink = () => wrapper.findByTestId('trial-link'); + const findUpgradeLink = () => wrapper.findByTestId('upgrade-link'); - const createComponent = ( - closeToLimit = false, - reachedLimit = false, - usersLimitDataset = {}, - props = {}, - ) => { + const createComponent = (limitVariant, usersLimitDataset = {}, props = {}) => { wrapper = shallowMountExtended(UserLimitNotification, { propsData: { - closeToLimit, - reachedLimit, + limitVariant, usersLimitDataset: { + remainingSeats, freeUsersLimit, - membersCount, newTrialRegistrationPath: 'newTrialRegistrationPath', purchasePath: 'purchasePath', ...usersLimitDataset, @@ -35,40 +31,46 @@ describe('UserLimitNotification', () => { }); }; - afterEach(() => { - wrapper.destroy(); - }); - - describe('when limit is not reached', () => { - it('renders empty block', () => { - createComponent(); - - expect(findAlert().exists()).toBe(false); - }); - }); - describe('when close to limit within a group', () => { it("renders user's limit notification", () => { - createComponent(true, false, { membersCount: 3 }); + createComponent(CLOSE_TO_LIMIT_VARIANT); const alert = findAlert(); expect(alert.attributes('title')).toEqual(WARNING_ALERT_TITLE); - expect(alert.text()).toEqual( - 'To get more members an owner of the group can start a trial or upgrade to a paid tier.', - ); + expect(alert.text()).toContain('To get more members an owner of the group can'); }); }); describe('when limit is reached', () => { it("renders user's limit notification", () => { - createComponent(true, true); + createComponent(REACHED_LIMIT_VARIANT); const alert = findAlert(); expect(alert.attributes('title')).toEqual("You've reached your 5 members limit for name"); - expect(alert.text()).toEqual(REACHED_LIMIT_UPGRADE_SUGGESTION_MESSAGE); + expect(alert.text()).toContain( + 'To invite new users to this namespace, you must remove existing users.', + ); }); }); + + describe('tracking', () => { + it.each([CLOSE_TO_LIMIT_VARIANT, REACHED_LIMIT_VARIANT])( + `has tracking attributes for %j variant`, + (variant) => { + createComponent(variant); + + expect(findTrialLink().attributes('data-track-action')).toBe('click_link'); + expect(findTrialLink().attributes('data-track-label')).toBe( + `start_trial_user_limit_notification_${variant}`, + ); + expect(findUpgradeLink().attributes('data-track-action')).toBe('click_link'); + expect(findUpgradeLink().attributes('data-track-label')).toBe( + `upgrade_user_limit_notification_${variant}`, + ); + }, + ); + }); }); diff --git a/spec/frontend/invite_members/mock_data/member_modal.js b/spec/frontend/invite_members/mock_data/member_modal.js index 4f4e9345e46..59d58f21bb0 100644 --- a/spec/frontend/invite_members/mock_data/member_modal.js +++ b/spec/frontend/invite_members/mock_data/member_modal.js @@ -19,7 +19,7 @@ export const propsData = { export const inviteSource = 'unknown'; export const newProjectPath = 'projects/new'; export const freeUsersLimit = 5; -export const membersCount = 1; +export const remainingSeats = 2; export const user1 = { id: 1, name: 'Name One', username: 'one_1', avatar_url: '' }; export const user2 = { id: 2, name: 'Name Two', username: 'one_2', avatar_url: '' }; diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index e89c7233af3..26e97e09f30 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -2667,644 +2667,6 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end - describe '#check_constraint_name' do - it 'returns a valid constraint name' do - name = model.check_constraint_name(:this_is_a_very_long_table_name, - :with_a_very_long_column_name, - :with_a_very_long_type) - - expect(name).to be_an_instance_of(String) - expect(name).to start_with('check_') - expect(name.length).to eq(16) - end - end - - describe '#check_constraint_exists?' do - before do - ActiveRecord::Migration.connection.execute( - 'ALTER TABLE projects ADD CONSTRAINT check_1 CHECK (char_length(path) <= 5) NOT VALID' - ) - - ActiveRecord::Migration.connection.execute( - 'CREATE SCHEMA new_test_schema' - ) - - ActiveRecord::Migration.connection.execute( - 'CREATE TABLE new_test_schema.projects (id integer, name character varying)' - ) - - ActiveRecord::Migration.connection.execute( - 'ALTER TABLE new_test_schema.projects ADD CONSTRAINT check_2 CHECK (char_length(name) <= 5)' - ) - end - - it 'returns true if a constraint exists' do - expect(model.check_constraint_exists?(:projects, 'check_1')) - .to be_truthy - end - - it 'returns false if a constraint does not exist' do - expect(model.check_constraint_exists?(:projects, 'this_does_not_exist')) - .to be_falsy - end - - it 'returns false if a constraint with the same name exists in another table' do - expect(model.check_constraint_exists?(:users, 'check_1')) - .to be_falsy - end - - it 'returns false if a constraint with the same name exists for the same table in another schema' do - expect(model.check_constraint_exists?(:projects, 'check_2')) - .to be_falsy - end - end - - describe '#add_check_constraint' do - before do - allow(model).to receive(:check_constraint_exists?).and_return(false) - end - - context 'constraint name validation' do - it 'raises an error when too long' do - expect do - model.add_check_constraint( - :test_table, - 'name IS NOT NULL', - 'a' * (Gitlab::Database::MigrationHelpers::MAX_IDENTIFIER_NAME_LENGTH + 1) - ) - end.to raise_error(RuntimeError) - end - - it 'does not raise error when the length is acceptable' do - constraint_name = 'a' * Gitlab::Database::MigrationHelpers::MAX_IDENTIFIER_NAME_LENGTH - - expect(model).to receive(:transaction_open?).and_return(false) - expect(model).to receive(:check_constraint_exists?).and_return(false) - expect(model).to receive(:with_lock_retries).and_call_original - expect(model).to receive(:execute).with(/ADD CONSTRAINT/) - - model.add_check_constraint( - :test_table, - 'name IS NOT NULL', - constraint_name, - validate: false - ) - end - end - - context 'inside a transaction' do - it 'raises an error' do - expect(model).to receive(:transaction_open?).and_return(true) - - expect do - model.add_check_constraint( - :test_table, - 'name IS NOT NULL', - 'check_name_not_null' - ) - end.to raise_error(RuntimeError) - end - end - - context 'outside a transaction' do - before do - allow(model).to receive(:transaction_open?).and_return(false) - end - - context 'when the constraint is already defined in the database' do - it 'does not create a constraint' do - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, 'check_name_not_null') - .and_return(true) - - expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/) - - # setting validate: false to only focus on the ADD CONSTRAINT command - model.add_check_constraint( - :test_table, - 'name IS NOT NULL', - 'check_name_not_null', - validate: false - ) - end - end - - context 'when the constraint is not defined in the database' do - it 'creates the constraint' do - expect(model).to receive(:with_lock_retries).and_call_original - expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) - - # setting validate: false to only focus on the ADD CONSTRAINT command - model.add_check_constraint( - :test_table, - 'char_length(name) <= 255', - 'check_name_not_null', - validate: false - ) - end - end - - context 'when validate is not provided' do - it 'performs validation' do - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, 'check_name_not_null') - .and_return(false).exactly(1) - - expect(model).to receive(:disable_statement_timeout).and_call_original - expect(model).to receive(:statement_timeout_disabled?).and_return(false) - expect(model).to receive(:execute).with(/SET statement_timeout TO/) - expect(model).to receive(:with_lock_retries).and_call_original - expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) - - # we need the check constraint to exist so that the validation proceeds - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, 'check_name_not_null') - .and_return(true).exactly(1) - - expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) - expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) - - model.add_check_constraint( - :test_table, - 'char_length(name) <= 255', - 'check_name_not_null' - ) - end - end - - context 'when validate is provided with a falsey value' do - it 'skips validation' do - expect(model).not_to receive(:disable_statement_timeout) - expect(model).to receive(:with_lock_retries).and_call_original - expect(model).to receive(:execute).with(/ADD CONSTRAINT/) - expect(model).not_to receive(:execute).with(/VALIDATE CONSTRAINT/) - - model.add_check_constraint( - :test_table, - 'char_length(name) <= 255', - 'check_name_not_null', - validate: false - ) - end - end - - context 'when validate is provided with a truthy value' do - it 'performs validation' do - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, 'check_name_not_null') - .and_return(false).exactly(1) - - expect(model).to receive(:disable_statement_timeout).and_call_original - expect(model).to receive(:statement_timeout_disabled?).and_return(false) - expect(model).to receive(:execute).with(/SET statement_timeout TO/) - expect(model).to receive(:with_lock_retries).and_call_original - expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) - - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, 'check_name_not_null') - .and_return(true).exactly(1) - - expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) - expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) - - model.add_check_constraint( - :test_table, - 'char_length(name) <= 255', - 'check_name_not_null', - validate: true - ) - end - end - end - end - - describe '#validate_check_constraint' do - context 'when the constraint does not exist' do - it 'raises an error' do - error_message = /Could not find check constraint "check_1" on table "test_table"/ - - expect(model).to receive(:check_constraint_exists?).and_return(false) - - expect do - model.validate_check_constraint(:test_table, 'check_1') - end.to raise_error(RuntimeError, error_message) - end - end - - context 'when the constraint exists' do - it 'performs validation' do - validate_sql = /ALTER TABLE test_table VALIDATE CONSTRAINT check_name/ - - expect(model).to receive(:check_constraint_exists?).and_return(true) - expect(model).to receive(:disable_statement_timeout).and_call_original - expect(model).to receive(:statement_timeout_disabled?).and_return(false) - expect(model).to receive(:execute).with(/SET statement_timeout TO/) - expect(model).to receive(:execute).ordered.with(validate_sql) - expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) - - model.validate_check_constraint(:test_table, 'check_name') - end - end - end - - describe '#remove_check_constraint' do - before do - allow(model).to receive(:transaction_open?).and_return(false) - end - - it 'removes the constraint' do - drop_sql = /ALTER TABLE test_table\s+DROP CONSTRAINT IF EXISTS check_name/ - - expect(model).to receive(:with_lock_retries).and_call_original - expect(model).to receive(:execute).with(drop_sql) - - model.remove_check_constraint(:test_table, 'check_name') - end - end - - describe '#copy_check_constraints' do - context 'inside a transaction' do - it 'raises an error' do - expect(model).to receive(:transaction_open?).and_return(true) - - expect do - model.copy_check_constraints(:test_table, :old_column, :new_column) - end.to raise_error(RuntimeError) - end - end - - context 'outside a transaction' do - before do - allow(model).to receive(:transaction_open?).and_return(false) - allow(model).to receive(:column_exists?).and_return(true) - end - - let(:old_column_constraints) do - [ - { - 'schema_name' => 'public', - 'table_name' => 'test_table', - 'column_name' => 'old_column', - 'constraint_name' => 'check_d7d49d475d', - 'constraint_def' => 'CHECK ((old_column IS NOT NULL))' - }, - { - 'schema_name' => 'public', - 'table_name' => 'test_table', - 'column_name' => 'old_column', - 'constraint_name' => 'check_48560e521e', - 'constraint_def' => 'CHECK ((char_length(old_column) <= 255))' - }, - { - 'schema_name' => 'public', - 'table_name' => 'test_table', - 'column_name' => 'old_column', - 'constraint_name' => 'custom_check_constraint', - 'constraint_def' => 'CHECK (((old_column IS NOT NULL) AND (another_column IS NULL)))' - }, - { - 'schema_name' => 'public', - 'table_name' => 'test_table', - 'column_name' => 'old_column', - 'constraint_name' => 'not_valid_check_constraint', - 'constraint_def' => 'CHECK ((old_column IS NOT NULL)) NOT VALID' - } - ] - end - - it 'copies check constraints from one column to another' do - allow(model).to receive(:check_constraints_for) - .with(:test_table, :old_column, schema: nil) - .and_return(old_column_constraints) - - allow(model).to receive(:not_null_constraint_name).with(:test_table, :new_column) - .and_return('check_1') - - allow(model).to receive(:text_limit_name).with(:test_table, :new_column) - .and_return('check_2') - - allow(model).to receive(:check_constraint_name) - .with(:test_table, :new_column, 'copy_check_constraint') - .and_return('check_3') - - expect(model).to receive(:add_check_constraint) - .with( - :test_table, - '(new_column IS NOT NULL)', - 'check_1', - validate: true - ).once - - expect(model).to receive(:add_check_constraint) - .with( - :test_table, - '(char_length(new_column) <= 255)', - 'check_2', - validate: true - ).once - - expect(model).to receive(:add_check_constraint) - .with( - :test_table, - '((new_column IS NOT NULL) AND (another_column IS NULL))', - 'check_3', - validate: true - ).once - - expect(model).to receive(:add_check_constraint) - .with( - :test_table, - '(new_column IS NOT NULL)', - 'check_1', - validate: false - ).once - - model.copy_check_constraints(:test_table, :old_column, :new_column) - end - - it 'does nothing if there are no constraints defined for the old column' do - allow(model).to receive(:check_constraints_for) - .with(:test_table, :old_column, schema: nil) - .and_return([]) - - expect(model).not_to receive(:add_check_constraint) - - model.copy_check_constraints(:test_table, :old_column, :new_column) - end - - it 'raises an error when the orginating column does not exist' do - allow(model).to receive(:column_exists?).with(:test_table, :old_column).and_return(false) - - error_message = /Column old_column does not exist on test_table/ - - expect do - model.copy_check_constraints(:test_table, :old_column, :new_column) - end.to raise_error(RuntimeError, error_message) - end - - it 'raises an error when the target column does not exist' do - allow(model).to receive(:column_exists?).with(:test_table, :new_column).and_return(false) - - error_message = /Column new_column does not exist on test_table/ - - expect do - model.copy_check_constraints(:test_table, :old_column, :new_column) - end.to raise_error(RuntimeError, error_message) - end - end - end - - describe '#add_text_limit' do - context 'when it is called with the default options' do - it 'calls add_check_constraint with an infered constraint name and validate: true' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'max_length') - check = "char_length(name) <= 255" - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:add_check_constraint) - .with(:test_table, check, constraint_name, validate: true) - - model.add_text_limit(:test_table, :name, 255) - end - end - - context 'when all parameters are provided' do - it 'calls add_check_constraint with the correct parameters' do - constraint_name = 'check_name_limit' - check = "char_length(name) <= 255" - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:add_check_constraint) - .with(:test_table, check, constraint_name, validate: false) - - model.add_text_limit( - :test_table, - :name, - 255, - constraint_name: constraint_name, - validate: false - ) - end - end - end - - describe '#validate_text_limit' do - context 'when constraint_name is not provided' do - it 'calls validate_check_constraint with an infered constraint name' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'max_length') - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:validate_check_constraint) - .with(:test_table, constraint_name) - - model.validate_text_limit(:test_table, :name) - end - end - - context 'when constraint_name is provided' do - it 'calls validate_check_constraint with the correct parameters' do - constraint_name = 'check_name_limit' - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:validate_check_constraint) - .with(:test_table, constraint_name) - - model.validate_text_limit(:test_table, :name, constraint_name: constraint_name) - end - end - end - - describe '#remove_text_limit' do - context 'when constraint_name is not provided' do - it 'calls remove_check_constraint with an infered constraint name' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'max_length') - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:remove_check_constraint) - .with(:test_table, constraint_name) - - model.remove_text_limit(:test_table, :name) - end - end - - context 'when constraint_name is provided' do - it 'calls remove_check_constraint with the correct parameters' do - constraint_name = 'check_name_limit' - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:remove_check_constraint) - .with(:test_table, constraint_name) - - model.remove_text_limit(:test_table, :name, constraint_name: constraint_name) - end - end - end - - describe '#check_text_limit_exists?' do - context 'when constraint_name is not provided' do - it 'calls check_constraint_exists? with an infered constraint name' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'max_length') - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, constraint_name) - - model.check_text_limit_exists?(:test_table, :name) - end - end - - context 'when constraint_name is provided' do - it 'calls check_constraint_exists? with the correct parameters' do - constraint_name = 'check_name_limit' - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, constraint_name) - - model.check_text_limit_exists?(:test_table, :name, constraint_name: constraint_name) - end - end - end - - describe '#add_not_null_constraint' do - context 'when it is called with the default options' do - it 'calls add_check_constraint with an infered constraint name and validate: true' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'not_null') - check = "name IS NOT NULL" - - expect(model).to receive(:column_is_nullable?).and_return(true) - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:add_check_constraint) - .with(:test_table, check, constraint_name, validate: true) - - model.add_not_null_constraint(:test_table, :name) - end - end - - context 'when all parameters are provided' do - it 'calls add_check_constraint with the correct parameters' do - constraint_name = 'check_name_not_null' - check = "name IS NOT NULL" - - expect(model).to receive(:column_is_nullable?).and_return(true) - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:add_check_constraint) - .with(:test_table, check, constraint_name, validate: false) - - model.add_not_null_constraint( - :test_table, - :name, - constraint_name: constraint_name, - validate: false - ) - end - end - - context 'when the column is defined as NOT NULL' do - it 'does not add a check constraint' do - expect(model).to receive(:column_is_nullable?).and_return(false) - expect(model).not_to receive(:check_constraint_name) - expect(model).not_to receive(:add_check_constraint) - - model.add_not_null_constraint(:test_table, :name) - end - end - end - - describe '#validate_not_null_constraint' do - context 'when constraint_name is not provided' do - it 'calls validate_check_constraint with an infered constraint name' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'not_null') - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:validate_check_constraint) - .with(:test_table, constraint_name) - - model.validate_not_null_constraint(:test_table, :name) - end - end - - context 'when constraint_name is provided' do - it 'calls validate_check_constraint with the correct parameters' do - constraint_name = 'check_name_not_null' - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:validate_check_constraint) - .with(:test_table, constraint_name) - - model.validate_not_null_constraint(:test_table, :name, constraint_name: constraint_name) - end - end - end - - describe '#remove_not_null_constraint' do - context 'when constraint_name is not provided' do - it 'calls remove_check_constraint with an infered constraint name' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'not_null') - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:remove_check_constraint) - .with(:test_table, constraint_name) - - model.remove_not_null_constraint(:test_table, :name) - end - end - - context 'when constraint_name is provided' do - it 'calls remove_check_constraint with the correct parameters' do - constraint_name = 'check_name_not_null' - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:remove_check_constraint) - .with(:test_table, constraint_name) - - model.remove_not_null_constraint(:test_table, :name, constraint_name: constraint_name) - end - end - end - - describe '#check_not_null_constraint_exists?' do - context 'when constraint_name is not provided' do - it 'calls check_constraint_exists? with an infered constraint name' do - constraint_name = model.check_constraint_name(:test_table, - :name, - 'not_null') - - expect(model).to receive(:check_constraint_name).and_call_original - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, constraint_name) - - model.check_not_null_constraint_exists?(:test_table, :name) - end - end - - context 'when constraint_name is provided' do - it 'calls check_constraint_exists? with the correct parameters' do - constraint_name = 'check_name_not_null' - - expect(model).not_to receive(:check_constraint_name) - expect(model).to receive(:check_constraint_exists?) - .with(:test_table, constraint_name) - - model.check_not_null_constraint_exists?(:test_table, :name, constraint_name: constraint_name) - end - end - end - describe '#create_extension' do subject { model.create_extension(extension) } @@ -3357,30 +2719,6 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end - describe '#rename_constraint' do - it "executes the statement to rename constraint" do - expect(model).to receive(:execute).with /ALTER TABLE "test_table"\nRENAME CONSTRAINT "fk_old_name" TO "fk_new_name"/ - - model.rename_constraint(:test_table, :fk_old_name, :fk_new_name) - end - end - - describe '#drop_constraint' do - it "executes the statement to drop the constraint" do - expect(model).to receive(:execute).with("ALTER TABLE \"test_table\" DROP CONSTRAINT \"constraint_name\" CASCADE\n") - - model.drop_constraint(:test_table, :constraint_name, cascade: true) - end - - context 'when cascade option is false' do - it "executes the statement to drop the constraint without cascade" do - expect(model).to receive(:execute).with("ALTER TABLE \"test_table\" DROP CONSTRAINT \"constraint_name\" \n") - - model.drop_constraint(:test_table, :constraint_name, cascade: false) - end - end - end - describe '#add_primary_key_using_index' do it "executes the statement to add the primary key" do expect(model).to receive(:execute).with /ALTER TABLE "test_table" ADD CONSTRAINT "old_name" PRIMARY KEY USING INDEX "new_name"/ diff --git a/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb b/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb new file mode 100644 index 00000000000..6848fc85aa1 --- /dev/null +++ b/spec/lib/gitlab/database/migrations/constraints_helpers_spec.rb @@ -0,0 +1,679 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::Migrations::ConstraintsHelpers do + let(:model) do + ActiveRecord::Migration.new.extend(described_class) + end + + before do + allow(model).to receive(:puts) + end + + describe '#check_constraint_name' do + it 'returns a valid constraint name' do + name = model.check_constraint_name(:this_is_a_very_long_table_name, + :with_a_very_long_column_name, + :with_a_very_long_type) + + expect(name).to be_an_instance_of(String) + expect(name).to start_with('check_') + expect(name.length).to eq(16) + end + end + + describe '#check_constraint_exists?' do + before do + ActiveRecord::Migration.connection.execute( + 'ALTER TABLE projects ADD CONSTRAINT check_1 CHECK (char_length(path) <= 5) NOT VALID' + ) + + ActiveRecord::Migration.connection.execute( + 'CREATE SCHEMA new_test_schema' + ) + + ActiveRecord::Migration.connection.execute( + 'CREATE TABLE new_test_schema.projects (id integer, name character varying)' + ) + + ActiveRecord::Migration.connection.execute( + 'ALTER TABLE new_test_schema.projects ADD CONSTRAINT check_2 CHECK (char_length(name) <= 5)' + ) + end + + it 'returns true if a constraint exists' do + expect(model) + .to be_check_constraint_exists(:projects, 'check_1') + end + + it 'returns false if a constraint does not exist' do + expect(model) + .not_to be_check_constraint_exists(:projects, 'this_does_not_exist') + end + + it 'returns false if a constraint with the same name exists in another table' do + expect(model) + .not_to be_check_constraint_exists(:users, 'check_1') + end + + it 'returns false if a constraint with the same name exists for the same table in another schema' do + expect(model) + .not_to be_check_constraint_exists(:projects, 'check_2') + end + end + + describe '#add_check_constraint' do + before do + allow(model).to receive(:check_constraint_exists?).and_return(false) + end + + context 'when constraint name validation' do + it 'raises an error when too long' do + expect do + model.add_check_constraint( + :test_table, + 'name IS NOT NULL', + 'a' * (Gitlab::Database::MigrationHelpers::MAX_IDENTIFIER_NAME_LENGTH + 1) + ) + end.to raise_error(RuntimeError) + end + + it 'does not raise error when the length is acceptable' do + constraint_name = 'a' * Gitlab::Database::MigrationHelpers::MAX_IDENTIFIER_NAME_LENGTH + + expect(model).to receive(:transaction_open?).and_return(false) + expect(model).to receive(:check_constraint_exists?).and_return(false) + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:execute).with(/ADD CONSTRAINT/) + + model.add_check_constraint( + :test_table, + 'name IS NOT NULL', + constraint_name, + validate: false + ) + end + end + + context 'when inside a transaction' do + it 'raises an error' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect do + model.add_check_constraint( + :test_table, + 'name IS NOT NULL', + 'check_name_not_null' + ) + end.to raise_error(RuntimeError) + end + end + + context 'when outside a transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + context 'when the constraint is already defined in the database' do + it 'does not create a constraint' do + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, 'check_name_not_null') + .and_return(true) + + expect(model).not_to receive(:execute).with(/ADD CONSTRAINT/) + + # setting validate: false to only focus on the ADD CONSTRAINT command + model.add_check_constraint( + :test_table, + 'name IS NOT NULL', + 'check_name_not_null', + validate: false + ) + end + end + + context 'when the constraint is not defined in the database' do + it 'creates the constraint' do + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) + + # setting validate: false to only focus on the ADD CONSTRAINT command + model.add_check_constraint( + :test_table, + 'char_length(name) <= 255', + 'check_name_not_null', + validate: false + ) + end + end + + context 'when validate is not provided' do + it 'performs validation' do + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, 'check_name_not_null') + .and_return(false).exactly(1) + + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) + + # we need the check constraint to exist so that the validation proceeds + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, 'check_name_not_null') + .and_return(true).exactly(1) + + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + model.add_check_constraint( + :test_table, + 'char_length(name) <= 255', + 'check_name_not_null' + ) + end + end + + context 'when validate is provided with a falsey value' do + it 'skips validation' do + expect(model).not_to receive(:disable_statement_timeout) + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:execute).with(/ADD CONSTRAINT/) + expect(model).not_to receive(:execute).with(/VALIDATE CONSTRAINT/) + + model.add_check_constraint( + :test_table, + 'char_length(name) <= 255', + 'check_name_not_null', + validate: false + ) + end + end + + context 'when validate is provided with a truthy value' do + it 'performs validation' do + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, 'check_name_not_null') + .and_return(false).exactly(1) + + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:execute).with(/ADD CONSTRAINT check_name_not_null/) + + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, 'check_name_not_null') + .and_return(true).exactly(1) + + expect(model).to receive(:execute).ordered.with(/VALIDATE CONSTRAINT/) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + model.add_check_constraint( + :test_table, + 'char_length(name) <= 255', + 'check_name_not_null', + validate: true + ) + end + end + end + end + + describe '#validate_check_constraint' do + context 'when the constraint does not exist' do + it 'raises an error' do + error_message = /Could not find check constraint "check_1" on table "test_table"/ + + expect(model).to receive(:check_constraint_exists?).and_return(false) + + expect do + model.validate_check_constraint(:test_table, 'check_1') + end.to raise_error(RuntimeError, error_message) + end + end + + context 'when the constraint exists' do + it 'performs validation' do + validate_sql = /ALTER TABLE test_table VALIDATE CONSTRAINT check_name/ + + expect(model).to receive(:check_constraint_exists?).and_return(true) + expect(model).to receive(:disable_statement_timeout).and_call_original + expect(model).to receive(:statement_timeout_disabled?).and_return(false) + expect(model).to receive(:execute).with(/SET statement_timeout TO/) + expect(model).to receive(:execute).ordered.with(validate_sql) + expect(model).to receive(:execute).ordered.with(/RESET statement_timeout/) + + model.validate_check_constraint(:test_table, 'check_name') + end + end + end + + describe '#remove_check_constraint' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + it 'removes the constraint' do + drop_sql = /ALTER TABLE test_table\s+DROP CONSTRAINT IF EXISTS check_name/ + + expect(model).to receive(:with_lock_retries).and_call_original + expect(model).to receive(:execute).with(drop_sql) + + model.remove_check_constraint(:test_table, 'check_name') + end + end + + describe '#copy_check_constraints' do + context 'when inside a transaction' do + it 'raises an error' do + expect(model).to receive(:transaction_open?).and_return(true) + + expect do + model.copy_check_constraints(:test_table, :old_column, :new_column) + end.to raise_error(RuntimeError) + end + end + + context 'when outside a transaction' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + allow(model).to receive(:column_exists?).and_return(true) + end + + let(:old_column_constraints) do + [ + { + 'schema_name' => 'public', + 'table_name' => 'test_table', + 'column_name' => 'old_column', + 'constraint_name' => 'check_d7d49d475d', + 'constraint_def' => 'CHECK ((old_column IS NOT NULL))' + }, + { + 'schema_name' => 'public', + 'table_name' => 'test_table', + 'column_name' => 'old_column', + 'constraint_name' => 'check_48560e521e', + 'constraint_def' => 'CHECK ((char_length(old_column) <= 255))' + }, + { + 'schema_name' => 'public', + 'table_name' => 'test_table', + 'column_name' => 'old_column', + 'constraint_name' => 'custom_check_constraint', + 'constraint_def' => 'CHECK (((old_column IS NOT NULL) AND (another_column IS NULL)))' + }, + { + 'schema_name' => 'public', + 'table_name' => 'test_table', + 'column_name' => 'old_column', + 'constraint_name' => 'not_valid_check_constraint', + 'constraint_def' => 'CHECK ((old_column IS NOT NULL)) NOT VALID' + } + ] + end + + it 'copies check constraints from one column to another' do + allow(model).to receive(:check_constraints_for) + .with(:test_table, :old_column, schema: nil) + .and_return(old_column_constraints) + + allow(model).to receive(:not_null_constraint_name).with(:test_table, :new_column) + .and_return('check_1') + + allow(model).to receive(:text_limit_name).with(:test_table, :new_column) + .and_return('check_2') + + allow(model).to receive(:check_constraint_name) + .with(:test_table, :new_column, 'copy_check_constraint') + .and_return('check_3') + + expect(model).to receive(:add_check_constraint) + .with( + :test_table, + '(new_column IS NOT NULL)', + 'check_1', + validate: true + ).once + + expect(model).to receive(:add_check_constraint) + .with( + :test_table, + '(char_length(new_column) <= 255)', + 'check_2', + validate: true + ).once + + expect(model).to receive(:add_check_constraint) + .with( + :test_table, + '((new_column IS NOT NULL) AND (another_column IS NULL))', + 'check_3', + validate: true + ).once + + expect(model).to receive(:add_check_constraint) + .with( + :test_table, + '(new_column IS NOT NULL)', + 'check_1', + validate: false + ).once + + model.copy_check_constraints(:test_table, :old_column, :new_column) + end + + it 'does nothing if there are no constraints defined for the old column' do + allow(model).to receive(:check_constraints_for) + .with(:test_table, :old_column, schema: nil) + .and_return([]) + + expect(model).not_to receive(:add_check_constraint) + + model.copy_check_constraints(:test_table, :old_column, :new_column) + end + + it 'raises an error when the orginating column does not exist' do + allow(model).to receive(:column_exists?).with(:test_table, :old_column).and_return(false) + + error_message = /Column old_column does not exist on test_table/ + + expect do + model.copy_check_constraints(:test_table, :old_column, :new_column) + end.to raise_error(RuntimeError, error_message) + end + + it 'raises an error when the target column does not exist' do + allow(model).to receive(:column_exists?).with(:test_table, :new_column).and_return(false) + + error_message = /Column new_column does not exist on test_table/ + + expect do + model.copy_check_constraints(:test_table, :old_column, :new_column) + end.to raise_error(RuntimeError, error_message) + end + end + end + + describe '#add_text_limit' do + context 'when it is called with the default options' do + it 'calls add_check_constraint with an infered constraint name and validate: true' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'max_length') + check = "char_length(name) <= 255" + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:add_check_constraint) + .with(:test_table, check, constraint_name, validate: true) + + model.add_text_limit(:test_table, :name, 255) + end + end + + context 'when all parameters are provided' do + it 'calls add_check_constraint with the correct parameters' do + constraint_name = 'check_name_limit' + check = "char_length(name) <= 255" + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:add_check_constraint) + .with(:test_table, check, constraint_name, validate: false) + + model.add_text_limit( + :test_table, + :name, + 255, + constraint_name: constraint_name, + validate: false + ) + end + end + end + + describe '#validate_text_limit' do + context 'when constraint_name is not provided' do + it 'calls validate_check_constraint with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'max_length') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:validate_check_constraint) + .with(:test_table, constraint_name) + + model.validate_text_limit(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls validate_check_constraint with the correct parameters' do + constraint_name = 'check_name_limit' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:validate_check_constraint) + .with(:test_table, constraint_name) + + model.validate_text_limit(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#remove_text_limit' do + context 'when constraint_name is not provided' do + it 'calls remove_check_constraint with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'max_length') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:remove_check_constraint) + .with(:test_table, constraint_name) + + model.remove_text_limit(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls remove_check_constraint with the correct parameters' do + constraint_name = 'check_name_limit' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:remove_check_constraint) + .with(:test_table, constraint_name) + + model.remove_text_limit(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#check_text_limit_exists?' do + context 'when constraint_name is not provided' do + it 'calls check_constraint_exists? with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'max_length') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, constraint_name) + + model.check_text_limit_exists?(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls check_constraint_exists? with the correct parameters' do + constraint_name = 'check_name_limit' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, constraint_name) + + model.check_text_limit_exists?(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#add_not_null_constraint' do + context 'when it is called with the default options' do + it 'calls add_check_constraint with an infered constraint name and validate: true' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + check = "name IS NOT NULL" + + expect(model).to receive(:column_is_nullable?).and_return(true) + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:add_check_constraint) + .with(:test_table, check, constraint_name, validate: true) + + model.add_not_null_constraint(:test_table, :name) + end + end + + context 'when all parameters are provided' do + it 'calls add_check_constraint with the correct parameters' do + constraint_name = 'check_name_not_null' + check = "name IS NOT NULL" + + expect(model).to receive(:column_is_nullable?).and_return(true) + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:add_check_constraint) + .with(:test_table, check, constraint_name, validate: false) + + model.add_not_null_constraint( + :test_table, + :name, + constraint_name: constraint_name, + validate: false + ) + end + end + + context 'when the column is defined as NOT NULL' do + it 'does not add a check constraint' do + expect(model).to receive(:column_is_nullable?).and_return(false) + expect(model).not_to receive(:check_constraint_name) + expect(model).not_to receive(:add_check_constraint) + + model.add_not_null_constraint(:test_table, :name) + end + end + end + + describe '#validate_not_null_constraint' do + context 'when constraint_name is not provided' do + it 'calls validate_check_constraint with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:validate_check_constraint) + .with(:test_table, constraint_name) + + model.validate_not_null_constraint(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls validate_check_constraint with the correct parameters' do + constraint_name = 'check_name_not_null' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:validate_check_constraint) + .with(:test_table, constraint_name) + + model.validate_not_null_constraint(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#remove_not_null_constraint' do + context 'when constraint_name is not provided' do + it 'calls remove_check_constraint with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:remove_check_constraint) + .with(:test_table, constraint_name) + + model.remove_not_null_constraint(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls remove_check_constraint with the correct parameters' do + constraint_name = 'check_name_not_null' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:remove_check_constraint) + .with(:test_table, constraint_name) + + model.remove_not_null_constraint(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#check_not_null_constraint_exists?' do + context 'when constraint_name is not provided' do + it 'calls check_constraint_exists? with an infered constraint name' do + constraint_name = model.check_constraint_name(:test_table, + :name, + 'not_null') + + expect(model).to receive(:check_constraint_name).and_call_original + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, constraint_name) + + model.check_not_null_constraint_exists?(:test_table, :name) + end + end + + context 'when constraint_name is provided' do + it 'calls check_constraint_exists? with the correct parameters' do + constraint_name = 'check_name_not_null' + + expect(model).not_to receive(:check_constraint_name) + expect(model).to receive(:check_constraint_exists?) + .with(:test_table, constraint_name) + + model.check_not_null_constraint_exists?(:test_table, :name, constraint_name: constraint_name) + end + end + end + + describe '#rename_constraint' do + it "executes the statement to rename constraint" do + expect(model).to receive(:execute).with( + /ALTER TABLE "test_table"\nRENAME CONSTRAINT "fk_old_name" TO "fk_new_name"/ + ) + + model.rename_constraint(:test_table, :fk_old_name, :fk_new_name) + end + end + + describe '#drop_constraint' do + it "executes the statement to drop the constraint" do + expect(model).to receive(:execute).with( + "ALTER TABLE \"test_table\" DROP CONSTRAINT \"constraint_name\" CASCADE\n" + ) + + model.drop_constraint(:test_table, :constraint_name, cascade: true) + end + + context 'when cascade option is false' do + it "executes the statement to drop the constraint without cascade" do + expect(model).to receive(:execute).with("ALTER TABLE \"test_table\" DROP CONSTRAINT \"constraint_name\" \n") + + model.drop_constraint(:test_table, :constraint_name, cascade: false) + end + end + end +end diff --git a/spec/lib/gitlab/metrics/global_search_slis_spec.rb b/spec/lib/gitlab/metrics/global_search_slis_spec.rb index 0c09cf6dd71..c10d83664ea 100644 --- a/spec/lib/gitlab/metrics/global_search_slis_spec.rb +++ b/spec/lib/gitlab/metrics/global_search_slis_spec.rb @@ -47,10 +47,10 @@ RSpec.describe Gitlab::Metrics::GlobalSearchSlis do describe '#record_apdex' do where(:search_type, :code_search, :duration_target) do - 'basic' | false | 7.031 - 'basic' | true | 21.903 - 'advanced' | false | 4.865 - 'advanced' | true | 13.546 + 'basic' | false | 8.812 + 'basic' | true | 27.538 + 'advanced' | false | 2.452 + 'advanced' | true | 15.52 end with_them do diff --git a/spec/requests/api/search_spec.rb b/spec/requests/api/search_spec.rb index 05f38aff6ab..60acf6b71dd 100644 --- a/spec/requests/api/search_spec.rb +++ b/spec/requests/api/search_spec.rb @@ -29,6 +29,19 @@ RSpec.describe API::Search do end end + shared_examples 'apdex recorded' do |scope:, level:, search: ''| + it 'increments the custom search sli apdex' do + expect(Gitlab::Metrics::GlobalSearchSlis).to receive(:record_apdex).with( + elapsed: a_kind_of(Numeric), + search_scope: scope, + search_type: 'basic', + search_level: level + ) + + get api(endpoint, user), params: { scope: scope, search: search } + end + end + shared_examples 'orderable by created_at' do |scope:| it 'allows ordering results by created_at asc' do get api(endpoint, user), params: { scope: scope, search: 'sortable', order_by: 'created_at', sort: 'asc' } @@ -172,6 +185,8 @@ RSpec.describe API::Search do it_behaves_like 'pagination', scope: :projects it_behaves_like 'ping counters', scope: :projects + + it_behaves_like 'apdex recorded', scope: 'projects', level: 'global' end context 'for issues scope' do @@ -186,6 +201,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :issues + it_behaves_like 'apdex recorded', scope: 'issues', level: 'global' + it_behaves_like 'issues orderable by created_at' describe 'pagination' do @@ -248,6 +265,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :merge_requests + it_behaves_like 'apdex recorded', scope: 'merge_requests', level: 'global' + it_behaves_like 'merge_requests orderable by created_at' describe 'pagination' do @@ -293,6 +312,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :milestones + it_behaves_like 'apdex recorded', scope: 'milestones', level: 'global' + describe 'pagination' do before do create(:milestone, project: project, title: 'another milestone') @@ -330,6 +351,8 @@ RSpec.describe API::Search do it_behaves_like 'pagination', scope: :users it_behaves_like 'ping counters', scope: :users + + it_behaves_like 'apdex recorded', scope: 'users', level: 'global' end context 'for snippet_titles scope' do @@ -343,6 +366,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :snippet_titles + it_behaves_like 'apdex recorded', scope: 'snippet_titles', level: 'global' + describe 'pagination' do before do create(:snippet, :public, title: 'another snippet', content: 'snippet content') @@ -352,17 +377,6 @@ RSpec.describe API::Search do end end - it 'increments the custom search sli apdex' do - expect(Gitlab::Metrics::GlobalSearchSlis).to receive(:record_apdex).with( - elapsed: a_kind_of(Numeric), - search_scope: 'issues', - search_type: 'basic', - search_level: 'global' - ) - - get api(endpoint, user), params: { scope: 'issues', search: 'john doe' } - end - it 'increments the custom search sli error rate with error false if no error occurred' do expect(Gitlab::Metrics::GlobalSearchSlis).to receive(:record_error_rate).with( error: false, @@ -466,6 +480,8 @@ RSpec.describe API::Search do it_behaves_like 'pagination', scope: :projects it_behaves_like 'ping counters', scope: :projects + + it_behaves_like 'apdex recorded', scope: 'projects', level: 'group' end context 'for issues scope' do @@ -479,6 +495,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :issues + it_behaves_like 'apdex recorded', scope: 'issues', level: 'group' + it_behaves_like 'issues orderable by created_at' describe 'pagination' do @@ -501,6 +519,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :merge_requests + it_behaves_like 'apdex recorded', scope: 'merge_requests', level: 'group' + it_behaves_like 'merge_requests orderable by created_at' describe 'pagination' do @@ -523,6 +543,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :milestones + it_behaves_like 'apdex recorded', scope: 'milestones', level: 'group' + describe 'pagination' do before do create(:milestone, project: project, title: 'another milestone') @@ -556,6 +578,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :users + it_behaves_like 'apdex recorded', scope: 'users', level: 'group' + describe 'pagination' do before do create(:group_member, :developer, group: group) @@ -645,6 +669,8 @@ RSpec.describe API::Search do it_behaves_like 'issues orderable by created_at' + it_behaves_like 'apdex recorded', scope: 'issues', level: 'project' + describe 'pagination' do before do create(:issue, project: project, title: 'another issue') @@ -677,6 +703,8 @@ RSpec.describe API::Search do it_behaves_like 'merge_requests orderable by created_at' + it_behaves_like 'apdex recorded', scope: 'merge_requests', level: 'project' + describe 'pagination' do before do create(:merge_request, source_project: repo_project, title: 'another mr', target_branch: 'another_branch') @@ -700,6 +728,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :milestones + it_behaves_like 'apdex recorded', scope: 'milestones', level: 'project' + describe 'pagination' do before do create(:milestone, project: project, title: 'another milestone') @@ -737,6 +767,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :users + it_behaves_like 'apdex recorded', scope: 'users', level: 'project' + describe 'pagination' do before do create(:project_member, :developer, project: project) @@ -757,6 +789,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :notes + it_behaves_like 'apdex recorded', scope: 'notes', level: 'project' + describe 'pagination' do before do mr = create(:merge_request, source_project: project, target_branch: 'another_branch') @@ -780,6 +814,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :wiki_blobs + it_behaves_like 'apdex recorded', scope: 'wiki_blobs', level: 'project' + describe 'pagination' do before do create(:wiki_page, wiki: wiki, title: 'home 2', content: 'Another page') @@ -802,6 +838,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :commits + it_behaves_like 'apdex recorded', scope: 'commits', level: 'project' + describe 'pipeline visibility' do shared_examples 'pipeline information visible' do it 'contains status and last_pipeline' do @@ -899,6 +937,8 @@ RSpec.describe API::Search do end it_behaves_like 'response is correct', schema: 'public_api/v4/commits_details' + + it_behaves_like 'apdex recorded', scope: 'commits', level: 'project' end context 'for blobs scope' do @@ -914,6 +954,8 @@ RSpec.describe API::Search do it_behaves_like 'ping counters', scope: :blobs + it_behaves_like 'apdex recorded', scope: 'blobs', level: 'project' + context 'filters' do it 'by filename' do get api(endpoint, user), params: { scope: 'blobs', search: 'mon filename:PROCESS.md' } |