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

Convert all JSON columns to JSONB #311

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

candleindark
Copy link
Collaborator

@candleindark candleindark commented Feb 23, 2024

This PR modifies the database models so that all columns that are of the type of JSON will be converted to the type of JSONB.

The needed migration script for this change in the database is also included as part of this PR.

The migration has been applied to our live instances of Datalad-Registry.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.73%. Comparing base (f2540a1) to head (f92b348).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #311   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          50       50           
  Lines        2368     2368           
=======================================
  Hits         2338     2338           
  Misses         30       30           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yarikoptic
Copy link
Member

do I need to do anything special if I want to apply the migration or it would be done augomagically? I want to try on the local instance. might be worth adding to README.md since ATM seems nothing hits:

❯ git grep migration
datalad_registry/tests/test__init__.py:        # Disable `Migrate.init_app()` to skip the database migration integration
migrations/alembic.ini:# template used to generate migration files
migrations/env.py:def run_migrations_offline():
migrations/env.py:    """Run migrations in 'offline' mode.
migrations/env.py:        context.run_migrations()
migrations/env.py:def run_migrations_online():
migrations/env.py:    """Run migrations in 'online' mode.
migrations/env.py:    # this callback is used to prevent an auto-migration from being generated
migrations/env.py:            context.run_migrations()
migrations/env.py:    run_migrations_offline()
migrations/env.py:    run_migrations_online()

@candleindark
Copy link
Collaborator Author

candleindark commented Feb 23, 2024

do I need to do anything special if I want to apply the migration or it would be done augomagically? I want to try on the local instance. might be worth adding to README.md since ATM seems nothing hits:

❯ git grep migration
datalad_registry/tests/test__init__.py:        # Disable `Migrate.init_app()` to skip the database migration integration
migrations/alembic.ini:# template used to generate migration files
migrations/env.py:def run_migrations_offline():
migrations/env.py:    """Run migrations in 'offline' mode.
migrations/env.py:        context.run_migrations()
migrations/env.py:def run_migrations_online():
migrations/env.py:    """Run migrations in 'online' mode.
migrations/env.py:    # this callback is used to prevent an auto-migration from being generated
migrations/env.py:            context.run_migrations()
migrations/env.py:    run_migrations_offline()
migrations/env.py:    run_migrations_online()

No, the migration needs to be applied manually, and I will do that to the Typhon instance, and I expect the read-only instance will just update itself to the Typhon instance.

As for a local instance, you don't need migration to try out the new JSONB columns. You can just delete the bind-mounted directory for the DB service. It is by default in the ./services directory. Once that's done. You can spin up Datalad-Registry again, and you will get a clean database with the latest model specification.

If your interest is to try doing the migration, you will need to have both the shell variable environment and network access to access the database service in a running instance of Datalad-Registry. I usually gain this environment and access by podman/docker exec into the web service container. After that you can run flask db upgrade in the /app directory to migrate. However, in order for flask db upgrade to work, the existing database has to be marked to its current revision. If you have never done a migration to the existing database, most likely, it is not marked. Most likely, the existing database is at the revision before the current head. Use flask db history to find out what that revision is, then mark the database by flask db stamp <revision>.

As you can see it is a bit of a mouthful to describe the migration process. The process also depends on what the current state, revision, of the target database is in. I will try my best to describe it in the README.md though.

For more detailed information regarding migration, checkout https://flask-migrate.readthedocs.io/en/latest/.

@yarikoptic
Copy link
Member

indeed -- it sounds like something which should be automated -- upon start of the application, if a new non applied migration is present, should run migration before (re)starting application. I believe we have something like that for our django-based https://github.com/dandi/dandi-archive/ because I am not aware of the necessity for any manual dance to be done to trigger migrations.

@candleindark candleindark marked this pull request as ready for review February 26, 2024 06:47
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Let's just do it!

@candleindark candleindark merged commit d95f7aa into datalad:master Feb 26, 2024
6 checks passed
@candleindark candleindark deleted the JSON-to-JSONB branch February 26, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants