From e10120894d85e3fa09e7d2acbb59810060993f09 Mon Sep 17 00:00:00 2001 From: Abdul Wadood Date: Fri, 9 Feb 2024 11:35:20 +0530 Subject: [PATCH] Update guidelines to add a new application setting To add a new rate limit or an application setting we should use a JSONB column to store the new setting. It has the following advantages: 1. Prevents the `application_settings` from getting wider. Postgres has a limit of 1600 columns. 2. We can continue using the rails validations. 3. Store different data types. 4. Adding a new setting to an existing column won't a review from the database team. --- doc/development/application_limits.md | 4 +++- doc/development/application_settings.md | 13 ++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/doc/development/application_limits.md b/doc/development/application_limits.md index cd048a5503e082..b0422517daa2e2 100644 --- a/doc/development/application_limits.md +++ b/doc/development/application_limits.md @@ -167,7 +167,9 @@ This applies to Rails controllers, Grape endpoints, and any other Rack requests. The process for adding a new throttle is loosely: -1. Add new columns to the `ApplicationSetting` model (`*_enabled`, `*_requests_per_period`, `*_period_in_seconds`). +1. Add new fields to the [rate_limits JSONB column](https://gitlab.com/gitlab-org/gitlab/-/blob/63b37287ae028842fcdcf56d311e6bb0c7e09e79/app/models/application_setting.rb#L603) +in the `ApplicationSetting` model. +1. Update the JSON schema validator for the [rate_limits column](https://gitlab.com/gitlab-org/gitlab/-/blob/63b37287ae028842fcdcf56d311e6bb0c7e09e79/app/validators/json_schemas/application_setting_rate_limits.json). 1. Extend `Gitlab::RackAttack` and `Gitlab::RackAttack::Request` to configure the new rate limit, and apply it to the desired requests. 1. Add the new settings to the Admin Area form in `app/views/admin/application_settings/_ip_limits.html.haml`. diff --git a/doc/development/application_settings.md b/doc/development/application_settings.md index a51272617e8b31..0c945840cae2d0 100644 --- a/doc/development/application_settings.md +++ b/doc/development/application_settings.md @@ -16,9 +16,20 @@ Application settings are stored in the `application_settings` table. Each settin First of all, you have to decide if it is necessary to add an application setting. Consider our [configuration principles](https://handbook.gitlab.com/handbook/product/product-principles/#configuration-principles) when adding a new setting. +We prefer saving the related application settings in a single JSONB column to avoid making the `application_settings` +table wider. Also, adding a new setting to an existing column won't require a database review so it saves time. + To add a new setting, you have to: -- Add a new column to the `application_settings` table. +- Check if there is an existing JSONB column that you can use to store the new setting. +- If there is an existing JSON column then: + - Add new setting to the JSONB column like [rate_limits](https://gitlab.com/gitlab-org/gitlab/-/blob/63b37287ae028842fcdcf56d311e6bb0c7e09e79/app/models/application_setting.rb#L603) + in the `ApplicationSetting` model. + - Update the JSON schema validator for the column like [rate_limits validator](https://gitlab.com/gitlab-org/gitlab/-/blob/63b37287ae028842fcdcf56d311e6bb0c7e09e79/app/validators/json_schemas/application_setting_rate_limits.json). +- If there isn't an existing JSON column which you can use then: + - Add a new JSON column to the `application_settings` table to store, see this [merge request](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/140633/diffs) for reference. + - Add a constraint to ensure the column always stores a hash, see this [merge request](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141765/diffs) for reference. + - Create follow-up issues to move existing related columns to this newly created JSONB column. - Add the new setting to the [list of visible attributes](https://gitlab.com/gitlab-org/gitlab/-/blob/6f33ad46ffeac454c6c9ce92d6ba328a72f062fd/app/helpers/application_settings_helper.rb#L215). - Add the new setting to it to [`ApplicationSettingImplementation#defaults`](https://gitlab.com/gitlab-org/gitlab/-/blob/6f33ad46ffeac454c6c9ce92d6ba328a72f062fd/app/models/application_setting_implementation.rb#L36), if the setting has a default value. - Add a [test for the default value](https://gitlab.com/gitlab-org/gitlab/-/blob/6f33ad46ffeac454c6c9ce92d6ba328a72f062fd/spec/models/application_setting_spec.rb#L20), if the setting has a default value. -- GitLab