fix(meeting): increase Participant role column length to prevent admission failures#96
Conversation
…ssion failures - Update the Participant model's role column to String(20) to accommodate the 11-character "participant" role. - Add Alembic migration to update the column length limit in the database. - Fixes the "Failed to admit" error when hosts admit authenticated users from locked lobbies. Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
fix(meeting): increase Participant role column length to prevent admi…
📝 WalkthroughWalkthroughThe pull request expands the ChangesParticipant role column expansion
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/modules/meeting/models.py (1)
104-104: IncreaseParticipant.rolewidth to matchParticipantRolevalues; consider DB-level enforcement.
String(20)onapp/modules/meeting/models.pycovers allParticipantRoleenum values (host,co_host,participant,guest). Current persistence allows any string becauseparticipants.roleis a plainMapped[str]with only uniqueness constraints, whileapp/modules/meeting/ws_router.pyreadsrolefrom incoming state (defaulting to"guest"). Consider constrainingparticipants.roleto theParticipantRoleset via a SQLAlchemyEnumor a DBCHECKconstraint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/modules/meeting/models.py` at line 104, Participant.role currently uses mapped_column(String(20)) which allows arbitrary strings; update the model to enforce the ParticipantRole set at the DB level by replacing String(20) with a SQLAlchemy Enum tied to the ParticipantRole enum (e.g., mapped_column(Enum(ParticipantRole), nullable=False, default=ParticipantRole.guest)) or, if you must keep a string, increase the length and add a CheckConstraint limiting values to ParticipantRole members; adjust any defaults/usages that pass plain strings (ws_router reading/writing participants.role) to use ParticipantRole enum values or .value consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@alembic/versions/96532f61566e_increase_participant_role_length.py`:
- Around line 34-44: The downgrade() migration alters participants.role from
length 20 to 10 which will fail or silently truncate existing values >10; before
calling op.alter_column in downgrade(), scan the participants table for any role
strings longer than 10 (e.g., SELECT role WHERE char_length(role) > 10), and
either (a) abort the downgrade with a clear error/log message listing offending
values, or (b) perform a safe migration step to normalize/truncate or map those
roles to permitted 10-char values and write those updates back to the table,
then proceed to call op.alter_column("participants", "role", ...). Ensure this
logic references downgrade(), op.alter_column, and the "participants"."role"
column so the migration explicitly handles or prevents data loss.
---
Nitpick comments:
In `@app/modules/meeting/models.py`:
- Line 104: Participant.role currently uses mapped_column(String(20)) which
allows arbitrary strings; update the model to enforce the ParticipantRole set at
the DB level by replacing String(20) with a SQLAlchemy Enum tied to the
ParticipantRole enum (e.g., mapped_column(Enum(ParticipantRole), nullable=False,
default=ParticipantRole.guest)) or, if you must keep a string, increase the
length and add a CheckConstraint limiting values to ParticipantRole members;
adjust any defaults/usages that pass plain strings (ws_router reading/writing
participants.role) to use ParticipantRole enum values or .value consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef34df81-e17d-4738-8e62-daf1fbbd5d57
📒 Files selected for processing (2)
alembic/versions/96532f61566e_increase_participant_role_length.pyapp/modules/meeting/models.py
| def downgrade() -> None: | ||
| """Downgrade schema.""" | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.alter_column( | ||
| "participants", | ||
| "role", | ||
| existing_type=sa.String(length=20), | ||
| type_=sa.VARCHAR(length=10), | ||
| existing_nullable=False, | ||
| ) | ||
| # ### end Alembic commands ### |
There was a problem hiding this comment.
Downgrade may fail or truncate data if roles exceed 10 characters.
The downgrade path does not validate or handle existing role values that exceed 10 characters (e.g., "participant" = 11 characters). This could result in:
- PostgreSQL: Migration failure with data truncation error
- MySQL/MariaDB: Silent data truncation and corruption
Since the PR title mentions preventing "admission failures" and the context shows "participant" is a valid role value, existing production data likely contains 11-character role values.
🛡️ Recommended fix to ensure safe downgrade
Add data validation before the schema change:
def downgrade() -> None:
"""Downgrade schema."""
+ # Check for role values that would be truncated
+ connection = op.get_bind()
+ result = connection.execute(
+ sa.text("SELECT COUNT(*) FROM participants WHERE LENGTH(role) > 10")
+ )
+ count = result.scalar()
+ if count > 0:
+ raise ValueError(
+ f"Cannot downgrade: {count} participant(s) have role values exceeding 10 characters. "
+ "Please update or remove these records before downgrading."
+ )
+
# ### commands auto generated by Alembic - please adjust! ###
op.alter_column(Alternatively, add a comment documenting the risk:
def downgrade() -> None:
- """Downgrade schema."""
+ """Downgrade schema.
+
+ WARNING: This downgrade will fail if any participant records have role values
+ longer than 10 characters (e.g., 'participant'). Ensure all role values are
+ shortened to <= 10 characters before running this downgrade.
+ """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@alembic/versions/96532f61566e_increase_participant_role_length.py` around
lines 34 - 44, The downgrade() migration alters participants.role from length 20
to 10 which will fail or silently truncate existing values >10; before calling
op.alter_column in downgrade(), scan the participants table for any role strings
longer than 10 (e.g., SELECT role WHERE char_length(role) > 10), and either (a)
abort the downgrade with a clear error/log message listing offending values, or
(b) perform a safe migration step to normalize/truncate or map those roles to
permitted 10-char values and write those updates back to the table, then proceed
to call op.alter_column("participants", "role", ...). Ensure this logic
references downgrade(), op.alter_column, and the "participants"."role" column so
the migration explicitly handles or prevents data loss.
1 participant
Summary by CodeRabbit
Release Notes