chore: move GetRole and Get Permission to new architecture BED-8824#2945
chore: move GetRole and Get Permission to new architecture BED-8824#2945stephanieslamb wants to merge 6 commits into
Conversation
stephanieslamb
added
the
api
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds a new vertical-slice ChangesIdentity module migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
added
enhancement
![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 (9)
server/identity/internal/appdb/appdb.go (2)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTerse field name.
Store.dbdoesn't follow the descriptive-naming guideline (same asService.dbinservices.go).As per coding guidelines, "For user- or agent-written Go code, prefer descriptive variable names (for example,
databaseInterfaceinstead ofdiordbi)."🤖 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 `@server/identity/internal/appdb/appdb.go` around lines 60 - 66, The Store struct’s db field name is too terse and should be renamed to a more descriptive identifier to match the naming guideline. Update the Store definition in appdb.go and any related references in NewStore or methods on Store to use a clearer name such as databaseInterface so the code is easier to understand and consistent with Service in services.go.Source: Coding guidelines
93-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuideline-driven cleanups: hoist variable declarations and wrap errors with
%w.
GetRolehoists its first batch of vars (94-99) but then declarespermQuery/permRows/permErr/permissionRowsinline (117-129);GetPermissiondoesn't hoist at all. Per the repo's Go guidelines, these should be grouped into a singlevar(...)block at the top of each function.Also, the error wraps at lines 113, 124, 128, and 152 use
fmt.Errorf("...: %s", err)instead of%w, which discards the ability for callers toerrors.Is/errors.Asinto the original cause.Finally, the role lookup uses
sqlbuilder(101) but the permissions-join query is a hand-builtfmt.Sprintfstring (117-121) — consider usingsqlbuilderconsistently for both.As per coding guidelines, "group variable initializations in a
var ( ... )block and hoist them to the top of the function when possible."♻️ Example for `GetPermission`
func (s *Store) GetPermission(ctx context.Context, id int) (services.Permission, error) { - sb := sqlbuilder.PostgreSQL.NewSelectBuilder() + var ( + sb = sqlbuilder.PostgreSQL.NewSelectBuilder() + rows pgx.Rows + row permission + err error + ) + sb.Select("*") sb.From(tablePermissions) sb.Where(sb.Equal("id", id)) sb.Limit(1) query, args := sb.Build() - rows, err := s.db.Query(ctx, query, args...) - if err != nil { + if rows, err = s.db.Query(ctx, query, args...); err != nil { return services.Permission{}, err } - row, err := pgx.CollectOneRow(rows, pgx.RowToStructByName[permission]) - if errors.Is(err, pgx.ErrNoRows) { + if row, err = pgx.CollectOneRow(rows, pgx.RowToStructByName[permission]); errors.Is(err, pgx.ErrNoRows) { return services.Permission{}, services.ErrNoPermissionFound - } - if err != nil { - return services.Permission{}, fmt.Errorf("finding permission: %s", err) + } else if err != nil { + return services.Permission{}, fmt.Errorf("finding permission: %w", err) } return toPermission(row), nil }🤖 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 `@server/identity/internal/appdb/appdb.go` around lines 93 - 156, `GetRole` and `GetPermission` should follow the Go style guideline by hoisting all local declarations into a single top-level `var (...)` block within each function, including the permission-related variables in `GetRole`. Update the wrapped errors in `GetRole` and `GetPermission` to use `%w` so callers can preserve and inspect the original cause. Also consider replacing the hand-built permissions join query in `GetRole` with `sqlbuilder` to match the existing `roleSB`/`sb` query-building pattern.Source: Coding guidelines
server/identity/internal/services/services.go (2)
53-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent ID type and terse field naming.
Database.GetPermissiontakesid intwhileGetRoletakesid int32, even though bothPermission.IDandRole.IDareint32. Consider usingint32consistently for both methods to avoid type-confusion at call sites.Separately,
Service.dbis a terse field name; the guideline's own example (databaseInterfaceinstead ofdi/dbi) is already used for the constructor parameter on line 62 but not carried through to the field.As per coding guidelines, "For user- or agent-written Go code, prefer descriptive variable names (for example,
databaseInterfaceinstead ofdiordbi)."♻️ Suggested field rename
type Service struct { - db Database + database Database } func NewService(databaseInterface Database) *Service { - return &Service{db: databaseInterface} + return &Service{database: databaseInterface} } func (s *Service) GetRole(ctx context.Context, id int32) (Role, error) { - return s.db.GetRole(ctx, id) + return s.database.GetRole(ctx, id) } func (s *Service) GetPermission(ctx context.Context, id int) (Permission, error) { - return s.db.GetPermission(ctx, id) + return s.database.GetPermission(ctx, id) }🤖 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 `@server/identity/internal/services/services.go` around lines 53 - 64, Align the ID types in the Database interface by making GetPermission use the same int32 ID type as GetRole, since both Role.ID and Permission.ID are int32. Also rename the terse Service.db field to a more descriptive name that matches the coding guideline and the constructor’s databaseInterface parameter, and update NewService and any Service methods to use the new field name consistently.Source: Coding guidelines
28-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDomain model imports
database/sqland embeds it in JSON output.
Permission/Roleusesql.NullTimedirectly as a JSON field. This couples the domain/service layer todatabase/sql(working against the onion-architecture separation this PR is implementing) andsql.NullTimehas no customMarshalJSON, so by default it serializes as a nested{"Time":...,"Valid":...}object rather than a plain nullable timestamp. If this is intentional and already covered by the e2e/handler tests (which appear to assert on.Valid), this is low priority, but consider a domain-level nullable time type instead of leaking the database driver type into the service/domain layer.🤖 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 `@server/identity/internal/services/services.go` around lines 28 - 48, The domain models are leaking database/sql into the service layer by using sql.NullTime directly on Permission and Role, and that type also serializes poorly for JSON. Replace the DeletedAt fields in Permission and Role with a domain-level nullable timestamp type (or equivalent custom wrapper) and update any JSON handling/tests to match the new representation. Keep the fix localized around the Permission and Role structs in services.go so the model no longer depends on database/sql.server/identity/internal/appdb/appdb_test.go (1)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMocked columns mirror the internal struct, not the real table.
These column lists are derived from the
permission/rolestructs inappdb.go, which are missing adeleted_atfield. If the real tables include that column, these unit tests will keep passing while the integration test/production path fails. Worth a comment noting the columns intentionally mirror the structs (not the live schema) once the struct-field gap flagged inappdb.gois fixed.Also applies to: 140-146
🤖 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 `@server/identity/internal/appdb/appdb_test.go` around lines 50 - 52, Add a brief comment near the mocked column helpers like permissionRowColumns and the corresponding role columns in appdb_test.go clarifying that these lists intentionally mirror the permission and role structs in appdb.go rather than the live database schema. Make sure the note mentions the current struct-field alignment, so future changes to appdb.go or the test helpers do not assume the mocks validate the real table columns.server/identity/identity.go (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment references "analysis" instead of "identity".
This comment was copied from the analysis module's
Registerand not updated — it says "analysis store -> service -> handler chain" and "analysis routes" but this function wires up the identity module.📝 Suggested fix
-// Register builds the analysis store -> service -> handler chain and attaches -// the analysis routes to the provided router. It is called from the modules +// Register builds the identity store -> service -> handler chain and attaches +// the identity routes to the provided router. It is called from the modules // registry and receives only the infrastructure it directly needs.🤖 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 `@server/identity/identity.go` around lines 28 - 31, Update the doc comment on Register in identity.go so it accurately describes the identity module instead of the analysis module. Replace the copied wording about the “analysis store -> service -> handler chain” and “analysis routes” with identity-specific language that matches what Register actually wires up for router.Router and pgxpool.Pool.server/identity/internal/routes/routes.go (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameter name
handlersshadows the importedhandlerspackage.Naming the parameter
handlersshadows thehandlerspackage identifier for the rest of the function body, preventing any future reference to other exports from that package withinRegister. The tests already usehandlerSetfor this purpose — consider matching that convention here.♻️ Suggested rename
-func Register(routerInst *router.Router, handlers *handlers.Handlers) { +func Register(routerInst *router.Router, handlerSet *handlers.Handlers) { var permissions = auth.Permissions() - routerInst.GET(fmt.Sprintf("/api/v2/roles/{%s}", api.URIPathVariableRoleID), handlers.GetRole).RequirePermissions(permissions.AuthManageSelf) - routerInst.GET(fmt.Sprintf("/api/v2/permissions/{%s}", api.URIPathVariablePermissionID), handlers.GetPermission).RequirePermissions(permissions.AuthManageSelf) + routerInst.GET(fmt.Sprintf("/api/v2/roles/{%s}", api.URIPathVariableRoleID), handlerSet.GetRole).RequirePermissions(permissions.AuthManageSelf) + routerInst.GET(fmt.Sprintf("/api/v2/permissions/{%s}", api.URIPathVariablePermissionID), handlerSet.GetPermission).RequirePermissions(permissions.AuthManageSelf) }🤖 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 `@server/identity/internal/routes/routes.go` at line 29, The Register function parameter name currently shadows the imported handlers package, so rename the Register argument from handlers to handlerSet (or another non-conflicting name) and update all references inside Register accordingly; keep using the router.Router and handlers.Handlers symbols but avoid reusing the package identifier so future package exports remain accessible.server/identity/internal/handlers/handlers.go (1)
34-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent ID parameter types in
Identityinterface.
GetRoletakesid int32whileGetPermissiontakesid int. Given the views (PermissionView.ID,RoleView.ID) andGetRole's parameter are bothint32, this asymmetry looks accidental rather than intentional. It risks confusion for future implementers/consumers of this interface.♻️ Suggested fix: align both methods on the same ID type
type Identity interface { GetRole(ctx context.Context, id int32) (services.Role, error) - GetPermission(ctx context.Context, id int) (services.Permission, error) + GetPermission(ctx context.Context, id int32) (services.Permission, error) }This also requires updating
strconv.Atoitostrconv.ParseInt(rawPermissionID, 10, 32)inGetPermission, mirroringGetRole.🤖 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 `@server/identity/internal/handlers/handlers.go` around lines 34 - 37, The `Identity` interface has inconsistent ID types between `GetRole` and `GetPermission`, so align both methods on the same `int32`-based signature. Update the `GetPermission` method in `Identity` and its implementations to accept `int32`, and adjust the `GetPermission` handler in this package to parse the raw permission ID with a 32-bit parse path instead of `strconv.Atoi`, matching the `GetRole` flow and the `PermissionView.ID`/`RoleView.ID` types.server/identity/identity_e2e_test.go (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFilename doesn't follow the integration-test naming convention.
This file is named
identity_e2e_test.goand carries theintegrationbuild tag, but the repo's testing convention pairs the*integrationbuild tag with a*_integration_test.gofilename (e.g.auth_integration_test.go). Renaming toidentity_integration_test.gowould align with this convention and the surrounding codebase.As per coding guidelines, "Separate integration tests from unit tests into distinct files, such as
auth_test.gofor unit tests andauth_integration_test.gofor integration tests."Also applies to: 19-19
🤖 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 `@server/identity/identity_e2e_test.go` at line 17, Rename the integration test file to match the repo’s convention by changing the `identity_e2e_test.go` filename to `identity_integration_test.go`; the `//go:build integration` tag is already correct, so only the test filename needs to align with the existing pattern used by other integration test files.Source: Coding guidelines
🤖 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 `@server/identity/internal/appdb/appdb.go`:
- Around line 68-91: toRole currently leaves the Permissions field nil when
permissionRows is empty, which can serialize as null instead of an empty array
in responses. Update toRole in appdb.go to initialize the permissions slice as a
non-nil empty slice (for example, using the permissionRows length as capacity)
before appending in the loop, so services.Role.Permissions is always [] even
with no matches. While touching this mapping, also check whether DeletedAt
should be propagated alongside the existing fields in toPermission/toRole.
In `@server/identity/internal/handlers/views.go`:
- Around line 30-37: `PermissionView.DeletedAt` (and the matching
`RoleView.DeletedAt`) should not use `sql.NullTime` directly because it marshals
with the wrong JSON shape for the documented `deleted_at` contract. Introduce a
wire-compatible null-time view type with JSON fields matching the public API
(`time` and `valid`), then update `BuildPermissionView` and `BuildRoleView` to
populate it via a helper such as `newNullTimeView(...)` so marshaling preserves
the expected wire format.
---
Nitpick comments:
In `@server/identity/identity_e2e_test.go`:
- Line 17: Rename the integration test file to match the repo’s convention by
changing the `identity_e2e_test.go` filename to `identity_integration_test.go`;
the `//go:build integration` tag is already correct, so only the test filename
needs to align with the existing pattern used by other integration test files.
In `@server/identity/identity.go`:
- Around line 28-31: Update the doc comment on Register in identity.go so it
accurately describes the identity module instead of the analysis module. Replace
the copied wording about the “analysis store -> service -> handler chain” and
“analysis routes” with identity-specific language that matches what Register
actually wires up for router.Router and pgxpool.Pool.
In `@server/identity/internal/appdb/appdb_test.go`:
- Around line 50-52: Add a brief comment near the mocked column helpers like
permissionRowColumns and the corresponding role columns in appdb_test.go
clarifying that these lists intentionally mirror the permission and role structs
in appdb.go rather than the live database schema. Make sure the note mentions
the current struct-field alignment, so future changes to appdb.go or the test
helpers do not assume the mocks validate the real table columns.
In `@server/identity/internal/appdb/appdb.go`:
- Around line 60-66: The Store struct’s db field name is too terse and should be
renamed to a more descriptive identifier to match the naming guideline. Update
the Store definition in appdb.go and any related references in NewStore or
methods on Store to use a clearer name such as databaseInterface so the code is
easier to understand and consistent with Service in services.go.
- Around line 93-156: `GetRole` and `GetPermission` should follow the Go style
guideline by hoisting all local declarations into a single top-level `var (...)`
block within each function, including the permission-related variables in
`GetRole`. Update the wrapped errors in `GetRole` and `GetPermission` to use
`%w` so callers can preserve and inspect the original cause. Also consider
replacing the hand-built permissions join query in `GetRole` with `sqlbuilder`
to match the existing `roleSB`/`sb` query-building pattern.
In `@server/identity/internal/handlers/handlers.go`:
- Around line 34-37: The `Identity` interface has inconsistent ID types between
`GetRole` and `GetPermission`, so align both methods on the same `int32`-based
signature. Update the `GetPermission` method in `Identity` and its
implementations to accept `int32`, and adjust the `GetPermission` handler in
this package to parse the raw permission ID with a 32-bit parse path instead of
`strconv.Atoi`, matching the `GetRole` flow and the
`PermissionView.ID`/`RoleView.ID` types.
In `@server/identity/internal/routes/routes.go`:
- Line 29: The Register function parameter name currently shadows the imported
handlers package, so rename the Register argument from handlers to handlerSet
(or another non-conflicting name) and update all references inside Register
accordingly; keep using the router.Router and handlers.Handlers symbols but
avoid reusing the package identifier so future package exports remain
accessible.
In `@server/identity/internal/services/services.go`:
- Around line 53-64: Align the ID types in the Database interface by making
GetPermission use the same int32 ID type as GetRole, since both Role.ID and
Permission.ID are int32. Also rename the terse Service.db field to a more
descriptive name that matches the coding guideline and the constructor’s
databaseInterface parameter, and update NewService and any Service methods to
use the new field name consistently.
- Around line 28-48: The domain models are leaking database/sql into the service
layer by using sql.NullTime directly on Permission and Role, and that type also
serializes poorly for JSON. Replace the DeletedAt fields in Permission and Role
with a domain-level nullable timestamp type (or equivalent custom wrapper) and
update any JSON handling/tests to match the new representation. Keep the fix
localized around the Permission and Role structs in services.go so the model no
longer depends on database/sql.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f9cd0043-42eb-48da-aba2-d0a26dfceffe
📒 Files selected for processing (20)
AGENTS.mdcmd/api/src/api/registration/v2.gocmd/api/src/api/v2/auth/auth.gocmd/api/src/api/v2/auth/auth_test.goserver/identity/identity.goserver/identity/identity_e2e_test.goserver/identity/identity_test.goserver/identity/internal/appdb/appdb.goserver/identity/internal/appdb/appdb_integration_test.goserver/identity/internal/appdb/appdb_test.goserver/identity/internal/handlers/handlers.goserver/identity/internal/handlers/handlers_test.goserver/identity/internal/handlers/mocks/identity.goserver/identity/internal/handlers/views.goserver/identity/internal/routes/routes.goserver/identity/internal/routes/routes_test.goserver/identity/internal/services/mocks/database.goserver/identity/internal/services/services.goserver/identity/internal/services/services_test.goserver/modules/modules.go
💤 Files with no reviewable changes (3)
- cmd/api/src/api/registration/v2.go
- cmd/api/src/api/v2/auth/auth_test.go
- cmd/api/src/api/v2/auth/auth.go
| func toPermission(row permission) services.Permission { | ||
| return services.Permission{ | ||
| Authority: row.Authority, | ||
| Name: row.Name, | ||
| ID: row.ID, | ||
| CreatedAt: row.CreatedAt, | ||
| UpdatedAt: row.UpdatedAt, | ||
| } | ||
| } | ||
|
|
||
| func toRole(row role, permissionRows []permission) services.Role { | ||
| var permissions []services.Permission | ||
| for _, permissionRow := range permissionRows { | ||
| permissions = append(permissions, toPermission(permissionRow)) | ||
| } | ||
| return services.Role{ | ||
| ID: row.ID, | ||
| Name: row.Name, | ||
| Description: row.Description, | ||
| Permissions: permissions, | ||
| CreatedAt: row.CreatedAt, | ||
| UpdatedAt: row.UpdatedAt, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
toRole returns a nil permissions slice when there are no matches.
var permissions []services.Permission (79) stays nil if permissionRows is empty, which serializes as JSON null instead of []. Consider initializing with make([]services.Permission, 0, len(permissionRows)) for a consistent, non-null array in API responses. This is secondary to the missing DeletedAt mapping noted above (also unhandled here).
🤖 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 `@server/identity/internal/appdb/appdb.go` around lines 68 - 91, toRole
currently leaves the Permissions field nil when permissionRows is empty, which
can serialize as null instead of an empty array in responses. Update toRole in
appdb.go to initialize the permissions slice as a non-nil empty slice (for
example, using the permissionRows length as capacity) before appending in the
loop, so services.Role.Permissions is always [] even with no matches. While
touching this mapping, also check whether DeletedAt should be propagated
alongside the existing fields in toPermission/toRole.
| type PermissionView struct { | ||
| Authority string `json:"authority"` | ||
| Name string `json:"name"` | ||
| ID int32 `json:"id"` | ||
| CreatedAt time.Time `json:"created_at"` | ||
| UpdatedAt time.Time `json:"updated_at"` | ||
| DeletedAt sql.NullTime `json:"deleted_at"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
sql.NullTime embedded directly breaks the documented deleted_at wire format.
sql.NullTime does not implement json.Marshaler; legacy sql.Null* types encode via plain reflection as a nested struct, so json.Marshal on PermissionView/RoleView will emit "deleted_at":{"Time":"...","Valid":true} (capitalized field names, no json tags on Time/Valid). The publicly documented BloodHound API contract for this exact field is "deleted_at": {"time": "...", "valid": true} (lowercase keys) — e.g. the public Get Permission reference shows "deleted_at": { "time": "2023-11-07T05:31:56Z", "valid": true }. Since this PR is meant to preserve existing behavior while migrating to the new architecture, this is a wire-contract break for any client parsing deleted_at.time/deleted_at.valid.
🐛 Suggested fix: introduce a wire-compatible null-time view type
type PermissionView struct {
Authority string `json:"authority"`
Name string `json:"name"`
ID int32 `json:"id"`
CreatedAt time.Time `json:"created_at"`
UpdatedAt time.Time `json:"updated_at"`
- DeletedAt sql.NullTime `json:"deleted_at"`
+ DeletedAt NullTime `json:"deleted_at"`
}
+
+// NullTime mirrors the documented BloodHound API wire format for nullable
+// timestamps: {"time": ..., "valid": ...}.
+type NullTime struct {
+ Time time.Time `json:"time"`
+ Valid bool `json:"valid"`
+}
+
+func newNullTimeView(t sql.NullTime) NullTime {
+ return NullTime{Time: t.Time, Valid: t.Valid}
+}Apply the same change to RoleView.DeletedAt and update BuildPermissionView/BuildRoleView to use newNullTimeView(...).
Also applies to: 61-69
🤖 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 `@server/identity/internal/handlers/views.go` around lines 30 - 37,
`PermissionView.DeletedAt` (and the matching `RoleView.DeletedAt`) should not
use `sql.NullTime` directly because it marshals with the wrong JSON shape for
the documented `deleted_at` contract. Introduce a wire-compatible null-time view
type with JSON fields matching the public API (`time` and `valid`), then update
`BuildPermissionView` and `BuildRoleView` to populate it via a helper such as
`newNullTimeView(...)` so marshaling preserves the expected wire format.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/api/src/api/registration/v2.go`:
- Around line 65-68: The PR includes unrelated authorization changes in route
registration that are outside the identity-migration work, so remove or isolate
them from this change. In registration/v2.go, review the SAML/SSO route setup
and any other affected router entries to restore the existing permission
requirements unless they are intentionally part of this PR; keep this PR focused
on the GetRole/GetPermission migration and move permission-model changes to a
separate reviewable commit. Use the route definitions around routerInst.GET,
routerInst.POST, and any other changed handlers in the diff to locate the
bundled permission updates.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1d445b1b-8aed-4eea-8db8-36cac1c6bfd9
📒 Files selected for processing (2)
cmd/api/src/api/registration/v2.goserver/modules/modules.go
💤 Files with no reviewable changes (1)
- server/modules/modules.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@cmd/api/src/api/registration/v2.go`:
- Around line 65-68: The PR includes unrelated authorization changes in route
registration that are outside the identity-migration work, so remove or isolate
them from this change. In registration/v2.go, review the SAML/SSO route setup
and any other affected router entries to restore the existing permission
requirements unless they are intentionally part of this PR; keep this PR focused
on the GetRole/GetPermission migration and move permission-model changes to a
separate reviewable commit. Use the route definitions around routerInst.GET,
routerInst.POST, and any other changed handlers in the diff to locate the
bundled permission updates.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1d445b1b-8aed-4eea-8db8-36cac1c6bfd9
📒 Files selected for processing (2)
cmd/api/src/api/registration/v2.goserver/modules/modules.go
💤 Files with no reviewable changes (1)
- server/modules/modules.go
🛑 Comments failed to post (1)
cmd/api/src/api/registration/v2.go (1)
65-68: 🎯 Functional Correctness | 🟠 Major
Unrelated permission changes bundled into an identity-migration "chore" PR.
These lines change authorization requirements for SAML/SSO endpoints (
AuthManageProviders→AuthReadProviders) and split file-upload permissions (GraphDBIngestManage/GraphDBIngestRead) — none of which relate to movingGetRole/GetPermissionto the new architecture. The AI summary and PR objectives describe this PR only as removing the legacyManagementResourceGetPermission/GetRoleendpoints; there's no mention of these permission-model changes, and the PR is explicitly framed as not modifying application functionality.Please confirm this is intentional, split it into a dedicated PR/commit for independent review and testing, or clarify why it's included here.
Also applies to: 77-77, 127-130
🤖 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 `@cmd/api/src/api/registration/v2.go` around lines 65 - 68, The PR includes unrelated authorization changes in route registration that are outside the identity-migration work, so remove or isolate them from this change. In registration/v2.go, review the SAML/SSO route setup and any other affected router entries to restore the existing permission requirements unless they are intentionally part of this PR; keep this PR focused on the GetRole/GetPermission migration and move permission-model changes to a separate reviewable commit. Use the route definitions around routerInst.GET, routerInst.POST, and any other changed handlers in the diff to locate the bundled permission updates.
1 participant
Description
This PR moves two existing handlers to the new onion architecture.
Motivation and Context
Resolves BED-8824
Why is this change required? What problem does it solve?
How Has This Been Tested?
E2E test was created before moving the handlers and swapped to using new handlers in the new architecture and tests pass. Unit and integration tests also added for each layer.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
400, missing records return404, and request timeouts return500.Chores