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

feat: support migrations in subdirectories #1226

Closed
wants to merge 4 commits into from

Conversation

mythmon
Copy link

@mythmon mythmon commented Jul 8, 2024

Adds support for migrations stored in subdirectories, which can help organize large groups of migrations. Also adds support for creating migrations in year-prefixed directories.

Fixes #932

@mythmon mythmon requested a review from Shinigami92 as a code owner July 8, 2024 23:05
@Shinigami92 Shinigami92 added c: feature Request for new feature p: 1-normal Nothing urgent labels Jul 9, 2024
@Shinigami92 Shinigami92 added this to the vAnytime milestone Jul 9, 2024
src/migration.ts Show resolved Hide resolved
test/runner.spec.ts Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jul 9, 2024

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 90.8% (🎯 90%)
⬇️ -1.23%
3150 / 3469
🟢 Statements 90.8% (🎯 90%)
⬇️ -1.23%
3150 / 3469
🟢 Functions 95.11% (🎯 90%)
⬇️ -0.73%
253 / 266
🟢 Branches 89.93% (🎯 85%)
⬇️ -0.10%
813 / 904
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Unchanged Files
src/db.ts 83.47% 88.46% 87.5% 83.47% 67-69, 99, 102-111, 113-114, 154-156
src/index.ts 100% 100% 100% 100%
src/migration.ts 64.63% 82.22% 57.14% 64.63% 70, 73-75, 77-90, 123-130, 132, 137-142, 145, 147-154, 157-158, 161-166, 168-169, 212-216, 218-223, 225-227, 229-231, 233-237, 257-258, 284-286, 294-295, 312-315, 344-345
src/migrationBuilder.ts 95.43% 92.85% 88.88% 95.43% 354-358, 497-499, 501-502, 526-527
src/runner.ts 68.15% 58.62% 80% 68.15% 46, 74-75, 84-85, 88-91, 93-96, 123-126, 128-131, 134-135, 177-180, 189-193, 206, 210-211, 213-215, 217-223, 242, 244-250, 253, 266, 278, 280-286, 288-291, 294-297, 307-308, 317-319, 328, 330, 332-339, 351-355, 357-358
src/sqlMigration.ts 90% 100% 80% 90% 45-46, 48-49
src/types.ts 100% 100% 100% 100%
src/operations/sql.ts 100% 100% 100% 100%
src/operations/casts/createCast.ts 100% 100% 100% 100%
src/operations/casts/dropCast.ts 100% 100% 100% 100%
src/operations/casts/index.ts 100% 100% 100% 100%
src/operations/domains/alterDomain.ts 100% 100% 100% 100%
src/operations/domains/createDomain.ts 100% 100% 100% 100%
src/operations/domains/dropDomain.ts 100% 100% 100% 100%
src/operations/domains/index.ts 100% 100% 100% 100%
src/operations/domains/renameDomain.ts 100% 100% 100% 100%
src/operations/domains/shared.ts 100% 100% 100% 100%
src/operations/extensions/createExtension.ts 100% 100% 100% 100%
src/operations/extensions/dropExtension.ts 100% 100% 100% 100%
src/operations/extensions/index.ts 100% 100% 100% 100%
src/operations/extensions/shared.ts 100% 100% 100% 100%
src/operations/functions/createFunction.ts 95.58% 94.44% 100% 95.58% 71-73
src/operations/functions/dropFunction.ts 100% 100% 100% 100%
src/operations/functions/index.ts 100% 100% 100% 100%
src/operations/functions/renameFunction.ts 100% 100% 100% 100%
src/operations/functions/shared.ts 100% 100% 100% 100%
src/operations/grants/grantOnSchemas.ts 100% 100% 100% 100%
src/operations/grants/grantOnTables.ts 100% 100% 100% 100%
src/operations/grants/grantRoles.ts 100% 100% 100% 100%
src/operations/grants/index.ts 100% 100% 100% 100%
src/operations/grants/revokeOnSchemas.ts 100% 100% 100% 100%
src/operations/grants/revokeOnTables.ts 100% 100% 100% 100%
src/operations/grants/revokeRoles.ts 100% 100% 100% 100%
src/operations/grants/shared.ts 95.65% 85.71% 100% 95.65% 71
src/operations/indexes/createIndex.ts 96.29% 95.23% 100% 96.29% 74-75
src/operations/indexes/dropIndex.ts 100% 100% 100% 100%
src/operations/indexes/index.ts 100% 100% 100% 100%
src/operations/indexes/shared.ts 88% 86.95% 100% 88% 22, 32-35, 47
src/operations/materializedViews/alterMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/createMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/dropMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/index.ts 100% 100% 100% 100%
src/operations/materializedViews/refreshMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/renameMaterializedView.ts 100% 100% 100% 100%
src/operations/materializedViews/renameMaterializedViewColumn.ts 100% 100% 100% 100%
src/operations/materializedViews/shared.ts 100% 87.5% 100% 100%
src/operations/operators/addToOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/createOperator.ts 100% 100% 100% 100%
src/operations/operators/createOperatorClass.ts 100% 83.33% 100% 100%
src/operations/operators/createOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/dropOperator.ts 100% 100% 100% 100%
src/operations/operators/dropOperatorClass.ts 100% 100% 100% 100%
src/operations/operators/dropOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/index.ts 100% 100% 100% 100%
src/operations/operators/removeFromOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/renameOperatorClass.ts 100% 100% 100% 100%
src/operations/operators/renameOperatorFamily.ts 100% 100% 100% 100%
src/operations/operators/shared.ts 85.71% 75% 100% 85.71% 24-25, 38
src/operations/policies/alterPolicy.ts 100% 100% 100% 100%
src/operations/policies/createPolicy.ts 100% 100% 100% 100%
src/operations/policies/dropPolicy.ts 100% 100% 100% 100%
src/operations/policies/index.ts 100% 100% 100% 100%
src/operations/policies/renamePolicy.ts 100% 100% 100% 100%
src/operations/policies/shared.ts 100% 100% 100% 100%
src/operations/roles/alterRole.ts 100% 100% 100% 100%
src/operations/roles/createRole.ts 100% 75% 100% 100%
src/operations/roles/dropRole.ts 100% 100% 100% 100%
src/operations/roles/index.ts 100% 100% 100% 100%
src/operations/roles/renameRole.ts 100% 100% 100% 100%
src/operations/roles/shared.ts 98.07% 76.92% 100% 98.07% 78
src/operations/schemas/createSchema.ts 100% 100% 100% 100%
src/operations/schemas/dropSchema.ts 100% 100% 100% 100%
src/operations/schemas/index.ts 100% 100% 100% 100%
src/operations/schemas/renameSchema.ts 100% 100% 100% 100%
src/operations/sequences/alterSequence.ts 94.11% 75% 100% 94.11% 23
src/operations/sequences/createSequence.ts 100% 100% 100% 100%
src/operations/sequences/dropSequence.ts 100% 100% 100% 100%
src/operations/sequences/index.ts 100% 100% 100% 100%
src/operations/sequences/renameSequence.ts 100% 100% 100% 100%
src/operations/sequences/shared.ts 78.57% 68.75% 100% 78.57% 41, 43-44, 49-50, 63-64, 69-70
src/operations/tables/addColumns.ts 100% 80% 100% 100%
src/operations/tables/addConstraint.ts 100% 100% 100% 100%
src/operations/tables/alterColumn.ts 88.57% 76.47% 100% 88.57% 75, 82-85, 87-89
src/operations/tables/alterTable.ts 100% 100% 100% 100%
src/operations/tables/createTable.ts 86.2% 66.66% 100% 86.2% 44-48, 65, 75-76
src/operations/tables/dropColumns.ts 100% 100% 100% 100%
src/operations/tables/dropConstraint.ts 100% 100% 100% 100%
src/operations/tables/dropTable.ts 100% 100% 100% 100%
src/operations/tables/index.ts 100% 100% 100% 100%
src/operations/tables/renameColumn.ts 100% 100% 100% 100%
src/operations/tables/renameConstraint.ts 100% 100% 100% 100%
src/operations/tables/renameTable.ts 100% 100% 100% 100%
src/operations/tables/shared.ts 82.31% 78.46% 80% 82.31% 137-138, 141-142, 195-199, 224, 228-229, 248-249, 274-275, 278-285, 288-289, 426-431, 433-436, 438-448, 450-451
src/operations/triggers/createTrigger.ts 86.41% 68.18% 100% 86.41% 53-54, 66-67, 70-71, 74-77, 113
src/operations/triggers/dropTrigger.ts 100% 100% 100% 100%
src/operations/triggers/index.ts 100% 100% 100% 100%
src/operations/triggers/renameTrigger.ts 100% 100% 100% 100%
src/operations/triggers/shared.ts 100% 100% 100% 100%
src/operations/types/addTypeAttribute.ts 100% 100% 100% 100%
src/operations/types/addTypeValue.ts 100% 100% 100% 100%
src/operations/types/createType.ts 100% 100% 100% 100%
src/operations/types/dropType.ts 100% 100% 100% 100%
src/operations/types/dropTypeAttribute.ts 100% 100% 100% 100%
src/operations/types/index.ts 100% 100% 100% 100%
src/operations/types/renameType.ts 100% 100% 100% 100%
src/operations/types/renameTypeAttribute.ts 100% 100% 100% 100%
src/operations/types/renameTypeValue.ts 100% 100% 100% 100%
src/operations/types/setTypeAttribute.ts 100% 100% 100% 100%
src/operations/views/alterView.ts 100% 100% 100% 100%
src/operations/views/alterViewColumn.ts 100% 100% 100% 100%
src/operations/views/createView.ts 100% 100% 100% 100%
src/operations/views/dropView.ts 100% 100% 100% 100%
src/operations/views/index.ts 100% 100% 100% 100%
src/operations/views/renameView.ts 100% 100% 100% 100%
src/operations/views/shared.ts 100% 66.66% 100% 100%
src/utils/PgLiteral.ts 88.88% 100% 75% 88.88% 37-38
src/utils/StringIdGenerator.ts 100% 100% 100% 100%
src/utils/createSchemalize.ts 100% 100% 100% 100%
src/utils/createTransformer.ts 100% 100% 100% 100%
src/utils/decamelize.ts 100% 100% 100% 100%
src/utils/escapeValue.ts 100% 100% 100% 100%
src/utils/formatLines.ts 100% 100% 100% 100%
src/utils/formatParams.ts 100% 100% 100% 100%
src/utils/getMigrationTableSchema.ts 100% 100% 100% 100%
src/utils/getSchemas.ts 100% 100% 100% 100%
src/utils/identity.ts 100% 100% 100% 100%
src/utils/index.ts 100% 100% 100% 100%
src/utils/intersection.ts 100% 100% 100% 100%
src/utils/makeComment.ts 100% 100% 100% 100%
src/utils/quote.ts 100% 100% 100% 100%
src/utils/toArray.ts 100% 100% 100% 100%
src/utils/types.ts 100% 100% 100% 100%
Generated in workflow #1038

