diff options
| -rw-r--r-- | app/models/project_services/chat_message/pipeline_message.rb | 15 | ||||
| -rw-r--r-- | changelogs/unreleased/slack-notification-retry-success-skip.yml | 5 | ||||
| -rw-r--r-- | doc/development/integrations/secure.md | 21 | ||||
| -rw-r--r-- | doc/development/reusing_abstractions.md | 2 | ||||
| -rw-r--r-- | doc/gitlab-basics/create-project.md | 18 | ||||
| -rw-r--r-- | doc/user/group/saml_sso/index.md | 21 | ||||
| -rw-r--r-- | doc/user/group/saml_sso/scim_setup.md | 21 | ||||
| -rw-r--r-- | doc/user/profile/account/delete_account.md | 28 | ||||
| -rw-r--r-- | doc/user/profile/personal_access_tokens.md | 19 | ||||
| -rw-r--r-- | lib/gitlab/database/batch_count.rb | 11 | ||||
| -rw-r--r-- | lib/gitlab/database/migration_helpers.rb | 139 | ||||
| -rw-r--r-- | spec/lib/gitlab/database/batch_count_spec.rb | 10 | ||||
| -rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 329 | ||||
| -rw-r--r-- | spec/models/project_services/chat_message/pipeline_message_spec.rb | 51 |
14 files changed, 640 insertions, 50 deletions
diff --git a/app/models/project_services/chat_message/pipeline_message.rb b/app/models/project_services/chat_message/pipeline_message.rb index 52a26f6211a..50b982a803f 100644 --- a/app/models/project_services/chat_message/pipeline_message.rb +++ b/app/models/project_services/chat_message/pipeline_message.rb @@ -34,7 +34,9 @@ module ChatMessage @duration = pipeline_attributes[:duration].to_i @finished_at = pipeline_attributes[:finished_at] ? Time.parse(pipeline_attributes[:finished_at]).to_i : nil @pipeline_id = pipeline_attributes[:id] - @failed_jobs = Array(data[:builds]).select { |b| b[:status] == 'failed' }.reverse # Show failed jobs from oldest to newest + + # Get list of jobs that have actually failed (after exhausting all retries) + @failed_jobs = actually_failed_jobs(Array(data[:builds])) @failed_stages = @failed_jobs.map { |j| j[:stage] }.uniq @project = Project.find(data[:project][:id]) @@ -90,6 +92,17 @@ module ChatMessage private + def actually_failed_jobs(builds) + succeeded_job_names = builds.map { |b| b[:name] if b[:status] == 'success' }.compact.uniq + + failed_jobs = builds.select do |build| + # Select jobs which doesn't have a successful retry + build[:status] == 'failed' && !succeeded_job_names.include?(build[:name]) + end + + failed_jobs.uniq { |job| job[:name] }.reverse + end + def fancy_notifications? Feature.enabled?(:fancy_pipeline_slack_notifications, default_enabled: true) end diff --git a/changelogs/unreleased/slack-notification-retry-success-skip.yml b/changelogs/unreleased/slack-notification-retry-success-skip.yml new file mode 100644 index 00000000000..1779ba8b138 --- /dev/null +++ b/changelogs/unreleased/slack-notification-retry-success-skip.yml @@ -0,0 +1,5 @@ +--- +title: Make pipeline info in chat notifications concise +merge_request: 28284 +author: +type: changed diff --git a/doc/development/integrations/secure.md b/doc/development/integrations/secure.md index 69128cfb625..b38e45778fb 100644 --- a/doc/development/integrations/secure.md +++ b/doc/development/integrations/secure.md @@ -233,6 +233,12 @@ describes the Secure report format version. The `vulnerabilities` field of the report is an array of vulnerability objects. +#### ID + +The `id`Â field is the unique identifier of the vulnerability. +It is used to reference a fixed vulnerability from a [remediation objects](#remediations). +We recommend that you generate a UUID and use it as the `id` field's value. + #### Category The value of the `category` field matches the report type: @@ -467,6 +473,15 @@ The `remediations` field of the report is an array of remediation objects. Each remediation describes a patch that can be applied to automatically fix a set of vulnerabilities. -Currently, remediations rely on a deprecated field named `cve` to reference vulnerabilities, -so it is recommended not to use them until a new format has been defined. -See [issue #36777](https://gitlab.com/gitlab-org/gitlab/issues/36777). +#### Summary + +The `summary` field is an overview of how the vulnerabilities can be fixed. + +#### Fixed vulnerabilities + +The `fixes` field is an array of objects that reference the vulnerabilities fixed by the +remediation. `fixes[].id` contains a fixed vulnerability's unique identifier. + +#### Diff + +The `diff` field is a base64-encoded remediation code diff, compatible with [`git apply`](https://git-scm.com/docs/git-format-patch#_discussion). diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index fce144f8dc2..8711bac69e0 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -127,6 +127,8 @@ Everything in `lib/api`. Everything that resides in `app/services`. +In Service classes the use of `execute` and `#execute` is preferred over `call` and `#call`. + #### ServiceResponse Service classes usually have an `execute` method, which can return a diff --git a/doc/gitlab-basics/create-project.md b/doc/gitlab-basics/create-project.md index 34e3ff7a6fa..1febe8337bc 100644 --- a/doc/gitlab-basics/create-project.md +++ b/doc/gitlab-basics/create-project.md @@ -80,10 +80,26 @@ To use a built-in template on the **New project** page: 1. Finish creating the project by filling out the project's details. The process is the same as creating a [blank project](#blank-projects). +##### Enterprise templates **(ULTIMATE)** + +GitLab is developing Enterprise templates to help you streamline audit management with selected regulatory standards. These templates automatically import issues that correspond to each regulatory requirement. + +To create a new project with an Enterprise template, on the **New project** page: + +1. On the **Create from template** tab, select the **Built-in** tab. +1. From the list of available built-in Enterprise templates, click the: + - **Preview** button to look at the template source itself. + - **Use template** button to start creating the project. +1. Finish creating the project by filling out the project's details. The process is the same as creating a [blank project](#blank-projects). + +Available Enterprise templates include: + +- HIPAA Audit Protocol template ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/13756) in GitLab 12.10) + TIP: **Tip:** You can improve the existing built-in templates or contribute new ones in the [`project-templates`](https://gitlab.com/gitlab-org/project-templates) and -[`pages`](https://gitlab.com/pages) groups. +[`pages`](https://gitlab.com/pages) groups by following [these steps](https://gitlab.com/gitlab-org/project-templates/contributing). #### Custom project templates **(PREMIUM)** diff --git a/doc/user/group/saml_sso/index.md b/doc/user/group/saml_sso/index.md index 4fcb5064c8c..2b4170d21af 100644 --- a/doc/user/group/saml_sso/index.md +++ b/doc/user/group/saml_sso/index.md @@ -42,7 +42,8 @@ GitLab.com uses the SAML NameID to identify users. The NameID element: - Is case sensitive. The NameID must match exactly on subsequent login attempts, so should not rely on user input that could change between upper and lower case. - Should not be an email address or username. We strongly recommend against these as it is hard to guarantee they will never change, for example when a person's name changes. Email addresses are also case-insensitive, which can result in users being unable to sign in. -The recommended field for supported providers are in the [provider specific notes](#providers). +The relevant field name and recommended value for supported providers are in the [provider specific notes](#providers). +appropriate corresponding field. CAUTION: **Warning:** Once users have signed into GitLab using the SSO SAML setup, changing the `NameID` will break the configuration and potentially lock users out of the GitLab group. @@ -407,11 +408,13 @@ If you do not wish to use that GitLab user with the SAML login, you can [unlink ### Message: "SAML authentication failed: User has already been taken" -The user you are signed in with already has SAML linked to a different identity. This might mean you've attempted to link multiple SAML identities to the same user for a given Identity Provider. This could also be a symptom of the Identity Provider returning an inconsistent [NameID](#nameid). +The user that you're signed in with already has SAML linked to a different identity. +Here are possible causes and solutions: -To change which identity you sign in with, you can [unlink the previous SAML identity](#unlinking-accounts) from this GitLab account. - -Alternatively, an admin of your Identity Provider can use the [SCIM API](../../../api/scim.md) to update your `extern_uid` to match the current **NameID**. +| Cause | Solution | +|------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| You've tried to link multiple SAML identities to the same user, for a given Identity Provider. | Change the identity that you sign in with. To do so, [unlink the previous SAML identity](#unlinking-accounts) from this GitLab account before attempting to sign in again. | +| The Identity Provider might be returning an inconsistent [NameID](#nameid). | Ask an admin of your Identity Provider to use the [SCIM API](../../../api/scim.md) to update your `extern_uid` to match the current **NameID**. | ### Message: "SAML authentication failed: Email has already been taken" @@ -427,13 +430,13 @@ This can be prevented by configuring the [NameID](#nameid) to return a consisten ### The NameID has changed -As mentioned in the [NameID](#nameid) section, if the NameID changes for any user, the user can be locked out. This is common for setups using an email address as the identifier. - -To fix the issue, follow the steps outlined in the ["SAML authentication failed: User has already been taken"](#message-saml-authentication-failed-user-has-already-been-taken) section. We recommend using the API method if many users are affected so that the changes can be done in a scripted batch. +| Cause | Solution | +|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| As mentioned in the [NameID](#nameid) section, if the NameID changes for any user, the user can be locked out. This is a common problem when an email address is used as the identifier. | Follow the steps outlined in the ["SAML authentication failed: User has already been taken"](#message-saml-authentication-failed-user-has-already-been-taken) section. If many users are affected, we recommend that you use the appropriate API. | ### I need to change my SAML app -Users need to [unlink the previous SAML identity](#unlinking-accounts) and [link their identity](#user-access-and-management) using the new SAML app. +Users will need to [unlink the current SAML identity](#unlinking-accounts) and [link their identity](#user-access-and-management) to the new SAML app. ### My identity provider isn't listed diff --git a/doc/user/group/saml_sso/scim_setup.md b/doc/user/group/saml_sso/scim_setup.md index 42bc52a9201..e333fd19c1b 100644 --- a/doc/user/group/saml_sso/scim_setup.md +++ b/doc/user/group/saml_sso/scim_setup.md @@ -167,7 +167,10 @@ As a workaround, try an alternate mapping: ### Message: "SAML authentication failed: Email has already been taken" -It is expected for the app's logs to show this error for any existing user until they sign in for the first time. GitLab will not allow multiple accounts to have the same email address. +This message may be caused by the following: + +- Existing users have not yet signed into the new app. +- The identity provider attempts to create a new user account in GitLab with an email address that already exists in GitLab.com. ### How do I diagnose why a user is unable to sign in @@ -197,15 +200,17 @@ Whether the value was changed or you need to map to a different field, ensure `i If GitLab's `externalId` doesn't match the SAML NameId, it will need to be updated in order for the user to log in. Ideally your identity provider will be configured to do such an update, but in some cases it may be unable to do so, such as when looking up a user fails due to an ID change. -Fixing the fields your SCIM identity provider sends as `id` and `externalId` can correct this, however we use these IDs to look up users so if the identity provider is unaware of the current values for these it may try to create new duplicate users instead. - -If the `externalId` we have stored for a user has an incorrect value that doesn't match the SAML NameId, then it can be corrected ine on or two ways. - -One option is to have users can be delinked and relink following details in the ["SAML authentication failed: User has already been taken"](./index.md#message-saml-authentication-failed-user-has-already-been-taken) section. Additionally, to unlink all users at once, remove all users from the SAML app while SCIM is still turned on. +Be cautious if you revise the fields used by your SCIM identity provider, typically `id` and `externalId`. +We use these IDs to look up users. If the identity provider does not know the current values for these fields, +that provider may create duplicate users. -Another option is with the manual use of the SCIM API. +If the `externalId` for a user is not correct, and also doesn't match the SAML NameID, +you can address the problem in the following ways: -The [SCIM API](../../../api/scim.md#update-a-single-saml-user) can be used to manually correct the `externalId` stored for users so that it matches the SAML NameId. You'll need to know the desired value that matches the `NameId` as well as the current `externalId` to look up the user. +- You can have users unlink and relink themselves, based on the ["SAML authentication failed: User has already been taken"](./index.md#message-saml-authentication-failed-user-has-already-been-taken) section. +- You can unlink all users simultaneously, by removing all users from the SAML app while provisioning is turned on. +- You can use the [SCIM API](../../../api/scim.md#update-a-single-saml-user) to manually correct the `externalId` stored for users to match the SAML `NameId`. + To look up a user, you'll need to know the desired value that matches the `NameId` as well as the current `externalId`. It is then possible to issue a manual SCIM#update request, for example: diff --git a/doc/user/profile/account/delete_account.md b/doc/user/profile/account/delete_account.md index 97827963be0..c9193c6d94c 100644 --- a/doc/user/profile/account/delete_account.md +++ b/doc/user/profile/account/delete_account.md @@ -32,18 +32,22 @@ As an administrator, you can delete a user account by: - **Delete user and contributions** to delete the user and their associated records. +DANGER: **Danger:** Using the **Delete user and contributions** option may result +in removing more data than intended. Please see [associated records](#associated-records) +below for additional details. + ## Associated Records -> - Introduced for issues in -> [GitLab 9.0](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/7393). -> - Introduced for merge requests, award emoji, notes, and abuse reports in -> [GitLab 9.1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10467). -> - Hard deletion from abuse reports and spam logs was introduced in -> [GitLab 9.1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10273), -> and from the API in -> [GitLab 9.3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/11853). +> - Introduced for issues in [GitLab 9.0](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/7393). +> - Introduced for merge requests, award emoji, notes, and abuse reports in [GitLab 9.1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10467). +> - Hard deletion from abuse reports and spam logs was introduced in [GitLab 9.1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10273), and from the API in [GitLab 9.3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/11853). + +There are two options for deleting users: -When a user account is deleted, not all associated records are deleted with it. +- **Delete user** +- **Delete user and contributions** + +When using the **Delete user** option, not all associated records are deleted with the user. Here's a list of things that will **not** be deleted: - Issues that the user created. @@ -57,6 +61,12 @@ user with the username "Ghost User", whose sole purpose is to act as a container for such records. Any commits made by a deleted user will still display the username of the original user. +When using the **Delete user and contributions** option, **all** associated records +are removed. This includes all of the items mentioned above including issues, +merge requests, notes/comments, and more. Consider +[blocking a user](../../admin_area/blocking_unblocking_users.md) +or using the **Delete user** option instead. + When a user is deleted from an [abuse report](../../admin_area/abuse_reports.md) or spam log, these associated records are not ghosted and will be removed, along with any groups the user diff --git a/doc/user/profile/personal_access_tokens.md b/doc/user/profile/personal_access_tokens.md index 204230c4ca3..1223f7b801a 100644 --- a/doc/user/profile/personal_access_tokens.md +++ b/doc/user/profile/personal_access_tokens.md @@ -4,11 +4,11 @@ type: concepts, howto # Personal access tokens -> [Introduced][ce-3749] in GitLab 8.8. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/3749) in GitLab 8.8. -If you're unable to use [OAuth2](../../api/oauth2.md), you can use a personal access token to authenticate with the [GitLab API][api]. +If you're unable to use [OAuth2](../../api/oauth2.md), you can use a personal access token to authenticate with the [GitLab API](../../api/README.md#personal-access-tokens). -You can also use personal access tokens with Git to authenticate over HTTP or SSH. Personal access tokens are required when [Two-Factor Authentication (2FA)][2fa] is enabled. In both cases, you can authenticate with a token in place of your password. +You can also use personal access tokens with Git to authenticate over HTTP or SSH. Personal access tokens are required when [Two-Factor Authentication (2FA)](../account/two_factor_authentication.md) is enabled. In both cases, you can authenticate with a token in place of your password. Personal access tokens expire on the date you define, at midnight UTC. @@ -41,21 +41,14 @@ the following table. | Scope | Introduced in | Description | | ------------------ | ------------- | ----------- | -| `read_user` | [GitLab 8.15](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/5951) | Allows access to the read-only endpoints under `/users`. Essentially, any of the `GET` requests in the [Users API][users] are allowed. | +| `read_user` | [GitLab 8.15](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/5951) | Allows access to the read-only endpoints under `/users`. Essentially, any of the `GET` requests in the [Users API](../../api/users.md) are allowed. | | `api` | [GitLab 8.15](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/5951) | Grants complete read/write access to the API, including all groups and projects, the container registry, and the package registry. | -| `read_api` | [GitLab 12.10](https://https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28944) | Grants read access to the API, including all groups and projects, the container registry, and the package registry. | -| `read_registry` | [GitLab 9.3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/11845) | Allows to read (pull) [container registry] images if a project is private and authorization is required. | +| `read_api` | [GitLab 12.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/28944) | Grants read access to the API, including all groups and projects, the container registry, and the package registry. | +| `read_registry` | [GitLab 9.3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/11845) | Allows to read (pull) [container registry](../packages/container_registry/index.md) images if a project is private and authorization is required. | | `sudo` | [GitLab 10.2](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/14838) | Allows performing API actions as any user in the system (if the authenticated user is an admin). | | `read_repository` | [GitLab 10.7](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/17894) | Allows read-only access (pull) to the repository through `git clone`. | | `write_repository` | [GitLab 11.11](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/26021) | Allows read-write access (pull, push) to the repository through `git clone`. Required for accessing Git repositories over HTTP when 2FA is enabled. | -[2fa]: ../account/two_factor_authentication.md -[api]: ../../api/README.md -[ce-3749]: https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/3749 -[container registry]: ../packages/container_registry/index.md -[users]: ../../api/users.md -[usage]: ../../api/README.md#personal-access-tokens - <!-- ## Troubleshooting Include any troubleshooting steps that you can foresee. If you know beforehand what issues diff --git a/lib/gitlab/database/batch_count.rb b/lib/gitlab/database/batch_count.rb index 3eb0197d178..2359dceae48 100644 --- a/lib/gitlab/database/batch_count.rb +++ b/lib/gitlab/database/batch_count.rb @@ -37,6 +37,7 @@ module Gitlab MIN_REQUIRED_BATCH_SIZE = 1_250 MAX_ALLOWED_LOOPS = 10_000 SLEEP_TIME_IN_SECONDS = 0.01 # 10 msec sleep + ALLOWED_MODES = [:itself, :distinct].freeze # Each query should take < 500ms https://gitlab.com/gitlab-org/gitlab/-/merge_requests/22705 DEFAULT_DISTINCT_BATCH_SIZE = 10_000 @@ -55,8 +56,8 @@ module Gitlab def count(batch_size: nil, mode: :itself, start: nil, finish: nil) raise 'BatchCount can not be run inside a transaction' if ActiveRecord::Base.connection.transaction_open? - raise "The mode #{mode.inspect} is not supported" unless [:itself, :distinct].include?(mode) - raise 'Use distinct count for optimized distinct counting' if @relation.limit(1).distinct_value.present? && mode != :distinct + + check_mode!(mode) # non-distinct have better performance batch_size ||= mode == :distinct ? DEFAULT_DISTINCT_BATCH_SIZE : DEFAULT_BATCH_SIZE @@ -102,6 +103,12 @@ module Gitlab def actual_finish(finish) finish || @relation.maximum(@column) || 0 end + + def check_mode!(mode) + raise "The mode #{mode.inspect} is not supported" unless ALLOWED_MODES.include?(mode) + raise 'Use distinct count for optimized distinct counting' if @relation.limit(1).distinct_value.present? && mode != :distinct + raise 'Use distinct count only with non id fields' if @column == :id && mode == :distinct + end end end end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index dc4de9b1906..3922f5c6683 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -1178,8 +1178,147 @@ into similar problems in the future (e.g. when new tables are created). end end + # Returns the name for a check constraint + # + # type: + # - Any value, as long as it is unique + # - Constraint names are unique per table in Postgres, and, additionally, + # we can have multiple check constraints over a column + # So we use the (table, column, type) triplet as a unique name + # - e.g. we use 'max_length' when adding checks for text limits + # or 'not_null' when adding a NOT NULL constraint + # + def check_constraint_name(table, column, type) + identifier = "#{table}_#{column}_check_#{type}" + # Check concurrent_foreign_key_name() for info on why we use a hash + hashed_identifier = Digest::SHA256.hexdigest(identifier).first(10) + + "check_#{hashed_identifier}" + end + + def check_constraint_exists?(table, constraint_name) + # Constraint names are unique per table in Postgres, not per schema + # Two tables can have constraints with the same name, so we filter by + # the table name in addition to using the constraint_name + check_sql = <<~SQL + SELECT COUNT(*) + FROM pg_constraint + JOIN pg_class ON pg_constraint.conrelid = pg_class.oid + WHERE pg_constraint.contype = 'c' + AND pg_constraint.conname = '#{constraint_name}' + AND pg_class.relname = '#{table}' + SQL + + connection.select_value(check_sql).positive? + end + + # Adds a check constraint to a table + # + # This method is the generic helper for adding any check constraint + # More specialized helpers may use it (e.g. add_text_limit or add_not_null) + # + # This method only requires minimal locking: + # - The constraint is added using NOT VALID + # This allows us to add the check constraint without validating it + # - The check will be enforced for new data (inserts) coming in + # - If `validate: true` the constraint is also validated + # Otherwise, validate_check_constraint() can be used at a later stage + # - Check comments on add_concurrent_foreign_key for more info + # + # table - The table the constraint will be added to + # check - The check clause to add + # e.g. 'char_length(name) <= 5' or 'store IS NOT NULL' + # constraint_name - The name of the check constraint (otherwise auto-generated) + # Should be unique per table (not per column) + # validate - Whether to validate the constraint in this call + # + # rubocop:disable Gitlab/RailsLogger + def add_check_constraint(table, check, constraint_name, validate: true) + # Transactions would result in ALTER TABLE locks being held for the + # duration of the transaction, defeating the purpose of this method. + if transaction_open? + raise 'add_check_constraint can not be run inside a transaction' + end + + if check_constraint_exists?(table, constraint_name) + warning_message = <<~MESSAGE + Check constraint was not created because it exists already + (this may be due to an aborted migration or similar) + table: #{table}, check: #{check}, constraint name: #{constraint_name} + MESSAGE + + Rails.logger.warn warning_message + else + # Only add the constraint without validating it + # Even though it is fast, ADD CONSTRAINT requires an EXCLUSIVE lock + # Use with_lock_retries to make sure that this operation + # will not timeout on tables accessed by many processes + with_lock_retries do + execute <<-EOF.strip_heredoc + ALTER TABLE #{table} + ADD CONSTRAINT #{constraint_name} + CHECK ( #{check} ) + NOT VALID; + EOF + end + end + + if validate + validate_check_constraint(table, constraint_name) + end + end + + def validate_check_constraint(table, constraint_name) + unless check_constraint_exists?(table, constraint_name) + raise missing_schema_object_message(table, "check constraint", constraint_name) + end + + disable_statement_timeout do + # VALIDATE CONSTRAINT only requires a SHARE UPDATE EXCLUSIVE LOCK + # It only conflicts with other validations and creating indexes + execute("ALTER TABLE #{table} VALIDATE CONSTRAINT #{constraint_name};") + end + end + + def remove_check_constraint(table, constraint_name) + # DROP CONSTRAINT requires an EXCLUSIVE lock + # Use with_lock_retries to make sure that this will not timeout + with_lock_retries do + execute <<-EOF.strip_heredoc + ALTER TABLE #{table} + DROP CONSTRAINT IF EXISTS #{constraint_name} + EOF + end + end + + # Migration Helpers for adding limit to text columns + def add_text_limit(table, column, limit, constraint_name: nil, validate: true) + add_check_constraint( + table, + "char_length(#{column}) <= #{limit}", + text_limit_name(table, column, name: constraint_name), + validate: validate + ) + end + + def validate_text_limit(table, column, constraint_name: nil) + validate_check_constraint(table, text_limit_name(table, column, name: constraint_name)) + end + + def remove_text_limit(table, column, constraint_name: nil) + remove_check_constraint(table, text_limit_name(table, column, name: constraint_name)) + end + + def check_text_limit_exists?(table, column, constraint_name: nil) + check_constraint_exists?(table, text_limit_name(table, column, name: constraint_name)) + end + private + def text_limit_name(table, column, name: nil) + name.presence || check_constraint_name(table, column, 'max_length') + end + def missing_schema_object_message(table, type, name) <<~MESSAGE Could not find #{type} "#{name}" on table "#{table}" which was referenced during the migration. diff --git a/spec/lib/gitlab/database/batch_count_spec.rb b/spec/lib/gitlab/database/batch_count_spec.rb index 7842323d009..7be84b8f980 100644 --- a/spec/lib/gitlab/database/batch_count_spec.rb +++ b/spec/lib/gitlab/database/batch_count_spec.rb @@ -86,10 +86,6 @@ describe Gitlab::Database::BatchCount do end describe '#batch_distinct_count' do - it 'counts with :id field' do - expect(described_class.batch_distinct_count(model, :id)).to eq(5) - end - it 'counts with column field' do expect(described_class.batch_distinct_count(model, column)).to eq(2) end @@ -137,6 +133,12 @@ describe Gitlab::Database::BatchCount do it 'returns fallback if batch size is less than min required' do expect(described_class.batch_distinct_count(model, column, batch_size: small_batch_size)).to eq(fallback) end + + it 'will raise an error if distinct count with the :id column is requested' do + expect do + described_class.batch_count(described_class.batch_distinct_count(model, :id)) + end.to raise_error 'Use distinct count only with non id fields' + end end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 8b765ce122d..3db9320c021 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -2046,4 +2046,333 @@ describe Gitlab::Database::MigrationHelpers do model.bulk_migrate_in(10.minutes, [%w(Class hello world)]) 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::Base.connection.execute( + 'ALTER TABLE projects ADD CONSTRAINT check_1 CHECK (char_length(path) <= 5) NOT VALID' + ) + end + + after do + ActiveRecord::Base.connection.execute( + 'ALTER TABLE projects DROP CONSTRAINT IF EXISTS check_1' + ) + 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 + end + + describe '#add_check_constraint' do + before do + allow(model).to receive(:check_constraint_exists?).and_return(false) + 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(:execute).with(/statement_timeout/) + 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 ALL/) + + 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(:execute).with(/statement_timeout/) + 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 ALL/) + + 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(:execute).with(/statement_timeout/) + expect(model).to receive(:execute).ordered.with(validate_sql) + expect(model).to receive(:execute).ordered.with(/RESET ALL/) + + model.validate_check_constraint(:test_table, 'check_name') + end + end + end + + describe '#remove_check_constraint' do + 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 '#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 end diff --git a/spec/models/project_services/chat_message/pipeline_message_spec.rb b/spec/models/project_services/chat_message/pipeline_message_spec.rb index 4210b52a8b9..e99148d1d1f 100644 --- a/spec/models/project_services/chat_message/pipeline_message_spec.rb +++ b/spec/models/project_services/chat_message/pipeline_message_spec.rb @@ -431,6 +431,57 @@ describe ChatMessage::PipelineMessage do end end + context "when jobs succeed on retries" do + before do + args[:builds] = [ + { id: 1, name: "job-1", status: "failed", stage: "stage-1" }, + { id: 2, name: "job-2", status: "failed", stage: "stage-2" }, + { id: 3, name: "job-3", status: "failed", stage: "stage-3" }, + { id: 7, name: "job-1", status: "failed", stage: "stage-1" }, + { id: 8, name: "job-1", status: "success", stage: "stage-1" } + ] + end + + it "do not return a job which succeeded on retry" do + expected_jobs = [ + "<http://example.gitlab.com/-/jobs/3|job-3>", + "<http://example.gitlab.com/-/jobs/2|job-2>" + ] + + expect(subject.attachments.first[:fields][3]).to eq( + title: "Failed jobs", + value: expected_jobs.join(", "), + short: true + ) + end + end + + context "when jobs failed even on retries" do + before do + args[:builds] = [ + { id: 1, name: "job-1", status: "failed", stage: "stage-1" }, + { id: 2, name: "job-2", status: "failed", stage: "stage-2" }, + { id: 3, name: "job-3", status: "failed", stage: "stage-3" }, + { id: 7, name: "job-1", status: "failed", stage: "stage-1" }, + { id: 8, name: "job-1", status: "failed", stage: "stage-1" } + ] + end + + it "returns only first instance of the failed job" do + expected_jobs = [ + "<http://example.gitlab.com/-/jobs/3|job-3>", + "<http://example.gitlab.com/-/jobs/2|job-2>", + "<http://example.gitlab.com/-/jobs/1|job-1>" + ] + + expect(subject.attachments.first[:fields][3]).to eq( + title: "Failed jobs", + value: expected_jobs.join(", "), + short: true + ) + end + end + context "when the CI config file contains a YAML error" do let(:has_yaml_errors) { true } |