Add latest changes from gitlab-org/gitlab@master

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

changelogs/unreleased/slack-notification-retry-success-skip.yml

+---
+title: Make pipeline info in chat notifications concise
+merge_request: 28284
+author:
+type: changed

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).

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

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)**
 

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
 

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:
 

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

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

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

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.

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

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

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 }