Skip to content

Exclude string and text arrays from Rubocop migration checks

What does this MR do?

This MR addresses minor issues with the Rubocop checks on migrations with text and string columns

There are currently two active Cops for checking text and string column definitions:

  1. The PreventStrings cop prevents strings columns from being defined and suggests using text columns.
  2. The AddLimitToTextColumns cop requires a limit to be added for all text columns.

Both of those rules do not apply to string/text arrays:

  1. Adding a limit to an array would result to limiting the number of elements, not their length. This is irrelevant to our case and, on top of that, a no-op in Postgres: 8.15.1. Declaration of Array Types

  2. Setting a limit in the definition of a string array properly limits the length of each string included in the array

    Using the following:

    add_column(:table, :column, :string, array: true, limit: 255, default: [])

    Results to:

    column character varying(255)[] DEFAULT '{}'::character varying[]

    Which is the intended behavior, limiting the length of each entry.

  3. In contrast we don't currently have a tool to support this functionality using text[].

    We would have to update add_text_limit to:

    1. Create a function that unnests the text array and checks each element
    2. Add a check constraint using that function

Until the need for such a complicated migration helper is proved, we can support the addition of textual arrays by using character varying(limit)[] or text[] without a limit set.

To do so, we have to at least first modify our cops to not complain for those valid and not otherwise supported use cases.

We can revisit them if we ever update our migration helpers to also support text arrays.

More info provided in related Issue: #217009 (closed)

Examples of string/text arrays in our database

As an example of cases this updated is relevant to, we are currently defining arrays of strings/texts in the following tables:

CREATE TABLE public.application_settings (
    ....
    outbound_local_requests_whitelist character varying(255)[] DEFAULT '{}'::character varying[] NOT NULL,

    protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session}'::character varying[],


    container_registry_features text[] DEFAULT '{}'::text[] NOT NULL,
    ....
)

CREATE TABLE public.cluster_providers_aws (
    ....
    subnet_ids character varying(255)[] DEFAULT '{}'::character varying[] NOT NULL,
    ....
)

CREATE TABLE public.alert_management_alerts (
    ....
    hosts text[] DEFAULT '{}'::text[] NOT NULL,
    ....
)

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by 🤖 GitLab Bot 🤖

Merge request reports

Loading