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

refactor(backend): db session locking to support exclusive mode #1380

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Shadow243
Copy link
Member

Related task item89272

@Shadow243 Shadow243 requested a review from kroky November 23, 2024 03:55
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch from 518c456 to 02ca4ba Compare November 23, 2024 03:58
@Shadow243 Shadow243 changed the title Revamp db session storage to work in exclusive locking mode like nati… Fix: Revamp db session storage to work in exclusive locking mode like nati… Nov 23, 2024
@Shadow243 Shadow243 changed the title Fix: Revamp db session storage to work in exclusive locking mode like nati… fix: Revamp db session storage to work in exclusive locking mode like nati… Nov 23, 2024
@Shadow243 Shadow243 changed the title fix: Revamp db session storage to work in exclusive locking mode like nati… refactor(session): refactor db session locking to support exclusive mode Nov 23, 2024
@Shadow243 Shadow243 changed the title refactor(session): refactor db session locking to support exclusive mode refactor(other): refactor db session locking to support exclusive mode Nov 23, 2024
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch from 02ca4ba to 2657f72 Compare November 25, 2024 05:52
lib/session_db.php Outdated Show resolved Hide resolved
scripts/setup_database.php Outdated Show resolved Hide resolved
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch from 4ff8dd5 to 9a6025f Compare November 29, 2024 03:56
@Shadow243 Shadow243 changed the title refactor(other): refactor db session locking to support exclusive mode refactor(other): db session locking to support exclusive mode Nov 29, 2024
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch 15 times, most recently from 066c003 to fadddc5 Compare November 29, 2024 05:49
@Shadow243 Shadow243 changed the title refactor(other): db session locking to support exclusive mode refactor(backend): db session locking to support exclusive mode Dec 4, 2024
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch 2 times, most recently from 5702228 to 7a0b305 Compare December 4, 2024 03:18
@Shadow243
Copy link
Member Author

Shadow243 commented Dec 4, 2024

@kroky I tested with postgresql and mysql

I got a new error with postgres:

