-
Notifications
You must be signed in to change notification settings - Fork 18
Improve database management documentation #593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
23249a8
9b228ca
8dc9c0b
aa93e62
03bd4c6
5955f6e
e8fb3e4
2e3ef06
654bbe2
f1eea3a
77ea379
abf861d
8618427
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
label: "Database Migrations" | ||
position: 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
--- | ||
sidebar_position: 2 | ||
--- | ||
|
||
# Entity Framework | ||
|
||
:::info | ||
|
||
For instructions on how to apply database migrations, please refer to the | ||
[Getting Started](../../getting-started/server/database/ef/index.mdx#updating-the-database) | ||
documentation. | ||
|
||
::: | ||
|
||
If you alter the database schema, you must create an EF migration script to ensure that EF databases | ||
keep pace with these changes. Developers must do this and include the migrations with their PR. | ||
|
||
To create these scripts, you must first update your data model in `Core/Entities` as desired. This | ||
will be used to generate the migrations for each of our EF targets. Additionally, for table changes | ||
it is strongly recommended to define or update an `IEntityTypeConfiguration<T>` to accurately | ||
represent any constraints needed on the data model. | ||
|
||
Once the model is updated, navigate to the `dev` directory in the `server` repo and execute the | ||
`ef_migrate.ps1` PowerShell command. You should provide a name for the migration as the only | ||
parameter: | ||
|
||
```bash | ||
pwsh ef_migrate.ps1 [NAME_OF_MIGRATION] | ||
``` | ||
|
||
This will generate the migrations, which should then be included in your PR. |
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,98 @@ | ||||||||||
--- | ||||||||||
sidebar_position: 1 | ||||||||||
--- | ||||||||||
|
||||||||||
# MSSQL | ||||||||||
|
||||||||||
:::info | ||||||||||
|
||||||||||
For instructions on how to apply database migrations, please refer to the | ||||||||||
[Getting Started](../../getting-started/server/database/mssql/index.md#updating-the-database) | ||||||||||
documentation. | ||||||||||
|
||||||||||
::: | ||||||||||
|
||||||||||
:::tip | ||||||||||
|
||||||||||
We recommend reading [T-SQL Code Style](../code-style/sql.md) since it has a major impact in how we | ||||||||||
write migrations. | ||||||||||
|
||||||||||
::: | ||||||||||
|
||||||||||
## SQL database project | ||||||||||
|
||||||||||
We use a | ||||||||||
[SDK-style SQL project](https://learn.microsoft.com/en-us/sql/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects?view=sql-server-ver16) | ||||||||||
(`.sqlproj`) to develop the database locally -- this means we have an up-to-date representation of | ||||||||||
the database in `src/Sql`, and any modifications need to be represented there as well. SDK-style SQL | ||||||||||
projects are available in Visual Studio Code that provides schema comparison and more. You may also | ||||||||||
modify the `.sql` files directly with any text editor. | ||||||||||
|
||||||||||
To make a database change, start by modifying the `.sql` files in `src/Sql/dbo`. These changes will | ||||||||||
also need to be applied in a migration script. Migration scripts are located in | ||||||||||
`util/Migrator/DbScripts`. | ||||||||||
|
||||||||||
You can either generate the migration scripts automatically using the _Schema Comparison_ | ||||||||||
functionality or by manually writing them. Do note that the automatic method will only take you so | ||||||||||
far and it will need to be manually edited to adhere to the code styles. | ||||||||||
|
||||||||||
For added safeguards we have automated linting and validation to ensure the SQL project is always up | ||||||||||
to date with the migrations. | ||||||||||
|
||||||||||
The separate database definitions in `src/Sql/.../dbo` serve as a "master" reference for the | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โ๏ธ No need for scare quotes here.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's an analogy to a master copy of something, like an album recording. This content just moved its position by the way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I know. "The source of truth" is both more inclusive than "master reference", and more accurate as a description. |
||||||||||
intended and final state of the database at that time. This is crucial because the state of database | ||||||||||
definitions at the current moment may differ from when a migration was added in the past. These | ||||||||||
definitions act as a lint and validation step to ensure that migrations work as expected, and the | ||||||||||
separation helps maintain clarity and accuracy in database schema management and synchronization | ||||||||||
processes. | ||||||||||
|
||||||||||
## Modifying the database | ||||||||||
|
||||||||||
In accordance with the tenets of [Evolutionary Database Design](./edd.mdx) every change must be | ||||||||||
considered as split into two parts: | ||||||||||
|
||||||||||
1. A backwards-compatible transition migration | ||||||||||
2. A non-backwards-compatible final migration | ||||||||||
|
||||||||||
Most changes are entirely backwards-compatible in their final form. If this is the case, only one | ||||||||||
phase of changes is required. With the use of beta testing, partial roll-outs, | ||||||||||
[feature flags](../feature-flags.md), etc. the often-chosen path is to spread a change across | ||||||||||
several major releases with a calculated future state that can perform a "cleanup" migration that is | ||||||||||
backwards-compatible but still represents an overall-_incompatible_ change beyond the boundaries of | ||||||||||
what we need for individual release safety. | ||||||||||
|
||||||||||
### Backwards compatible migration | ||||||||||
|
||||||||||
1. Modify the source `.sql` files in `src/Sql/dbo`. | ||||||||||
2. Write a migration script, and place it in `util/Migrator/DbScripts`. Each script must be prefixed | ||||||||||
with the current date. | ||||||||||
|
||||||||||
Tips to ensure backwards compatibility: | ||||||||||
|
||||||||||
- any existing stored procedure accepts the same input parameters and that new parameters have | ||||||||||
nullable defaults | ||||||||||
- when a column is renamed the existing stored procedures first check (coalesce) the new location | ||||||||||
before falling back to the old location | ||||||||||
- continued updating of the old data columns since in case of a rollback no data should be lost | ||||||||||
|
||||||||||
### Non-backwards compatible migration | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ค I think we should use affirmative language here. The 'non-backwards compatible migration' is talking about a 'breaking change'.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this, but let's make sure to update everywhere, not just this heading There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I support the current term. The point of this documentation is that your change can't go backwards. It may be "breaking" to some, but the intention of the documentation is about direction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use affirmative language to describe that. The EDD page uses "finalization migration" as its term for this phase.
Suggested change
|
||||||||||
|
||||||||||
These changes should be written from the perspective of "all data has been migrated" and any old | ||||||||||
stored procedures that were kept around for backwards compatibility should be removed. Any logic for | ||||||||||
syncing old and new data should also be removed in this step. | ||||||||||
|
||||||||||
Since the `dbo` schema represents the current state we need to introduce a "future" state that we | ||||||||||
will call `dbo_finalization`. | ||||||||||
|
||||||||||
1. Copy the relevant `.sql` files from `src/Sql/dbo` to `src/Sql/dbo_finalization`. | ||||||||||
2. Remove the backwards compatibility logic that is no longer needed e.g. dual reads and writes to | ||||||||||
columns. | ||||||||||
3. Write a new Migration and place it in `src/Migrator/DbScripts_finalization`. Name it | ||||||||||
`YYYY-0M-FinalizationMigration.sql`. | ||||||||||
- Typically migrations are designed to be run in sequence. However since the migrations in | ||||||||||
`DbScripts_finalization` can be run out of order, care must be taken to ensure they remain | ||||||||||
compatible with the changes to `DbScripts`. In order to achieve this we only keep a single | ||||||||||
migration, which executes all backwards incompatible schema changes. | ||||||||||
Comment on lines
+94
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that Is the finalization script added at the same time as the backwards-compatible change? So there is only 1 finalization script at a time, it is always moved to (not being overly critical - just trying to understand this process given we've never really done it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. It's always been mentioned. You break up a pure DB non backwards compatible change into two steps. An example would be the column rename example in https://contributing.bitwarden.com/contributing/database-migrations/edd#migrations-1. Initial migration would:
Transition migration would:
Finalization migration (run at next deploy):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do note we technically never have to remove the old column but not doing so adds database debt since we have
This is why we'd like to automate it since relying on human elements here is dangerous since I could easily be OOO for a release or two or forget about cleaning it up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This actually needs to be run 2 deploys away from the initial migration to maintain our self-host server guarantees. We can apply it earlier on our infrastructure.
Unless doing so would cross the the 8kb maximum record length or 1,024 column limit. I'd expect something will cross the 8kb maximum record length if we never remove columns. It'd be much harder to hit the column limit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could've been clearer in my question - my previous understanding was that you might have several scripts in That said, it is unusual that multiple teams would append to the same file so this might be worth stating more explicitly, e.g.
Rather than "Write a new migration, put it here, name it this" but then saying "we only keep a single migration". Or if I've misunderstood please let me know. |
||||||||||
|
||||||||||
Upon execution any finalization scripts will be [automatically moved](./edd.mdx#online-environments) | ||||||||||
for proper history. | ||||||||||
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.