Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new database schema for session management (settings + sessions table + view + trigger) and wires it into the Liquibase changelog. It also rewrites parts of the existing V1 schema/triggers scripts.
Changes:
- Add
session_settings,sessions,v_sessions, and an expiration trigger/function via a new Liquibase changeSet. - Modify the existing V1 schema script (
V1__init_tables.sql) including changingusers.passwordtype and removing several previously-defined tables. - Modify the existing V1 triggers script to drop the
resourcestrigger.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| liquibase/scripts/V1__triggers.sql | Removes the resources updated_at trigger from the V1 triggers script |
| liquibase/scripts/V1__session.sql | Adds new session-related tables, view, indexes, and a trigger/function |
| liquibase/scripts/V1__init_tables.sql | Rewrites the V1 init schema to only create users and changes password column type |
| liquibase/changelog.xml | Adds changeSet id=5 to run the new session SQL file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </changeSet> | ||
|
|
||
| <changeSet id="5" author="you"> | ||
| <sqlFile path="scripts/V1__session.sql" splitStatements="false" stripComments="true"/> |
There was a problem hiding this comment.
sqlFile is configured with splitStatements="false", but scripts/V1__session.sql contains many separate statements (CREATE TABLE, INSERT, CREATE VIEW, CREATE INDEX, CREATE FUNCTION, DROP/CREATE TRIGGER). With splitStatements disabled Liquibase may attempt to execute the whole file as a single statement, which commonly fails on PostgreSQL/JDBC. Consider splitting this into multiple changeSets/files (only the function body needs non-splitting) or enabling statement splitting with an appropriate delimiter configuration.
| <sqlFile path="scripts/V1__session.sql" splitStatements="false" stripComments="true"/> | |
| <sqlFile path="scripts/V1__session.sql" stripComments="true"/> |
| CREATE TABLE IF NOT EXISTS sessions ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id INT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| token_hash TEXT NOT NULL UNIQUE, |
There was a problem hiding this comment.
gen_random_uuid() requires the pgcrypto extension in PostgreSQL. There is no CREATE EXTENSION IF NOT EXISTS pgcrypto; in the Liquibase changelog/scripts, so this migration will fail on fresh databases where pgcrypto isn’t already enabled. Add an explicit extension creation step (in a dedicated changeSet) or switch to a UUID generation approach that matches the extensions you already enable.
|
|
||
| -- 4. Index (indispensables pour la rapidité) | ||
| CREATE INDEX IF NOT EXISTS idx_sessions_user_id ON sessions(user_id); | ||
| CREATE INDEX IF NOT EXISTS idx_sessions_token_lookup ON sessions(token_hash); |
There was a problem hiding this comment.
token_hash is declared UNIQUE, which already creates an index in PostgreSQL. CREATE INDEX idx_sessions_token_lookup ON sessions(token_hash); is therefore redundant and adds extra write overhead on inserts/updates. Consider dropping the explicit index and relying on the unique index (or drop the UNIQUE constraint if uniqueness is not required).
| CREATE INDEX IF NOT EXISTS idx_sessions_token_lookup ON sessions(token_hash); |
| CREATE TABLE users ( | ||
| id SERIAL PRIMARY KEY, | ||
| first_name VARCHAR(64) NOT NULL, | ||
| last_name VARCHAR(64) NOT NULL, | ||
| email VARCHAR(320) UNIQUE NOT NULL, | ||
| password CHAR(60) NOT NULL, | ||
| password VARCHAR(255) NOT NULL, |
There was a problem hiding this comment.
PR title indicates adding sessions, but this change also removes the creation of multiple unrelated tables (modules/roles/resources/permissions/user_roles, and the previous sessions table) from the initial V1 schema. If the goal is only to add/replace session handling, consider keeping unrelated schema unchanged and introducing sessions changes in a new migration; otherwise please align the PR title/description with the broader schema removal.
| CREATE TABLE IF NOT EXISTS session_settings ( | ||
| id BOOLEAN PRIMARY KEY DEFAULT TRUE, | ||
| session_duration INTERVAL, | ||
| CONSTRAINT one_row_only CHECK (id) | ||
| ); | ||
|
|
||
| INSERT INTO session_settings (session_duration) | ||
| VALUES ('7 days') | ||
| ON CONFLICT (id) DO NOTHING; | ||
|
|
||
| -- 2. Table des sessions (SANS la colonne générée) | ||
| CREATE TABLE IF NOT EXISTS sessions ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), |
There was a problem hiding this comment.
Other migration scripts in this repo use plain CREATE TABLE ... without IF NOT EXISTS (e.g., V1__init_tables.sql). Using IF NOT EXISTS here can mask partial/failed migrations or schema drift by silently succeeding when objects already exist. Consider removing IF NOT EXISTS in this changeSet and letting Liquibase manage idempotency via changeSet tracking.
| -- 3. La VUE pour ton Backend Rust | ||
| -- C'est cette "table virtuelle" que tu interrogeras dans ton code | ||
| CREATE OR REPLACE VIEW v_sessions AS | ||
| SELECT *, |
There was a problem hiding this comment.
v_sessions is defined as SELECT * from sessions. Using * in a view makes the view’s column set change implicitly when columns are added/removed on sessions, which can be a breaking change for consumers. Prefer listing the explicit columns from sessions that the backend should rely on, plus is_active.
| SELECT *, | |
| SELECT | |
| id, | |
| user_id, | |
| token_hash, | |
| device_info, | |
| ip_address, | |
| created_at, | |
| expires_at, | |
| revoked_at, |
| CREATE TABLE users ( | ||
| id SERIAL PRIMARY KEY, | ||
| first_name VARCHAR(64) NOT NULL, | ||
| last_name VARCHAR(64) NOT NULL, | ||
| email VARCHAR(320) UNIQUE NOT NULL, | ||
| password CHAR(60) NOT NULL, | ||
| password VARCHAR(255) NOT NULL, | ||
| phone_number VARCHAR(15), | ||
| photo bytea, | ||
| status VARCHAR(16) NOT NULL CHECK (status IN ('active', 'inactive', 'pending', 'offline')) DEFAULT 'offline', | ||
| created_at TIMESTAMPTZ DEFAULT now(), | ||
| updated_at TIMESTAMPTZ DEFAULT now() | ||
| ); |
There was a problem hiding this comment.
V1__init_tables.sql is referenced by changeSet id="2" in changelog.xml. Modifying an existing, already-applied changeSet script (changing column types and deleting multiple tables) will cause Liquibase checksum validation to fail on environments where changeSet 2 has already run, and it also changes the historical schema of V1. These changes should be done via a new changeSet (e.g., V2__... with ALTER/DROP statements) instead of editing the V1 script in-place.
| CREATE TRIGGER trg_users_updated_at | ||
| BEFORE UPDATE ON users | ||
| FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); |
There was a problem hiding this comment.
V1__triggers.sql is referenced by changeSet id="3". Editing this V1 migration in-place will change the stored checksum and can break Liquibase updates on databases where changeSet 3 has already been applied. Prefer adding a new changeSet to drop/create triggers instead of modifying the original V1 script.
No description provided.