@mythmon mythmon requested a review from Shinigami92 July 9, 2024 15:17
@mythmon
Copy link
Author

mythmon commented Jul 9, 2024

I've tried to use this branch in main project I work on (the one that has nearly 500 migrations), and I'd actually like to change my approach a bit. I'm curious what you think:

In this PR so far I've defined the name of the migration as the path to the migration. So a migration file like 2024/20240709191253834-add-new-field.sql (using the new year/utc format) would have a migration name of 2024/20240709191253834-add-new-field.

However, since my goal is to migrate an existing code base, this makes things very awkward. If we rename existing migrations, it is very easy to end up in a case where we try to run the migrations from scratch because the names are different now. I can get it to work by manually editing the database to update the migration names, but I'm not very comfortable doing that.

Instead, what do you think about the name of the migration being only the last part of the path? So the file 2024/20240709191253834-add-new-field.sql would define a migration with the name 20240709191253834-add-new-field. Any leading path prefixes would be ignored.

The main concern I have with this is that if there are migrations in different paths but with the same basename (like 1/1.sql and 2/1.sql) they would have the same calculated name. I don't think that has been possible until now. Maybe node-pg-migrate could detect that and give a helpful error?

@Shinigami92
Copy link
Collaborator

[...]

The main concern I have with this is that if there are migrations in different paths but with the same basename (like 1/1.sql and 2/1.sql) they would have the same calculated name. I don't think that has been possible until now. Maybe node-pg-migrate could detect that and give a helpful error?

