[server] allow anyone to list active users#11
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds a non-nullable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant HTTP as HTTP Handler (/users/search)
participant Service as Users Service
participant Repo as Users Repository
participant DB as Database
Client->>HTTP: GET /users/search?query=...&limit=&offset=
HTTP->>Service: s.Search(ctx, q, pagination)
Service->>Repo: repo.Search(ctx, q, pagination)
Repo->>DB: SELECT ... WHERE lower(name) LIKE escaped_q ORDER BY name LIMIT/OFFSET
DB-->>Repo: rows + total
Repo-->>Service: []User, total
Service-->>HTTP: []User, total
HTTP-->>Client: 200 OK with paginated UserBriefList
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/db/migrations/20260424000000_add_user_name.sql (1)
16-16: Nit: stray---separator between Up and Down sections.
---is technically a valid SQL line comment but is non-standard in Goose migrations (the convention is a blank line). Consider removing it for consistency with the other migrations in this repo.🧹 Proposed tweak
-- +goose StatementEnd ---- + -- +goose Down🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/migrations/20260424000000_add_user_name.sql` at line 16, Remove the stray '---' separator line between the Up and Down sections in this migration: locate the literal '---' that sits between the Up block and the Down block and delete it so the migration uses a single blank line (consistent with other Goose migrations) while preserving the surrounding SQL statements in the Up and Down sections.internal/users/repository.go (1)
104-128: Consider indexing for the prefix search as the user table grows.
LOWER(name) LIKE 'prefix%'can use an index only if there is a functional/expression index onLOWER(name)(PostgreSQL) with the appropriate operator class (text_pattern_ops/varchar_pattern_ops) so the LIKE optimization kicks in. Without it, every search becomes a full scan of the active-user set. If you expect any meaningful user volume, add an index in a follow-up migration, e.g. for Postgres:CREATE INDEX users_name_lower_prefix_idx ON users (LOWER(name) varchar_pattern_ops) WHERE status = 'active';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/repository.go` around lines 104 - 128, The Search method in Repository uses "LOWER(name) LIKE '%s%%'" which will force full table scans unless the DB has a functional prefix index; add a follow-up DB migration that creates a conditional functional index on LOWER(name) with the appropriate operator class for prefix searches (and filter by status = 'active') so queries generated by Search (the LOWER(name) LIKE prefix) can use an index (e.g. create an index on LOWER(name) using varchar_pattern_ops/text_pattern_ops with WHERE status = 'active'). Ensure the migration targets your SQL dialect (Postgres) and documents the rationale for the Repository.Search query.internal/users/models.go (1)
62-75: Consider usingstrings.CutforemailUsername.The hand-rolled loop can be expressed more idiomatically and with identical fallback semantics:
♻️ Proposed refactor
-func emailUsername(email string) string { - // Best-effort parsing; email is expected to contain '@' due to validation elsewhere. - at := 0 - for i := range len(email) { - if email[i] == '@' { - at = i - break - } - } - if at <= 0 { - return email - } - return email[:at] +func emailUsername(email string) string { + // Best-effort parsing; email is expected to contain '@' due to validation elsewhere. + if local, _, ok := strings.Cut(email, "@"); ok && local != "" { + return local + } + return email }(Requires adding
"strings"to the import block.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/models.go` around lines 62 - 75, The emailUsername function uses a manual byte-loop to find '@'; replace it with strings.Cut to split the email on "@" and return the left part when cut succeeds, otherwise return the original email to preserve fallback semantics; add "strings" to the import block and update emailUsername to call strings.Cut(email, "@") and return the appropriate value based on the boolean result.internal/server/users/handler.go (1)
98-112: Nit:@Failure 403for/users/searchcan never fire.The route has no role gating (only JWT auth), so the handler has no code path that returns 403. The annotation is harmless but slightly misleading for API consumers. Consider dropping the
403line (same applies to the generated Swagger block indocs.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/users/handler.go` around lines 98 - 112, The OpenAPI annotation for the search handler includes a spurious "@Failure 403" entry that can never occur because the route only enforces JWT auth and the handler (handleSearch) has no role-based checks; remove the "@Failure 403" line from the comment block above handleSearch in users/handler.go and also remove the corresponding 403 failure entry in the generated Swagger block in docs.go so the docs accurately reflect possible responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/migrations/20260424000000_add_user_name.sql`:
- Around line 3-15: The migration currently adds the users.name column but no
index; update the migration to create an index on the name column (e.g.,
idx_users_name) in the Up section and drop that index in Down so prefix searches
can use the index, and ensure the column uses the existing case-insensitive
collation (utf8mb4_unicode_ci) or explicitly set it if needed; then update the
repository query at the function in internal/users/repository.go (around the
code that calls LOWER(name) LIKE ...) to remove the LOWER(...) wrapper and use
name LIKE ? (prefix%) so the index is usable.
---
Nitpick comments:
In `@internal/db/migrations/20260424000000_add_user_name.sql`:
- Line 16: Remove the stray '---' separator line between the Up and Down
sections in this migration: locate the literal '---' that sits between the Up
block and the Down block and delete it so the migration uses a single blank line
(consistent with other Goose migrations) while preserving the surrounding SQL
statements in the Up and Down sections.
In `@internal/server/users/handler.go`:
- Around line 98-112: The OpenAPI annotation for the search handler includes a
spurious "@Failure 403" entry that can never occur because the route only
enforces JWT auth and the handler (handleSearch) has no role-based checks;
remove the "@Failure 403" line from the comment block above handleSearch in
users/handler.go and also remove the corresponding 403 failure entry in the
generated Swagger block in docs.go so the docs accurately reflect possible
responses.
In `@internal/users/models.go`:
- Around line 62-75: The emailUsername function uses a manual byte-loop to find
'@'; replace it with strings.Cut to split the email on "@" and return the left
part when cut succeeds, otherwise return the original email to preserve fallback
semantics; add "strings" to the import block and update emailUsername to call
strings.Cut(email, "@") and return the appropriate value based on the boolean
result.
In `@internal/users/repository.go`:
- Around line 104-128: The Search method in Repository uses "LOWER(name) LIKE
'%s%%'" which will force full table scans unless the DB has a functional prefix
index; add a follow-up DB migration that creates a conditional functional index
on LOWER(name) with the appropriate operator class for prefix searches (and
filter by status = 'active') so queries generated by Search (the LOWER(name)
LIKE prefix) can use an index (e.g. create an index on LOWER(name) using
varchar_pattern_ops/text_pattern_ops with WHERE status = 'active'). Ensure the
migration targets your SQL dialect (Postgres) and documents the rationale for
the Repository.Search query.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1588b7f2-37f5-40c1-bf47-f5053dd3a496
📒 Files selected for processing (24)
go.modinternal/db/migrations/20260424000000_add_user_name.sqlinternal/example/config.gointernal/example/doc.gointernal/example/domain.gointernal/example/errors.gointernal/example/metrics.gointernal/example/models.gointernal/example/module.gointernal/example/repository.gointernal/example/service.gointernal/server/auth/dto.gointernal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/dto_comments.gointernal/server/users/dto.gointernal/server/users/handler.gointernal/users/domain.gointernal/users/models.gointernal/users/repository.gointernal/users/service.gorequests.http
💤 Files with no reviewable changes (9)
- internal/example/domain.go
- internal/example/config.go
- internal/example/service.go
- internal/example/errors.go
- internal/example/metrics.go
- internal/example/doc.go
- internal/example/repository.go
- internal/example/module.go
- internal/example/models.go
capcom6
force-pushed
the
server/allow-to-list-users
branch
from
054c145 to
8d721f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/server/docs/docs.go (1)
1120-1182:⚠️ Potential issue | 🟡 MinorGenerated spec is out of sync with the handler annotations.
This block was generated from a version of
handleSearchwhose annotations differ from what's currently ininternal/server/users/handler.go(parameter namenamehere vs.querythere;403response here vs. only401there). Regeneratedocs.gowithswag initafter the handler annotations are finalized to avoid shipping a misleading API spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/docs/docs.go` around lines 1120 - 1182, The generated OpenAPI block in docs.go is out of sync with the current handler annotations for handleSearch: update the handler annotations in handleSearch to the desired final state (confirm whether the query parameter is named "query" or "name" and whether 403 should be present), then regenerate the spec by running swag init to rebuild docs.go so the parameter name and responses (401/403) match the handler; ensure you re-run swag init after finalizing annotations for the handleSearch function to avoid mismatches.
🧹 Nitpick comments (1)
internal/users/repository.go (1)
105-113: Consider rejecting emptynameto avoid an unbounded match.If
name == "",patternbecomes"%"which matches every active user, effectively turningSearchinto an unrestricted listing of active users. The handler-side DTO marksqueryas required, but a defensive guard here keeps the repository contract safe regardless of how it is called:func (r *Repository) Search( ctx context.Context, name string, pagination *Pagination, ) ([]User, int, error) { + if name == "" { + return []User{}, 0, nil + } // Escape LIKE metacharacters in user input so they are matched literally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/repository.go` around lines 105 - 113, Repository.Search currently builds a LIKE pattern from name which becomes "%" for an empty string and matches all users; add a defensive guard at the top of Search (in function Repository.Search) that checks if strings.TrimSpace(name) == "" and returns an appropriate error (e.g., ErrInvalidArgument or a context-appropriate error) instead of proceeding to build pattern and query, ensuring callers receive a clear failure rather than an unbounded match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/users/handler.go`:
- Around line 88-101: The Swagger comment for the handleSearch handler has a
parameter name mismatch and an incorrect 403 failure entry; update the `@Param`
line to use the DTO's query name ("name") to match the SearchQuery binding
(query:"name") and remove any `@Failure` documentation for 403 Forbidden, then
re-run swag to regenerate docs.go so the generated spec matches the SearchQuery
DTO and the actual route behavior.
In `@internal/users/repository.go`:
- Around line 115-118: The WHERE clause is malformed: replace the incorrect
Where("LIKE ?", pattern) in the Search query built with r.db.NewSelect() (the
query variable that chains Model(&models), StatusActive and pattern) with a
proper column reference — e.g. Where("LOWER(name) LIKE ?", pattern) (or
Where("LOWER(name) LIKE ? ESCAPE '\\\\'", pattern) if you want explicit ESCAPE)
so the SQL becomes WHERE status = ? AND LOWER(name) LIKE ? and the query in the
Search method runs correctly.
---
Duplicate comments:
In `@internal/server/docs/docs.go`:
- Around line 1120-1182: The generated OpenAPI block in docs.go is out of sync
with the current handler annotations for handleSearch: update the handler
annotations in handleSearch to the desired final state (confirm whether the
query parameter is named "query" or "name" and whether 403 should be present),
then regenerate the spec by running swag init to rebuild docs.go so the
parameter name and responses (401/403) match the handler; ensure you re-run swag
init after finalizing annotations for the handleSearch function to avoid
mismatches.
---
Nitpick comments:
In `@internal/users/repository.go`:
- Around line 105-113: Repository.Search currently builds a LIKE pattern from
name which becomes "%" for an empty string and matches all users; add a
defensive guard at the top of Search (in function Repository.Search) that checks
if strings.TrimSpace(name) == "" and returns an appropriate error (e.g.,
ErrInvalidArgument or a context-appropriate error) instead of proceeding to
build pattern and query, ensuring callers receive a clear failure rather than an
unbounded match.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7fa1d7b-8cf9-40e3-9f5a-fb3e407e5595
📒 Files selected for processing (15)
go.modinternal/db/migrations/20260424000000_add_user_name.sqlinternal/server/auth/dto.gointernal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/dto_comments.gointernal/server/users/dto.gointernal/server/users/handler.gointernal/users/domain.gointernal/users/models.gointernal/users/repository.gointernal/users/service.gorequests.http
✅ Files skipped from review due to trivial changes (7)
- requests.http
- internal/users/domain.go
- internal/server/dto/response.go
- internal/server/tasks/dto_comments.go
- go.mod
- internal/server/tasks/dto_attachments.go
- internal/server/tasks/dto.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/server/auth/dto.go
- internal/users/service.go
- internal/users/models.go
- internal/db/migrations/20260424000000_add_user_name.sql
- internal/server/users/dto.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/users/service.go (1)
61-67: Consider trimming the query before the empty check.A whitespace‑only
q(e.g." ") bypasses this guard and reaches the repository, where it becomes aLIKE/prefix pattern that matches nothing — not a correctness bug, but it sends a pointless query and an empty page back to the caller. Trimming once here keeps the contract consistent withErrEmptyQueryand saves a DB round‑trip.♻️ Proposed refactor
func (s *Service) Search(ctx context.Context, q string, pagination *Pagination) ([]User, int, error) { - if q == "" { - return nil, 0, ErrEmptyQuery - } - - return s.repo.Search(ctx, q, pagination) + q = strings.TrimSpace(q) + if q == "" { + return nil, 0, ErrEmptyQuery + } + + return s.repo.Search(ctx, q, pagination) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/service.go` around lines 61 - 67, Service.Search currently checks q == "" but doesn't trim whitespace, so inputs like " " bypass the empty check and cause a pointless DB query; update Search to trim the query (use strings.TrimSpace) before the emptiness check and return ErrEmptyQuery for trimmed-empty input, then pass the trimmed value to s.repo.Search; ensure the strings package is imported and only trim once at the start of Service.Search.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/users/handler.go`:
- Around line 88-118: handleSearch currently returns toListResponse (which
reuses GetResponse exposing email/status/created_at/updated_at) allowing any
authenticated caller to see PII; fix by introducing a public search DTO (e.g.,
SearchResponse and SearchResponseItem with fields ID, Name, Role) and a
converter function toSearchResponse(items []users.User, total int)
SearchResponse that omits email and other sensitive fields, then update
handleSearch to return c.JSON(toSearchResponse(usersList, total)) and adjust the
Swagger `@Success` annotation to reference the new SearchResponse type instead of
ListResponse/GetResponse.
---
Nitpick comments:
In `@internal/users/service.go`:
- Around line 61-67: Service.Search currently checks q == "" but doesn't trim
whitespace, so inputs like " " bypass the empty check and cause a pointless DB
query; update Search to trim the query (use strings.TrimSpace) before the
emptiness check and return ErrEmptyQuery for trimmed-empty input, then pass the
trimmed value to s.repo.Search; ensure the strings package is imported and only
trim once at the start of Service.Search.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e4c8c84-6412-4bf7-83fc-8d98fdeecfb6
📒 Files selected for processing (6)
internal/server/docs/docs.gointernal/server/users/dto.gointernal/server/users/handler.gointernal/users/errors.gointernal/users/repository.gointernal/users/service.go
✅ Files skipped from review due to trivial changes (1)
- internal/users/errors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/users/repository.go
capcom6
force-pushed
the
server/allow-to-list-users
branch
from
89a590a to
1a8bfff
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/server/tasks/dto.go (1)
106-138:⚠️ Potential issue | 🟡 MinorTracking pre-existing issue: stubbed
UserBriefauthor/assignee.Outside this PR's scope, but worth flagging: the comment on line 107 promises that
newTaskResponse"requires fetching author and assignee from the users service," yetName,Role, andCreatedAtare still hard-coded to""(just like the previousEmail: ""). Withnamenow actually meaningful (populated for/users/search), the inconsistency is more visible — task responses give back essentially{id: N, name: "", role: "", created_at: ""}for author/assignee, which front-ends will likely render as blank.Consider opening a follow-up to populate these via the
usersservice (batch lookup by IDs) so consumers of/taskssee the same data shape as/users/search.Want me to open a follow-up issue tracking the author/assignee enrichment in
newTaskResponse?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 106 - 138, newTaskResponse currently returns stubbed dto.UserBrief for Author and Assignee (Name/Role/CreatedAt empty); fix by enriching these fields from the users service: perform a batch lookup for task.AuthorID and task.AssigneeID (use or add a users service method like UsersService.LookupByIDs or a users client that returns map[userID]User), then populate dto.UserBrief.Name, Role, CreatedAt from the fetched User data (or change newTaskResponse signature to accept a pre-fetched map of users to avoid blocking calls inside the function); ensure date fields are converted to RFC3339 for CreatedAt and handle missing users by leaving the existing ID-only stub.internal/server/docs/docs.go (1)
1043-1119:⚠️ Potential issue | 🟠 MajorRemove the
Userstag or restore admin-only enforcement to match the implementation.The route handler still enforces
jwtauth.WithRole(users.RoleAdmin)onGET /users(internal/server/users/handler.go:43), but the Swagger documentation tags it asUsersand describes it as "List all users"—implying general authenticated access. Either revert the docs to theAdmintag, or if the intent is to open this endpoint to all authenticated users, remove the role guard from the handler. Note:GET /users/searchis the unguarded endpoint for listing active users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/docs/docs.go` around lines 1043 - 1119, The OpenAPI docs for the GET /users endpoint are out of sync with the handler protection: the handler still uses jwtauth.WithRole(users.RoleAdmin) while docs tag/summary imply general access; either (A) update the docs entry in docs.go for the "/users" GET to use an "Admin" tag and a summary/description that clearly marks it as admin-only (e.g., "Admin: List all users") so it matches jwtauth.WithRole(users.RoleAdmin), or (B) remove the role guard jwtauth.WithRole(users.RoleAdmin) from the GET /users route handler so the endpoint becomes general-authenticated like the docs (ensure other auth middleware remains intact); pick one approach and make the corresponding change to the symbol docs.go "/users" GET entry or the route registration that applies jwtauth.WithRole(users.RoleAdmin).
🧹 Nitpick comments (3)
internal/server/users/dto.go (1)
53-66: Optional: simplifydefaultSearchQuery/SearchQuery.Setting
Query: ""indefaultSearchQuery()is redundant — it's the zero value, and therequired,min=1validator will reject an empty/missing value anyway. You could drop the explicitQuery: ""for clarity. Also consider embedding a shareddto.PaginationQuery(asTaskListQuerydoes) to keepLimit/Offsetdefinitions DRY across DTOs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/users/dto.go` around lines 53 - 66, Remove the redundant explicit Query: "" assignment in defaultSearchQuery and simplify SearchQuery by embedding the shared pagination DTO; replace Limit and Offset fields in SearchQuery with an embedded dto.PaginationQuery (like TaskListQuery does) and update defaultSearchQuery to return SearchQuery{} (or construct with the embedded pagination defaults) so Limit/Offset defaults come from the shared dto.PaginationQuery while leaving Query as the required string field validated by the tags.internal/server/dto/response.go (1)
20-39: LGTM, with one nit.Mapping logic is clean. Minor:
ToUserBrieftakes*users.UserwhileToUserBriefList'slo.Mapcallback receivesusers.Userby value and then takes&u. That copies each element before re-addressing it. If the underlyingusers.Useris large or hot-path, switching the callback tofunc(u *users.User, _ int)(when supported by the slice type — here you'd iterate[]users.Userwith index) avoids the copy. Not critical given typical page sizes ≤ 100.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/dto/response.go` around lines 20 - 39, The ToUserBriefList currently maps []users.User and uses lo.Map with a value receiver which causes a copy when taking &u; update ToUserBriefList to avoid copying by iterating by index and passing the address of each element to ToUserBrief (e.g., loop for i := range items and call ToUserBrief(&items[i])), or alternatively change the function signature to accept []*users.User and map pointers directly; ensure references to ToUserBrief and lo.Map/users.User are updated accordingly.internal/server/docs/docs.go (1)
1177-1194: Double-taggingPATCH /users/{id}with bothAdminandUsers.Adding
UsersalongsideAdmincauses this admin-only mutation to also appear under the public-lookingUserstag in Swagger UI. If the access control is unchanged (admin-only per the description), keeping justAdminis clearer; otherwise the dual tag suggests it's accessible to regular users. Recommend droppingUsershere unless authz was intentionally relaxed.Suggested fix (in source annotations, then regenerate)
In the handler's swag annotations for PATCH
/users/{id}:- // `@Tags` Admin Users + // `@Tags` AdminThen regenerate
internal/server/docs/docs.gowithswag init.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/docs/docs.go` around lines 1177 - 1194, The PATCH /users/{id} operation is incorrectly tagged with both "Admin" and "Users", which exposes an admin-only mutation under the public "Users" tag; update the handler's swag annotations for the PATCH /users/{id} endpoint to remove the "Users" tag so only "Admin" remains, then run swag init to regenerate internal/server/docs/docs.go (look for the PATCH /users/{id} annotations in the corresponding handler function to apply the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/server/docs/docs.go`:
- Around line 1043-1119: The OpenAPI docs for the GET /users endpoint are out of
sync with the handler protection: the handler still uses
jwtauth.WithRole(users.RoleAdmin) while docs tag/summary imply general access;
either (A) update the docs entry in docs.go for the "/users" GET to use an
"Admin" tag and a summary/description that clearly marks it as admin-only (e.g.,
"Admin: List all users") so it matches jwtauth.WithRole(users.RoleAdmin), or (B)
remove the role guard jwtauth.WithRole(users.RoleAdmin) from the GET /users
route handler so the endpoint becomes general-authenticated like the docs
(ensure other auth middleware remains intact); pick one approach and make the
corresponding change to the symbol docs.go "/users" GET entry or the route
registration that applies jwtauth.WithRole(users.RoleAdmin).
In `@internal/server/tasks/dto.go`:
- Around line 106-138: newTaskResponse currently returns stubbed dto.UserBrief
for Author and Assignee (Name/Role/CreatedAt empty); fix by enriching these
fields from the users service: perform a batch lookup for task.AuthorID and
task.AssigneeID (use or add a users service method like UsersService.LookupByIDs
or a users client that returns map[userID]User), then populate
dto.UserBrief.Name, Role, CreatedAt from the fetched User data (or change
newTaskResponse signature to accept a pre-fetched map of users to avoid blocking
calls inside the function); ensure date fields are converted to RFC3339 for
CreatedAt and handle missing users by leaving the existing ID-only stub.
---
Nitpick comments:
In `@internal/server/docs/docs.go`:
- Around line 1177-1194: The PATCH /users/{id} operation is incorrectly tagged
with both "Admin" and "Users", which exposes an admin-only mutation under the
public "Users" tag; update the handler's swag annotations for the PATCH
/users/{id} endpoint to remove the "Users" tag so only "Admin" remains, then run
swag init to regenerate internal/server/docs/docs.go (look for the PATCH
/users/{id} annotations in the corresponding handler function to apply the
change).
In `@internal/server/dto/response.go`:
- Around line 20-39: The ToUserBriefList currently maps []users.User and uses
lo.Map with a value receiver which causes a copy when taking &u; update
ToUserBriefList to avoid copying by iterating by index and passing the address
of each element to ToUserBrief (e.g., loop for i := range items and call
ToUserBrief(&items[i])), or alternatively change the function signature to
accept []*users.User and map pointers directly; ensure references to ToUserBrief
and lo.Map/users.User are updated accordingly.
In `@internal/server/users/dto.go`:
- Around line 53-66: Remove the redundant explicit Query: "" assignment in
defaultSearchQuery and simplify SearchQuery by embedding the shared pagination
DTO; replace Limit and Offset fields in SearchQuery with an embedded
dto.PaginationQuery (like TaskListQuery does) and update defaultSearchQuery to
return SearchQuery{} (or construct with the embedded pagination defaults) so
Limit/Offset defaults come from the shared dto.PaginationQuery while leaving
Query as the required string field validated by the tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76ababdb-6ea7-4eeb-9d44-07625143a8ba
📒 Files selected for processing (25)
go.modinternal/db/migrations/20260424000000_add_user_name.sqlinternal/example/config.gointernal/example/doc.gointernal/example/domain.gointernal/example/errors.gointernal/example/metrics.gointernal/example/models.gointernal/example/module.gointernal/example/repository.gointernal/example/service.gointernal/server/auth/dto.gointernal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/dto_comments.gointernal/server/users/dto.gointernal/server/users/handler.gointernal/users/domain.gointernal/users/errors.gointernal/users/models.gointernal/users/repository.gointernal/users/service.gorequests.http
💤 Files with no reviewable changes (9)
- internal/example/models.go
- internal/example/module.go
- internal/example/errors.go
- internal/example/repository.go
- internal/example/metrics.go
- internal/example/config.go
- internal/example/domain.go
- internal/example/service.go
- internal/example/doc.go
✅ Files skipped from review due to trivial changes (5)
- requests.http
- internal/users/errors.go
- internal/users/domain.go
- go.mod
- internal/db/migrations/20260424000000_add_user_name.sql
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/server/tasks/dto_comments.go
- internal/server/auth/dto.go
- internal/server/tasks/dto_attachments.go
- internal/users/service.go
- internal/users/repository.go
- internal/users/models.go
- internal/server/users/handler.go
Co-authored-by: Copilot <copilot@github.com>
capcom6
force-pushed
the
server/allow-to-list-users
branch
from
0075764 to
901b96d
Compare
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores