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:
- The
PreventStrings
cop preventsstrings
columns from being defined and suggests usingtext
columns. - The
AddLimitToTextColumns
cop requires a limit to be added for alltext
columns.
Both of those rules do not apply to string/text arrays:
-
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
-
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.
-
In contrast we don't currently have a tool to support this functionality using
text[]
.We would have to update
add_text_limit
to:- Create a function that unnests the text array and checks each element
- 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
- [-] Changelog entry
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides - [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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