IMO 1/1.sql and 2/1.sql should be a valid and possible solution and not result in an error.
The files that are generate by using the template are just a help for organizing files, but it is (I think) totally valid to use your own syntax.

e.g. 2024/v1.0_init.sql, 2025/v2.0_init.sql
but also
2024/000_something.sql, 2025/000_something.sql

@mythmon
Copy link
Author

mythmon commented Jul 10, 2024

Do you have any ideas about how to transition existing sets of migrations to use a new structure?

@Shinigami92
Copy link
Collaborator

Do you have any ideas about how to transition existing sets of migrations to use a new structure?

No, but IMO also not in the scope of node-pg-migrate.
Users should be able to organize their migration folder however they want, but node-pg-migrate will just read the folder (and with this PR now also sub-folders) in sequential order.

@mythmon
Copy link
Author

mythmon commented Jul 19, 2024

In that case I think this PR is ready to be reviewed. I'll have to figure out how to transition my project, but if that's out of scope, then that isn't a concern for this PR.

@Shinigami92
Copy link
Collaborator

In that case I think this PR is ready to be reviewed. I'll have to figure out how to transition my project, but if that's out of scope, then that isn't a concern for this PR.

ok, will have a look later, but right now I assume my weekend will be very full, even first half of next week is full.

maybe @osdiab could have a look in beforehand

