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

db: Make 'text' field in 'comments' table NOT NULL and handling data migration #1019

Merged

Conversation

pkvach
Copy link
Contributor

@pkvach pkvach commented Apr 27, 2024

Checklist

  • All new and existing tests are passing
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

This update introduces a schema migration to version 4 for the database, focusing on enhancing the 'comments' table. This ensures that the 'text' field in the 'comments' table will always have a value, which improves data consistency and integrity.

The key changes:

  • Incrementing the database schema version from 3 to 4.
  • Updating the "text" field in the "comments" table to be NOT NULL by setting empty strings for any NULL values.
  • Creating a new table, "comments_new", with a NOT NULL constraint on the "text" field and then migrating data from the old "comments" table to this new table.
  • Renaming the original "comments" table to "comments_backup_v3" for backup purposes and renaming "comments_new" to "comments" to finalize the migration.
  • The migration process includes transaction handling to ensure data integrity, with a rollback in case of any errors during the migration.

Why is this necessary?

@pkvach pkvach force-pushed the fix/db-schema-v4-comments-text-not-null branch from d18cec1 to 27bdc72 Compare April 27, 2024 16:05
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Nice effort, thank you.

I noticed that these lines should have been a migration as well:

isso/isso/db/comments.py

Lines 44 to 47 in 27bdc72

try:
self.db.execute(['ALTER TABLE comments ADD COLUMN notification INTEGER DEFAULT 0;'])
except Exception:
pass

Could you add them while you're at it?

isso/db/__init__.py Show resolved Hide resolved
isso/db/__init__.py Outdated Show resolved Hide resolved
isso/db/__init__.py Outdated Show resolved Hide resolved
isso/db/__init__.py Outdated Show resolved Hide resolved
isso/tests/test_db.py Show resolved Hide resolved
isso/tests/test_db.py Show resolved Hide resolved
@pkvach
Copy link
Contributor Author

pkvach commented May 2, 2024

Nice effort, thank you.

I noticed that these lines should have been a migration as well:

isso/isso/db/comments.py

Lines 44 to 47 in 27bdc72

try:
self.db.execute(['ALTER TABLE comments ADD COLUMN notification INTEGER DEFAULT 0;'])
except Exception:
pass

Could you add them while you're at it?

Thanks for the review.
I have added the suggested changes. I will do squash commits once the code review is complete.

Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

This is a massive improvement, thank you! A few suggestions inline, feel free to ignore if you feel they are too nitpicky.

isso/db/__init__.py Outdated Show resolved Hide resolved
isso/db/__init__.py Show resolved Hide resolved
isso/db/__init__.py Outdated Show resolved Hide resolved
isso/db/comments.py Outdated Show resolved Hide resolved
isso/tests/test_db.py Show resolved Hide resolved
@pkvach
Copy link
Contributor Author

pkvach commented May 4, 2024

@ix5
Thank you for your suggestions. I've made the changes.

…migration

This update introduces a schema migration to version 4 for
the database, focusing on enhancing the 'comments' table.
This ensures that the 'text' field in the 'comments' table
will always have a value, which improves data consistency
and integrity.

See:
- isso-comments#979
- isso-comments#994
@ix5 ix5 force-pushed the fix/db-schema-v4-comments-text-not-null branch from eac0b37 to c8602c0 Compare May 5, 2024 20:06
@ix5 ix5 added server (Python) server code testing/ci Test coverage & GitHub actions etc. improvement Not a new feature, but makes Isso more pleasant to use labels May 5, 2024
@ix5 ix5 added this to the 0.14 milestone May 5, 2024
@ix5 ix5 merged commit ed7a0d5 into isso-comments:master May 5, 2024
15 checks passed
@ix5
Copy link
Member

ix5 commented May 5, 2024

Squashed (pushed to your branch) and merged.

I have a few changes that I wrote and stashed locally when reviewing this, but that's for another day. This PR is a great improvement that allows for more confidence when releasing the next version.

@pkvach
Copy link
Contributor Author

pkvach commented May 6, 2024

Great, thank you.

@pkvach pkvach deleted the fix/db-schema-v4-comments-text-not-null branch May 6, 2024 05:08
@ix5 ix5 modified the milestones: 0.14, 0.13.1 May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Not a new feature, but makes Isso more pleasant to use server (Python) server code testing/ci Test coverage & GitHub actions etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants