feat(jwt)!: rework mutation #286
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors JWT mutation functions in a database migration system, converting from PL/pgSQL to SQL implementations and renaming functions for consistency. The changes involve breaking API modifications including function signature changes, removal of error handling, and reordering of migration dependencies.
Changes:
- Converted
jwt_create,jwt_update_attendance_addfunctions from PL/pgSQL to SQL - Renamed
event_unlocktojwt_update_guest_addand simplified its implementation - Changed
table_jwtprimary key from a generated column to a DEFAULT value - Reordered sqitch.plan to move attendance-related migrations earlier in the sequence
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/deploy/function_jwt_create.sql | Converted from PL/pgSQL to SQL; removed guest authentication path and error handling |
| src/deploy/function_jwt_update_attendance_add.sql | Converted from PL/pgSQL to SQL with new JWT construction pattern |
| src/deploy/function_jwt_update_guest_add.sql | New function replacing event_unlock; removed validation logic and event information from return type |
| src/deploy/function_event_unlock.sql | Deleted as part of rename to jwt_update_guest_add |
| src/deploy/table_jwt.sql | Changed primary key from GENERATED ALWAYS to DEFAULT gen_random_uuid() |
| src/sqitch.plan | Reordered migrations to place attendance features before jwt_update_guest_add |
| src/revert/function_jwt_update_guest_add.sql | Added revert script for new function |
| src/revert/function_event_unlock.sql | Removed revert script for deleted function |
| src/verify/function_jwt_update_guest_add.sql | Updated verify script to check new function name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dargmuesli
force-pushed
the
feat/jwt/rework-mutation
branch
from
69e6c49 to
d729dc6
Compare
dargmuesli
force-pushed
the
feat/jwt/rework-mutation
branch
from
d729dc6 to
1423243
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SELECT COALESCE(current_setting('vibetype.jwt_expiry_duration', true), '1 day')::INTERVAL AS duration | ||
| ), | ||
| _existing_jwt_id AS ( | ||
| SELECT current_setting('jwt.claims.jti', true)::UUID AS id |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.jti', true) returns an empty string, which will cause an error when cast to UUID. The code should use NULLIF(current_setting('jwt.claims.jti', true), '')::UUID instead, following the pattern used in function_invoker_account_id.sql for similar JWT claim extraction.
| SELECT current_setting('jwt.claims.jti', true)::UUID AS id | |
| SELECT NULLIF(current_setting('jwt.claims.jti', true), '')::UUID AS id |
| ELSE ARRAY[jwt_update_guest_add.guest_id] | ||
| END AS guest_claims, | ||
| COALESCE( | ||
| current_setting('jwt.claims.exp', true)::BIGINT, |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.exp', true) returns an empty string, which will cause an error when cast to BIGINT. The code should use NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT to handle the case where no JWT exists.
| current_setting('jwt.claims.exp', true)::BIGINT, | |
| NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT, |
| ELSE NULL | ||
| END AS guest_claims, | ||
| COALESCE( | ||
| current_setting('jwt.claims.exp', true)::BIGINT, |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.exp', true) returns an empty string, which will cause an error when cast to BIGINT. The code should use NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT to handle the case where no JWT exists.
| current_setting('jwt.claims.exp', true)::BIGINT, | |
| NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT, |
| ELSE ARRAY[jwt_update_guest_add.guest_id] | ||
| END AS guest_claims, | ||
| COALESCE( | ||
| current_setting('jwt.claims.exp', true)::BIGINT, |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.exp', true) returns an empty string, which will cause an error when cast to BIGINT. The code should use NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT to handle the case where no JWT exists.
| current_setting('jwt.claims.exp', true)::BIGINT, | |
| NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT, |
| COALESCE(existing_jwt.id, gen_random_uuid()) AS token_id, | ||
| COALESCE(current_setting('jwt.claims.role', true)::TEXT, 'vibetype_anonymous') AS role, | ||
| vibetype.invoker_account_id() AS account_id, | ||
| current_setting('jwt.claims.username', true)::TEXT AS username |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.username', true) returns an empty string. Since other nullable JWT fields use NULLIF to convert empty strings to NULL, this field should likely use NULLIF(current_setting('jwt.claims.username', true), '')::TEXT for consistency.
| current_setting('jwt.claims.username', true)::TEXT AS username | |
| NULLIF(current_setting('jwt.claims.username', true), '')::TEXT AS username |
| SELECT COALESCE(current_setting('vibetype.jwt_expiry_duration', true), '1 day')::INTERVAL AS duration | ||
| ), | ||
| _existing_jwt_id AS ( | ||
| SELECT current_setting('jwt.claims.jti', true)::UUID AS id |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.jti', true) returns an empty string, which will cause an error when cast to UUID. The code should use NULLIF(current_setting('jwt.claims.jti', true), '')::UUID instead, following the pattern used in function_invoker_account_id.sql for similar JWT claim extraction.
| SELECT current_setting('jwt.claims.jti', true)::UUID AS id | |
| SELECT NULLIF(current_setting('jwt.claims.jti', true), '')::UUID AS id |
| ELSE NULL | ||
| END AS guest_claims, | ||
| COALESCE( | ||
| current_setting('jwt.claims.exp', true)::BIGINT, |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.exp', true) returns an empty string, which will cause an error when cast to BIGINT. The code should use NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT to handle the case where no JWT exists.
| current_setting('jwt.claims.exp', true)::BIGINT, | |
| NULLIF(current_setting('jwt.claims.exp', true), '')::BIGINT, |
| COALESCE(existing_jwt.id, gen_random_uuid()) AS token_id, | ||
| COALESCE(current_setting('jwt.claims.role', true)::TEXT, 'vibetype_anonymous') AS role, | ||
| vibetype.invoker_account_id() AS account_id, | ||
| current_setting('jwt.claims.username', true)::TEXT AS username |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.username', true) returns an empty string. Since other nullable JWT fields use NULLIF to convert empty strings to NULL, this field should likely use NULLIF(current_setting('jwt.claims.username', true), '')::TEXT for consistency.
| current_setting('jwt.claims.username', true)::TEXT AS username | |
| NULLIF(current_setting('jwt.claims.username', true), '')::TEXT AS username |
| COALESCE(existing_jwt.id, gen_random_uuid()) AS token_id, | ||
| COALESCE(current_setting('jwt.claims.role', true)::TEXT, 'vibetype_anonymous') AS role, | ||
| vibetype.invoker_account_id() AS account_id, | ||
| current_setting('jwt.claims.username', true)::TEXT AS username |
There was a problem hiding this comment.
When no JWT exists in the session, current_setting('jwt.claims.username', true) returns an empty string. Since other nullable JWT fields use NULLIF to convert empty strings to NULL, this field should likely use NULLIF(current_setting('jwt.claims.username', true), '')::TEXT for consistency.
| current_setting('jwt.claims.username', true)::TEXT AS username | |
| NULLIF(current_setting('jwt.claims.username', true), '')::TEXT AS username |
| gen_random_uuid() AS token_id, | ||
| ROW( | ||
| NULL, | ||
| EXTRACT(EPOCH FROM (_current_time.now + _expiry_duration.duration)), | ||
| NULL, | ||
| gen_random_uuid(), | ||
| 'vibetype_account', | ||
| authenticated_account.id, | ||
| authenticated_account.username | ||
| )::vibetype.jwt AS token | ||
| FROM _authenticated_account authenticated_account, _current_time, _expiry_duration |
There was a problem hiding this comment.
The jti field in the JWT token should match the token_id used for insertion, but here two different gen_random_uuid() calls will generate different UUIDs. The fourth field in the ROW constructor (line 52) should use token_id instead of generating a new UUID.
| gen_random_uuid() AS token_id, | |
| ROW( | |
| NULL, | |
| EXTRACT(EPOCH FROM (_current_time.now + _expiry_duration.duration)), | |
| NULL, | |
| gen_random_uuid(), | |
| 'vibetype_account', | |
| authenticated_account.id, | |
| authenticated_account.username | |
| )::vibetype.jwt AS token | |
| FROM _authenticated_account authenticated_account, _current_time, _expiry_duration | |
| token_data.token_id, | |
| ROW( | |
| NULL, | |
| EXTRACT(EPOCH FROM (_current_time.now + _expiry_duration.duration)), | |
| NULL, | |
| token_data.token_id, | |
| 'vibetype_account', | |
| authenticated_account.id, | |
| authenticated_account.username | |
| )::vibetype.jwt AS token | |
| FROM _authenticated_account authenticated_account, _current_time, _expiry_duration, | |
| LATERAL (SELECT gen_random_uuid() AS token_id) AS token_data |
1 participant
Breaking changes:
jwt_createjwt_update_attendance_addjwt_update_guest_add(before:event_unlock)jwt_createto prevent privacy leaksjwt_create, it's unnecessaryjwt_update_guest_addAlso:
jwt_update_guest_add