@osdiab
Copy link
Collaborator

osdiab commented Jul 20, 2024

To make sure I understand before jumping into the review is the intention that

  • migration files can be in any subfolder or directly in the migrations dir
  • Sub folders don’t have an effect on the ordering, it’s still determined by the file name prefix
  • And a helper to auto sort migrations into the folders

Is this right?

@mythmon
Copy link
Author

mythmon commented Jul 20, 2024

Not quite. Points 1 and 3 are accurate, but not 2. Although I talked about having subfolders not affect order, that's not what I did originally. The code implemented here does take into account the sub folder names when determining order.

I'm on the fence about if sub folders should or should not affect order. Shinigami92 seems pretty confident that they should, and I'm happy to defer to that.

@osdiab
Copy link
Collaborator

osdiab commented Jul 20, 2024

Ok, I’m cool with that and will take a look soon!

@Shinigami92
Copy link
Collaborator

Shinigami92 commented Jul 20, 2024

Oh I see, I think I need to come up with some acceptance criteria, so we can align each other.

@osdiab while reviewing the code, please make familiar also with your own thoughts not only about that the code is ok, but also

  1. how was it for the customer in the past
  2. how should it be in the future
  3. do we need to implement one or more extra options to allow more control for the user
  4. what should be the default behavior
  5. is this a breaking change? can we achieve it without being a breaking change? do we need to split a smaller version with no breaking change and then have a re-iteration in another PR with the breaking changes

sadly I do not have much time right now, so I will come back (potentially also asking ChatGPT a bit) and define some acceptance criteria for this feature request

@osdiab
Copy link
Collaborator

osdiab commented Jul 20, 2024

to help things along:

  1. Users expected a flat migrations folder; I don’t actually know what it does today when there’s a sub folder in the migrations directory - I think it just ignores those files
  2. Idea is to
    1. allow organizing migrations with subfolders (a quality of life thing, not something I personally consider important but seems like there’s demand for it)
    2. Add a CLI option to output migrations in yearly subfolders (seems fairly workflow specific but doesn’t exclude other behaviors so I wouldn’t fight it)
    3. Change the execution behavior to take into account the (alphabetic?) ordering of subfolders when determining the overall ordering of migrations (I think this choice makes sense, personally I think it would be a little weird for subdirectories to not have an impact on the ordering)
  3. Maybe; it would be a breaking change for users who do put subfolders in the migrations directory eg for utilities. To avoid a breaking change would probably need to opt into sub folders
  4. Breaking change aside, I think this is very workflow specific and shouldn’t be a default. The simple file ordering of before is easy to understand and sub folders only become useful after you’ve reached a large volume of migrations, which is likely not the most common case
  5. I think it is in the absence of making it an opt-in behavior. If it’s opt-in (eg a migrationsSubdirs option) then I don’t think it would be breaking. Can auto enable it if the config specified one of the sub folders naming convention options, or by not adding such an option and tying the sub folder behaviors together with the naming convention option

@Shinigami92
Copy link
Collaborator

I really like to opt-in idea! Lets do this and by default it will be turned off, so it behaves just right now.
The option can already be documented that it will be turned on by default in next major release.
The feature will recursively traverse through all folders (also sub-sub folders and so on) in the given migration folder and look for the migration files (js, ts, sql).
The order of the migration files will also respect the folder names.
Example:

results in:

  1. migrations/3/a.sql
  2. migrations/123/test/b.sql
  3. migrations/123/c.sql
  4. migrations/2014/d.sql
  5. migrations/test/e.sql

so in other words, it will take the full path, and sort by (a, b) => a.localeCompare(b, 'en', { numeric: true })

It is potentially also open to define another options to pass a custom sort, but this would be its own PR I assume, because it is another feature request.

@mythmon What do you think? Are you fine with this?

@mythmon
Copy link
Author

mythmon commented Jul 20, 2024

That all sounds fine to me. It will probably be a few days until I can modify this PR to make those changes. Feel free to modify this branch as you see fit if you get to it before me, however.

@Shinigami92
Copy link
Collaborator

image

No need to rush, I have all the time to wait. And this is a free open source software. So just work on it when you find your time and will. Until then this is just marked as vAnytime and so can be shipped just when it is ready to be shipped.

@mythmon
Copy link
Author

mythmon commented Aug 5, 2024

In your example

  • migrations/3/a.sql
  • migrations/123/test/b.sql
  • migrations/123/c.sql
  • migrations/2014/d.sql
  • migrations/test/e.sql

shouldn't migrations/123/c.sql come before migrations/123/test/b.sql, since c comes before test, and it's comparing the paths, not the filenames?

@Shinigami92
Copy link
Collaborator

In your example

  • migrations/3/a.sql
  • migrations/123/test/b.sql
  • migrations/123/c.sql
  • migrations/2014/d.sql
  • migrations/test/e.sql

shouldn't migrations/123/c.sql come before migrations/123/test/b.sql, since c comes before test, and it's comparing the paths, not the filenames?

I thought about using it this way, because it is the structure you would also have in File Explorer or your IDE (e.g. VSCode)
Folders are coming first and then files

@mythmon
Copy link
Author

mythmon commented Aug 6, 2024

That would require something a bit more sophisticated than just String.localeCompare then. I'm actually not quite sure how to do that sorting.

I think that as a user I'd prefer directories to be sorted the same as files instead of always coming first though. I'm thinking of this use case:

  • migrations/001-init.js
  • migrations/002-feature/1-schema.js
  • migrations/002-feature/2-data.js

That would be hard to do with the sorting scheme out suggest.

@Shinigami92
Copy link
Collaborator

But that would enforce a specific naming again.
On the other side it all comes with some issues regarding that it is not really possible by design to insert files in the middle.
So if you would open a new folder, suddenly it comes before all files...

I need to think a bit about what the best sorting strategy might be 🤔

@mythmon
Copy link
Author

mythmon commented Aug 6, 2024

I don't think it enforces a specific naming pattern. It's like you said:

so in other words, it will take the full path, and sort by (a, b) => a.localeCompare(b, 'en', { numeric: true })

That would work with dates, numbers, or even just non-numeric. If you want all the directories to come first you could prefix them all with something alphabetically-low. If you want them to come last you could do that too. On the other hand, if directories are defined as always coming before peer files, then you can't override the directory ordering with a file name.

@Shinigami92
Copy link
Collaborator

I don't think it enforces a specific naming pattern. It's like you said:

so in other words, it will take the full path, and sort by (a, b) => a.localeCompare(b, 'en', { numeric: true })

That would work with dates, numbers, or even just non-numeric. If you want all the directories to come first you could prefix them all with something alphabetically-low. If you want them to come last you could do that too. On the other hand, if directories are defined as always coming before peer files, then you can't override the directory ordering with a file name.

I thought now a bit and I tend to agree... Neither way, the user could add files somewhere in between and so crash the migration order of an existing db.
But this would even be possible nowadays by inserting a file in between 🤷

So, maybe we should lay more focus on good documentation and suggestions.
And explain how it works and how (not) to break the system.

Maybe later on we could think about of repair/regenerate strategies. Or updating strategies to allow a patch in between. Like a "I know what I'm doing, and I just want to patch and now my DBs are all green with that patch"-mode.

So for now, I think you are one of the requesters / users that want this subdirectory feature, and I should kinda trust you what would potentially be best.
And also it is just an opt-in, so we don't break others folder structures. 👍

@benkroeger
Copy link
Contributor

have you considered using glob instead?
It can work with multiple patterns as well as ignore-patterns.
sorting the resulting iterable of paths via localeCompare should do the trick here.
I'd consider creating migrations with year-prefixed / directories a separate feature - so that this one can be moved forward (I am also in need of this... and glob would be ideal for my usecases).

I would make the current options vs glob config mutually exclusive to not break backwards-compatibility.

also willing to contribute a PR.

@Shinigami92
Copy link
Collaborator

have you considered using glob instead? It can work with multiple patterns as well as ignore-patterns. sorting the resulting iterable of paths via localeCompare should do the trick here. I'd consider creating migrations with year-prefixed / directories a separate feature - so that this one can be moved forward (I am also in need of this... and glob would be ideal for my usecases).

I would make the current options vs glob config mutually exclusive to not break backwards-compatibility.

also willing to contribute a PR.

you can try, give it a go

@mythmon
Copy link
Author

mythmon commented Sep 18, 2024

@Shinigami92 I was waiting for more feedback from you about this, since you seem to agree with my assessment above. Is there more you wanted me to do here? Sorry if I've missed something.

@Shinigami92
Copy link
Collaborator

@Shinigami92 I was waiting for more feedback from you about this, since you seem to agree with my assessment above. Is there more you wanted me to do here? Sorry if I've missed something.

My last feedback was in #1226 (comment)

But I also see that the Pipeline is red and the branch is not up-to-date

So right now it is your turn to move on 😺

@benkroeger
Copy link
Contributor

@mythmon I think this library shouldn't implement generating migration files in a particular folder structure in order to adhere to workflows of individual users. I'd rather recommend to execute the create command in a small wrapper that would provide proper values to the available cli or api (i.e. --migrations-dir and dir accordingly)

the use-case of subdirectories should also be covered by the glob functionality proposed in #1274 , do you agree?

@Shinigami92
Copy link
Collaborator

The glob pattern support landed in v7.7.0
Please evaluate if this PR needs a rebase or can even be closed as not needed anymore

@mythmon
Copy link
Author

mythmon commented Sep 25, 2024

I think the glob support from #1274 would handle most of what I wanted from this PR. Thanks for implementing that @benkroeger!

I think the main other problem that I'll have in the future is changing the file structure of migrations. Since the name of the migration would change if a file is moved, it means that we can't re-organize migrations after the fact without a complicated and manual process. Neither PR has a solution to that if I'm reading things right. This will be helpful if we ever set up a new migration system though.

@mythmon mythmon closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrations into subdirectories (readdir recursive directory search)
4 participants