Add new jsonb column to store settings for default branch protection
What does this MR do and why?
Currently, the options for default branch protection at the instance and group level lag behind, and are not as fine-grained, as the options available from the Protected Branches feature
The branch defaults lag behind because the current implementation uses an integer column to store the default settings, which becomes difficult to expand as new options and finer detail controls are added to the protected branches feature
at a high-ish level the existing feature is implemented as:
- there are integer columns on the database tables (application level and group level)
- these integers are mapped to a subset of protected branch settings
- these mappings defined in access.rb
- mappings are used here via
BranchProtection
helper
- when creating a default branch, those settings are passed into the protected branch service
To offer better support of protected branches features on the default branch, we can:
- use a jsonb column rather than an integer
- update the settings API to accept a payload the matches the protected branches API
- Pass those settings into the ProtecteBrancheService
This commit performs step 1, by adding a new jsonb
column
resolves: #408150 (closed) Changelog: added
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %16.1
assigned to @wandering_person
- A deleted user
added databasereview pending label
4 Warnings a4329108: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 37a0f878: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. b84c0e5a: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
1 Message This merge request adds or changes files that require a review from the Database team. This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
-
Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
-
Prepare your MR for database review according to the docs.
-
Assign and mention the database reviewer suggested by Reviewer Roulette.
-
Kick off the
db:gitlabcom-database-testing
manual job. This job can also be used before requesting review to test your migrations against production data.
The following files require a review from the Database team:
db/migrate/20230606124754_add_default_branch_protections_json_to_application_settings.rb
db/migrate/20230606124854_add_default_branch_protections_json_to_namespace_settings.rb
db/migrate/20230606124855_add_size_constraint_to_namespace_settings_json.rb
db/migrate/20230606124856_add_size_constraint_to_application_settings_json.rb
db/schema_migrations/20230606124754
db/schema_migrations/20230606124854
db/schema_migrations/20230606124855
db/schema_migrations/20230606124856
db/structure.sql
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Serena Fang (
@serenafang
) (UTC-5, 12 hours behind@wandering_person
)Mark Chao (
@lulalala
) (UTC+8, 1 hour ahead of@wandering_person
)database Sincheol (David) Kim (
@dskim_gitlab
) (UTC+9.5, 2.5 hours ahead of@wandering_person
)Mayra Cabrera (
@mayra-cabrera
) (UTC-6, 13 hours behind@wandering_person
)~"migration" No reviewer available No maintainer available To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- A deleted user
added Data WarehouseImpact Check label
added 1 commit
- d9b25550 - Add new jsonb column to store settings for default branch protection
added 1 commit
- 582f5b52 - Add new jsonb column to store settings for default branch protection
@ibaum and @jon_jenkins would you be able to do the initial backend and database reviews? thanks!
requested review from @jon_jenkins
requested review from @ibaum
- Resolved by Michael Becker
@wandering_person I think this looks good from a backend perspective, approving
removed review request for @ibaum
@ibaum
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
requested review from @ayufan
- Resolved by Michael Becker
- Resolved by Michael Becker
removed review request for @ayufan
removed review request for @jon_jenkins
added 2018 commits
-
582f5b52...1c610394 - 2016 commits from branch
master
- ee2f1727 - Add new jsonb column to store settings for default branch protection
- 62251a8a - Move namespace level column to the `namespace_settings` table
-
582f5b52...1c610394 - 2016 commits from branch
added 151 commits
-
62251a8a...9ebaaeea - 149 commits from branch
master
- f3815cee - Add new jsonb column to store settings for default branch protection
- 04d47744 - Move namespace level column to the `namespace_settings` table
-
62251a8a...9ebaaeea - 149 commits from branch
added 1 commit
- 39d015c5 - Move namespace level column to the `namespace_settings` table
added 1 commit
- e3067815 - Move namespace level column to the `namespace_settings` table
requested review from @ayufan and @jon_jenkins
LGTM! Passing on to @DylanGriffith for database maintainer review.
added databasereviewed label and removed databasereview pending label
requested review from @DylanGriffith and removed review request for @jon_jenkins
added Data WarehouseNot Impacted label and removed Data WarehouseImpact Check label
- Resolved by Michael Becker
@wandering_person I feel like I need some more context on why this needs to be a JSON column rather than relying on modelling the features directly in normal Postgres types/tables/columns . I realise the JSONB can seem like it is much more efficient but excessive use of JSON can become more difficult maintenance over time. This forces us to additionally define schemas in the JSON schema validation but that doesn't give the same guarantees as Postgres constraints. Migrations can become more difficult with JSON columns because you can't guarantee the current schema unless you had Postgres constraints. Also there are additional performance considerations with storing JSONB columns because they can end up much larger than integers, end up in a TOAST column which then makes reading the rows from tables like
namespaces
much less performant as you have to lookup another TOAST table.I don't think we have a hard and fast rule about when to use JSONB but my instinct is to use it sparingly and only for cases where modelling the data statically is impossible or when the backend doesn't really care about the contents.
To help me understand this a little better perhaps you can expand a little on:
- What is the next feature(s) you want to build on top of this
- What would the modelling look like without JSONB
- What would the modelling look like with JSONB
Started database testing pipeline (limited access). This comment will be updated once the pipeline has finished running.
- Resolved by Michael Becker
- Resolved by Michael Becker
Database migrations (on the main database)
Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).
Migration Type Total runtime Result DB size change 20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings Regular 1.5 s +0.00 B 20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings Regular 1.5 s +0.00 B 20230524124855 - AddSizeConstraintToNamespaceSettingsJson Regular 1.7 s +0.00 B 20230524124856 - AddSizeConstraintToApplicationSettingsJson Regular 1.7 s +0.00 B Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 14 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings
- Type: Regular
- Duration: 1.5 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 7.8 ms 7.8 ms 7.8 ms 0 ALTER TABLE "application_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
1 0.0 ms 0.0 ms 0.0 ms 1 SELECT $1::regtype::oid
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AddDefaultBranchProtectionsJsonToApplicationSettings
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 4 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings
- Type: Regular
- Duration: 1.5 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 3.1 ms 3.1 ms 3.1 ms 0 ALTER TABLE "namespace_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
1 0.0 ms 0.0 ms 0.0 ms 1 SELECT $1::regtype::oid
Histogram for AddDefaultBranchProtectionsJsonToNamespaceSettings
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 4 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124855 - AddSizeConstraintToNamespaceSettingsJson
- Type: Regular
- Duration: 1.7 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 0.7 ms 0.7 ms 0.7 ms 0 ALTER TABLE namespace_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AddSizeConstraintToNamespaceSettingsJson
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 3 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124856 - AddSizeConstraintToApplicationSettingsJson
- Type: Regular
- Duration: 1.7 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 1.9 ms 1.9 ms 1.9 ms 0 ALTER TABLE application_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AddSizeConstraintToApplicationSettingsJson
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 3 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Other information
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change 20230522103433 - RemoveGitHubImportDeprecatedWorkers Regular 1.7 s +0.00 B 20230524095108 - RemoveIndexOnNameOnOrganization Regular 2.6 s -16.00 KiB 20230522220709 - EnsureIncidentWorkItemTypeBackfillIsFinished Post deploy 1.7 s +0.00 B 20230522225610 - RemoveTmpIndexIssuesOnIssueTypeAndIdOnlyIncidents Post deploy 3.3 s -8.86 MiB 20230523131914 - RecreateIndexOnVulnerabilityReads Post deploy 179.9 s -118.62 MiB 20230523132647 - RecreateIndexOnVulnerabilityReads2 Post deploy 179.1 s -112.62 MiB Clone details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-1977426-10184547-main
2023-05-30T04:47:33Z 2023-05-27T02:59:07Z 2023-05-30 16:58:16 +0000 database-testing-1977426-10184547-ci
2023-05-30T04:47:33Z 2023-05-26T15:37:27Z 2023-05-30 16:58:16 +0000 Database migrations (on the ci database)
Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).
Migration Type Total runtime Result DB size change 20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings Regular 2.1 s +0.00 B 20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings Regular 2.1 s +0.00 B 20230524124855 - AddSizeConstraintToNamespaceSettingsJson Regular 2.3 s +0.00 B 20230524124856 - AddSizeConstraintToApplicationSettingsJson Regular 2.3 s +0.00 B Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 14 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings
- Type: Regular
- Duration: 2.1 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 7.3 ms 7.3 ms 7.3 ms 0 ALTER TABLE "application_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
1 0.0 ms 0.0 ms 0.0 ms 1 SELECT $1::regtype::oid
Histogram for AddDefaultBranchProtectionsJsonToApplicationSettings
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 4 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings
- Type: Regular
- Duration: 2.1 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 4.0 ms 4.0 ms 4.0 ms 0 ALTER TABLE "namespace_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
1 0.0 ms 0.0 ms 0.0 ms 1 SELECT $1::regtype::oid
Histogram for AddDefaultBranchProtectionsJsonToNamespaceSettings
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 4 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124855 - AddSizeConstraintToNamespaceSettingsJson
- Type: Regular
- Duration: 2.3 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 3.0 ms 3.0 ms 3.0 ms 0 ALTER TABLE namespace_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AddSizeConstraintToNamespaceSettingsJson
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 3 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20230524124856 - AddSizeConstraintToApplicationSettingsJson
- Type: Regular
- Duration: 2.3 s
- Database size change: +0.00 B
Calls Total Time Max Time Mean Time Rows Query 1 5.0 ms 5.0 ms 5.0 ms 0 ALTER TABLE application_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
2 0.0 ms 0.0 ms 0.0 ms 2 SELECT pg_backend_pid()
Histogram for AddSizeConstraintToApplicationSettingsJson
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 3 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Other information
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change 20230522103433 - RemoveGitHubImportDeprecatedWorkers Regular 2.3 s +0.00 B 20230524095108 - RemoveIndexOnNameOnOrganization Regular 2.5 s -8.00 KiB 20230522220709 - EnsureIncidentWorkItemTypeBackfillIsFinished Post deploy 2.0 s +0.00 B 20230522225610 - RemoveTmpIndexIssuesOnIssueTypeAndIdOnlyIncidents Post deploy 3.9 s -8.00 KiB 20230523131914 - RecreateIndexOnVulnerabilityReads Post deploy 4.2 s +0.00 B 20230523132647 - RecreateIndexOnVulnerabilityReads2 Post deploy 4.2 s +0.00 B Clone details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-1977426-10184547-main
2023-05-30T04:47:33Z 2023-05-27T02:59:07Z 2023-05-30 16:58:16 +0000 database-testing-1977426-10184547-ci
2023-05-30T04:47:33Z 2023-05-26T15:37:27Z 2023-05-30 16:58:16 +0000
Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic
- A deleted user
added database-testing-automation label
@wandering_person I left some thoughts. The migration looks fine to me but my main comment is just around understanding why JSONB is important for this use case. In my experience the maintenance of schemaless columns like this tends to end up being more effort than just getting all the tables in place from the beginning.
removed review request for @DylanGriffith
requested review from @DylanGriffith
removed review request for @DylanGriffith
removed review request for @ayufan
added 664 commits
-
ed2e610b...f6872ca1 - 660 commits from branch
master
- 520e1aac - Add new jsonb column to store settings for default branch protection
- 3b9a7760 - Move namespace level column to the `namespace_settings` table
- 2ef3644f - Make json schema stricter
- b0be7526 - Add db-level size constraint to jsonb columns
Toggle commit list-
ed2e610b...f6872ca1 - 660 commits from branch
- A deleted user
added documentation label