Skip to content
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

enhancement(database)!: add db identifier shortening and migration #19732

Merged
merged 92 commits into from Mar 25, 2024

Conversation

innerdvations
Copy link
Contributor

@innerdvations innerdvations commented Mar 11, 2024

What does it do?

  • Enables the name shortening algorithm in the database
  • adds an internal migration to ensure all identifiers are renamed as gracefully as possible so no data will be lost (note that foreign key indexes are currently not renamed properly, and in the case of sqlite, all indexes must be dropped and recreated because they do not support renaming indexes)

TODO

These will likely come in another PR before v5 release.

  • find a way to rename foreign key indexes so that they don't have to be recreated by the db schema sync
  • add more tests, particularly testing snake case names in relations

Why is it needed?

To finally close #13117 🥳

How to test it?

Simple version: starting with a Strapi 4 project, switch to this branch and start Strapi. The migration should run, and any of your database identifiers (table names, column names, indexes) that were longer than 55 characters will be renamed to fit into that length. Additionally, identifier parts such as "components" will have been shortened to "cmp", "order_inv_fk" to "oifk", etc.

Detailed version:

What is currently even possible to test will depend on the database being used. By default, postgres has a limit of 63 characters and mysql has a limit of 65, sqlite is unlimited.

  • Start with a Strapi 4 project (from develop)
  • add some content types with names that are at least 56 characters long
  • add some attribute names (of all types, including relations, components, and dynamic zones) that are at least 56 characters long (postgres+mysql might require being in a content type name that is only a couple characters for that to work, it will take some trial and error to find the right balance for what strapi 4 will accept)
  • in those, include some names that include numbers and/or other things that will test snakeCasing
  • add a full range of content to each of those
  • [sqlite only] create content types and attributes with names that are > 63 characters long
  • at this point, you might want to do a dump of the database so that you can restore to it and test again later
  • Switch to this branch + build
  • Start up the Strapi project
  • no data should have been lost
  • now create content types and attributes of all types with names longer than 65 characters (the previous hard limit within the dbs)
  • they should all work!

You should also see the following changes in your database:

  • every table name longer than 55 characters should be shortened to 55
  • every column name longer than 55 characters should be shortened to 55
  • every index name longer than 55 characters should be shortened to 55
  • prefixes and suffixes should also be using shortened forms, eg "oifk" instead of "order_inv_fk"
  • absolutely no data loss should happen

If you notice any identifiers in the database longer than 55 characters, or any data loss, please report details here.

Additionally, tests have already been provided in previous PRs for all the naming functions and createMetadata itself, covering every case we could think of comparing the full length to the expected shortened length, but since there are potentially edge cases that were missed, if you notice any please let me know.

Related issue(s)/PR(s)

Let us know if this is related to any issue/pull request

@innerdvations innerdvations self-assigned this Mar 11, 2024
@innerdvations innerdvations changed the title feat(database): add migration feat(database): add migration for db identifier shortening Mar 11, 2024
@@ -34,7 +34,7 @@ const createColumn = (name: string, attribute: Attribute): Column => {
};
};

const createTable = (meta: Meta): Table => {
const createTable = (meta: Meta, options: MetadataOptions): Table => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid exposing & passing arround the metadata options could you store them in the metadata and expose a meta.getOptions() fn instead to keep a bit of encapsulation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or it's a matter of using another type :)

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did a very quick read. pretty nice 👍

Note: We definitely want to create an identifierProvider that we can init with options instead of passing the options in each function call :)

@innerdvations
Copy link
Contributor Author

innerdvations commented Mar 22, 2024

Just did some last minute QA and there are some indexes that aren't being renamed, at least on sqlite, even though the rename is called on them. If it's -only- on sqlite that's fine and we can ignore it, because they're not actually renamed anyway, but if it's mysql and postgres I would like to fix that to avoid having to recreate indexes unneccessarily. I'll look into it on Monday

@innerdvations innerdvations merged commit fd7c075 into v5/main Mar 25, 2024
23 of 26 checks passed
@innerdvations innerdvations deleted the enhancement/short-db-names-migration branch March 25, 2024 17:02
@echoes-hq echoes-hq bot added the echoes/type: feature/enhancement For enhancements to existing functionality and tools label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/type: feature/enhancement For enhancements to existing functionality and tools flag: 💥 Breaking change This PR contains breaking changes and should not be merged pr: enhancement This PR adds or updates some part of the codebase or features source: core:database Source is core/database package
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants