-
Notifications
You must be signed in to change notification settings - Fork 677
SMQ-2761 - Add route field to channels #2772
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2772 +/- ##
==========================================
+ Coverage 27.68% 31.19% +3.50%
==========================================
Files 353 65 -288
Lines 55931 15355 -40576
==========================================
- Hits 15485 4790 -10695
+ Misses 39679 10312 -29367
+ Partials 767 253 -514 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
18184de to
65de6ed
Compare
7481f5f to
0b3a639
Compare
dborovcanin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixgateru Please rebase.
1900dbf to
0aedef8
Compare
299a2d3 to
4967efe
Compare
4967efe to
7d10692
Compare
7d10692 to
056e5c6
Compare
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
056e5c6 to
08a776b
Compare
| Up: []string{ | ||
| `ALTER TABLE channels ADD COLUMN route VARCHAR(36) NOT NULL;`, | ||
| `ALTER TABLE channels ADD CONSTRAINT unique_domain_route UNIQUE (domain_id, route);`, | ||
| }, | ||
| Down: []string{ | ||
| `ALTER TABLE channels DROP CONSTRAINT unique_domain_route;`, | ||
| `ALTER TABLE channels DROP COLUMN route;`, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have applied these migration in existing channels
I get error like this in channels
2025-05-21 14:25:56 {"time":"2025-05-21T08:55:56.963692336Z","level":"ERROR","msg":"failed to apply migrations : ERROR: column \"route\" of relation \"channels\" contains null values (SQLSTATE 23502) handling channels_03"}
In order make this work with existing deployments
We can simply copy channel id to route
-- Step 1: Add the 'route' column, allowing NULLs temporarily
ALTER TABLE channels ADD COLUMN route VARCHAR(36);
-- Step 2: Populate the 'route' column for existing rows with values from 'id'
-- Assuming 'id' is a UUID or a type that can be cast to VARCHAR(36)
-- If 'id' is a different type (e.g., INT), you might need a different casting or conversion
UPDATE channels SET route = id::VARCHAR(36);
-- Step 3: Add the NOT NULL constraint to the 'route' column
-- This will succeed now because all existing rows have a value for 'route'
ALTER TABLE channels ALTER COLUMN route SET NOT NULL;
-- Step 4: Add the unique constraint
ALTER TABLE channels ADD CONSTRAINT unique_domain_route UNIQUE (domain_id, route);Signed-off-by: Felix Gateru <[email protected]>
channels/postgres/init.go
Outdated
| `ALTER TABLE channels ADD COLUMN route VARCHAR(36);`, | ||
| `UPDATE channels SET route = id WHERE route IS NULL;`, | ||
| `ALTER TABLE channels ALTER COLUMN route SET NOT NULL;`, | ||
| `ALTER TABLE channels ADD CONSTRAINT unique_domain_route UNIQUE (domain_id, route);`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we make route required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing, routes must not be in UUID format so we can easily and quickly make a difference between ID and route; something like username and email.
Signed-off-by: Felix Gateru <[email protected]>
| } | ||
| if req.Channel.Route != "" { | ||
| return api.ValidateRoute(req.Channel.Route) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| if req.Channel.Route != "" { | |
| if err := api.ValidateRoute(req.Channel.Route); err != nil { | |
| return err | |
| } | |
| } |
Signed-off-by: Felix Gateru <[email protected]>
arvindh123
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| domainID: validID, | ||
| req: channels.Channel{ | ||
| Name: valid, | ||
| Route: "__invalid", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, add a check for UUID format. We must not allow UUIDs in routes to prevent confusion and make our lives easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test here as well:
{
desc: "create channel with UUID route",
token: validToken,
domainID: validID,
req: channels.Channel{
Name: valid,
Route: testsutil.GenerateUUID(t),
},
contentType: contentType,
status: http.StatusBadRequest,
err: apiutil.ErrInvalidRouteFormat,
},Validation testing is fine, but it only works if decoding request works properly, so we to test it here as well.
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
Signed-off-by: Felix Gateru <[email protected]>
What type of PR is this?
This is a feature as it adds a route field to channels.
What does this do?
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Yes, tests have been added for my changes.
Did you document any new/modified feature?
Yes, api documentation has been updated for the change
Notes