CREATE TABLE
psql:/home/runner/work/cypht/cypht/tests/phpunit/data/schema.sql:10: ERROR:  type "longblob" does not exist
LINE 1: ... EXISTS hm_user_session (hm_id varchar(255), data longblob,

@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch 13 times, most recently from 7b35f16 to 9da4405 Compare December 9, 2024 00:32
@Shadow243
Copy link
Member Author

Shadow243 commented Dec 9, 2024

@kroky The current function effectively generates a migration file with a timestamp-based name, ensuring the correct order of migrations. While this implementation works as expected, it could be improved by adding more flexibility. Once the PR: #1196 is approved, we can further enhance this functionality by integrating Symfony Console, making migration management more flexible and accessible directly from the command line.

Once the PR is validated, I will also migrate the tests to use migrations for table creation and remove all .sql files

@Shadow243 Shadow243 marked this pull request as ready for review December 9, 2024 00:46
@kroky
Copy link
Member

kroky commented Dec 9, 2024

@kroky The current function effectively generates a migration file with a timestamp-based name, ensuring the correct order of migrations. While this implementation works as expected, it could be improved by adding more flexibility. Once the PR: #1196 is approved, we can further enhance this functionality by integrating Symfony Console, making migration management more flexible and accessible directly from the command line.

Once the PR is validated, I will also migrate the tests to use migrations for table creation and remove all .sql files

This was is better but I'd like to change the approach. I see you copied large chunks of class contents from other frameworks. This is not the way to go because it makes maintenance harder and probably violates licenses. If you really wish to be fancy with php migrations, you can take a look here: https://packagist.org/?query=migrate for a suitable library and use one via composer. No need to maintain all that code.
Otherwise, my idea was much more simpler - do sql migrations - files stored in database/*.sql - run them one by one with timestamps and just execute whatever is there. No rollback. This should allow for the migration runner to be as simple as 1-2 methods with several lines of code - get all the migrations, check what has not been run against the current db and execute it via PDO.

@Shadow243 Shadow243 marked this pull request as draft December 9, 2024 07:54
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch from 9da4405 to 1ee9fd1 Compare December 9, 2024 09:36
@Shadow243
Copy link
Member Author

Shadow243 commented Dec 9, 2024

This was is better but I'd like to change the approach. I see you copied large chunks of class contents from other frameworks. This is not the way to go because it makes maintenance harder and probably violates licenses. If you really wish to be fancy with php migrations, you can take a look here: https://packagist.org/?query=migrate for a suitable library and use one via composer. No need to maintain all that code.
Otherwise, my idea was much more simpler - do sql migrations - files stored in database/*.sql - run them one by one with timestamps and just execute whatever is there. No rollback. This should allow for the migration runner to be as simple as 1-2 methods with several lines of code - get all the migrations, check what has not been run against the current db and execute it via PDO.

You raised valid points, and I agree that maintaining a custom migration system could become complex over time.

If we consider using .sql files for database migrations, we must address compatibility across different database engines. SQL syntax can vary between systems like MySQL, PostgreSQL, and SQLite. To manage this, we could organize migrations into subdirectories by engine, such as:

/database/migrations/mysql
/database/migrations/postgresql
/database/migrations/sqlite

The migration runner would detect the current database engine and execute only the files from the relevant directory. This keeps the migration process straightforward while supporting multiple database systems.

Alternatively, leveraging a well-maintained package from Packagist could be a more robust and scalable solution. Libraries such as bizley/migration offer extensive support for multiple database systems and advanced features like schema management. Another option is doctrine/migrations, which provides powerful migration capabilities but may introduce additional complexity.

Each approach has its strengths. Using .sql files keeps things simple and transparent but requires custom implementation and careful handling of database-specific syntax(ex: 20241209040300_add_lock_to_hm_user_session_table.sql). Adopting a mature package simplifies maintenance and enhances functionality but adds external dependencies and a learning curve.

@kroky What can you advise me ?

Here is a screenshot that explains the incompatibility between databases using .sql files:
Screenshot 2024-12-09 at 12 59 03

@Shadow243 Shadow243 marked this pull request as ready for review December 9, 2024 09:40
@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch from 1ee9fd1 to 2dfe62d Compare December 9, 2024 10:06
@kroky
Copy link
Member

kroky commented Dec 9, 2024

Yes, both options are valid. I think it is more in the spirit of cypt to go with the simpler approach and use per-database migration file. Cypht database is relatively simple, schema changes are not done often, so it doesn't make sense to me to include big dependencies as doctrine now.

@Shadow243
Copy link
Member Author

Yes, both options are valid. I think it is more in the spirit of cypt to go with the simpler approach and use per-database migration file. Cypht database is relatively simple, schema changes are not done often, so it doesn't make sense to me to include big dependencies as doctrine now.

Understood, I will create the SQL files for each DBMS and test then ping you.

@Shadow243 Shadow243 force-pushed the revamp-db-session-storage branch from 2dfe62d to cd0b746 Compare December 14, 2024 17:18
@Shadow243
Copy link
Member Author

@kroky I updated schema for all databases(sqlite,pgsql and mysql). you can review.

@marclaporte marclaporte requested a review from kroky December 15, 2024 00:55

DO $$
BEGIN
IF NOT EXISTS (SELECT 1 FROM information_schema.tables
Copy link
Member

Choose a reason for hiding this comment

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

Migration should not check for existence of tables or not - it should simply do what it is required to do - e.g. create the user table. I understand you want to check existing databases with hm_user tables that don't have anything in the migrations table (as it is freshly created). We should solve this issue differently. I think the most robust approach is this:

  1. Have a schema.sql file for all the database types. This is an up-to-date version of the db schema as of the current commit (in your case, that includes the lock columns).
  2. New installations use the schema which create the tables including migrations table AND populate migrations table with the list of all migrations. We don't want to run migrations on new installations - migrations tend to break as time passes, so we instead prefer to run a complete schema.sql file to create the db. However, we need to make sure the migrations won't be run again in the future, thus, we must populate the migrations table initially with all the existing migrations (without running them).
  3. Keep migrations simple - create table, alter table adding a column, etc. No checks for existing tables, no dropping and recreating with a renamed tmp name, etc.
  4. runMigrations is good - you check for migrations not run and run them. This should be executed every time cypht is updated - can be part of config_gen.
  5. Finally, add just one migration for now - containing the changes from this PR - the lock columns. When people deploy, they run config_gen, which runs the migrations and they automatically update their databases with the lock columns changes. We don't need migrations for hm_user, sessions creation, etc. This will be in the schema and new installations will use the schema.

How does this sound to you?

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