[LFXV2-976] Migrate to generic FGA sync message format #40
Conversation
Migrate from project-specific FGA sync subjects to the new generic, resource-agnostic FGA sync handlers. This change aligns with the standardized FGA sync client guide and enables consistent access control management across all LFX services. Changes: - Replace old subjects (lfx.update_access.project, lfx.delete_all_access.project) with new generic subjects (lfx.fga-sync.update_access, lfx.fga-sync.delete_access) - Introduce GenericFGAMessage envelope format with object_type, operation, and data fields - Add UpdateAccessData and DeleteAccessData for type-safe FGA operations - Transform Writers/Auditors/MeetingCoordinators arrays into relations map - Transform ParentUID into references map with proper formatting (project:<uid>) - Remove deprecated ProjectAccessMessage and ProjectAccessData types - Update all tests to use new message format - Update documentation in README.md and CLAUDE.md with message examples The new format uses a relations map for user roles and a references map for object relationships, providing a more flexible and generic approach to access control management. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <[email protected]>
WalkthroughIntroduces a GenericFGAMessage envelope plus UpdateAccessData/DeleteAccessData, replaces project-specific access payloads, updates NATS subjects/constants, adapts service flows to publish Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Projects Service
participant Converters as buildFGAUpdateAccessMessage
participant MsgBuilder as MessageBuilder
participant NATS as NATS
participant FGA as FGA Sync Consumer
Service->>Converters: Provide Project DB + Settings
Converters->>Service: Return GenericFGAMessage (ObjectType, Operation, Data{UID, Public, Relations, References})
Service->>MsgBuilder: SendAccessMessage(subject, GenericFGAMessage)
MsgBuilder->>NATS: Publish(subject: lfx.fga-sync.update_access / lfx.fga-sync.delete_access, payload)
NATS->>FGA: Deliver message
FGA->>FGA: Apply full-sync update or delete tuples
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/project_operations_test.go (1)
134-254: Add unit tests forUpdateProjectBase,UpdateProjectSettings, andDeleteProject.The exported functions
UpdateProjectBase,UpdateProjectSettings, andDeleteProject(lines 310, 506, and 671 ininternal/service/project_operations.go) are missing corresponding unit tests. All three useGenericFGAMessagewithSendAccessMessagecalls and must have unit test coverage per coding guidelines ("Each production function must have exactly one corresponding TestFunction test").
🧹 Nitpick comments (3)
internal/service/project_operations.go (1)
178-208: Consider extracting relation/reference building into a helper function.The logic to build
relationsandreferencesmaps is duplicated acrossCreateProject(lines 179-205),UpdateProjectBase(lines 462-488), andUpdateProjectSettings(lines 620-646). This pattern could be extracted into a helper function to reduce duplication and ensure consistent behavior.♻️ Suggested helper function
// buildFGAAccessData constructs UpdateAccessData from project base and settings. func buildFGAAccessData(projectDB *models.ProjectBase, projectSettingsDB *models.ProjectSettings) models.UpdateAccessData { relations := make(map[string][]string) if writers := extractUsernames(projectSettingsDB.Writers); len(writers) > 0 { relations["writer"] = writers } if auditors := extractUsernames(projectSettingsDB.Auditors); len(auditors) > 0 { relations["auditor"] = auditors } if coordinators := extractUsernames(projectSettingsDB.MeetingCoordinators); len(coordinators) > 0 { relations["meeting_coordinator"] = coordinators } references := make(map[string][]string) if projectDB.ParentUID != "" { references["parent"] = []string{fmt.Sprintf("project:%s", projectDB.ParentUID)} } return models.UpdateAccessData{ UID: projectDB.UID, Public: projectDB.Public, Relations: relations, References: references, } }internal/infrastructure/nats/message.go (1)
171-188: Consider adding sync mode support for consistency.
SendProjectEventMessageonly supports async publishing, whileSendIndexerMessageandSendAccessMessageboth support sync/async modes via async boolparameter. If synchronous delivery confirmation is ever needed for project events, this inconsistency could require a breaking change.If async-only is intentional for this use case, this is fine as-is. Otherwise, consider aligning with the other methods:
♻️ Optional: Add sync support for consistency
-func (m *MessageBuilder) SendProjectEventMessage(ctx context.Context, subject string, message any) error { +func (m *MessageBuilder) SendProjectEventMessage(ctx context.Context, subject string, message any, sync bool) error { messageBytes, err := json.Marshal(message) if err != nil { slog.ErrorContext(ctx, "error marshalling project event message into JSON", constants.ErrKey, err, "subject", subject) return err } - err = m.publishMessage(subject, messageBytes) - if err != nil { - slog.ErrorContext(ctx, "error publishing project event message to NATS", constants.ErrKey, err, "subject", subject) - return err - } - - slog.DebugContext(ctx, "published project event message to NATS", "subject", subject) - return nil + return m.sendMessage(ctx, subject, messageBytes, sync) }internal/domain/models/message_test.go (1)
190-249: Consider adding edge case tests.The current tests cover the happy path well. For more robust coverage, consider adding cases for edge scenarios:
- Empty
RelationsandReferencesmapsnilmaps vs empty maps (behavior difference withomitempty)- Empty
ExcludeRelationsslice♻️ Optional: Edge case test example
{ name: "update access data with minimal fields", data: UpdateAccessData{ UID: "project-minimal", Public: false, // Relations, References, ExcludeRelations intentionally nil }, verify: func(t *testing.T, data UpdateAccessData) { assert.Equal(t, "project-minimal", data.UID) assert.False(t, data.Public) assert.Nil(t, data.Relations) assert.Nil(t, data.References) assert.Nil(t, data.ExcludeRelations) }, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
CLAUDE.mdREADME.mdcmd/project-api/service_endpoint_project_test.gointernal/domain/models/message.gointernal/domain/models/message_test.gointernal/infrastructure/nats/message.gointernal/infrastructure/nats/message_test.gointernal/service/project_operations.gointernal/service/project_operations_test.gopkg/constants/nats.go
🧰 Additional context used
📓 Path-based instructions (5)
cmd/project-api/service*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cmd/project-api/service*.go: Implement Goa-generated service interfaces in cmd/project-api/service*.go
Enforce ETag concurrency via If-Match on PUT/DELETE handlers
Files:
cmd/project-api/service_endpoint_project_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Place unit tests alongside implementation with the same filename and *_test.go suffix
Each production function must have exactly one corresponding TestFunction test (table with multiple cases allowed)
Use table-driven tests for coverage (subtests over a single TestFunction)
Mock all external dependencies (e.g., repositories, message builders) in unit tests
Focus unit tests on exported package functions
Files:
cmd/project-api/service_endpoint_project_test.gointernal/infrastructure/nats/message_test.gointernal/service/project_operations_test.gointernal/domain/models/message_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure code formatting and linting pass (make fmt, make lint, make check)
Files:
cmd/project-api/service_endpoint_project_test.gointernal/service/project_operations.gointernal/infrastructure/nats/message_test.gointernal/infrastructure/nats/message.gointernal/domain/models/message.gopkg/constants/nats.gointernal/service/project_operations_test.gointernal/domain/models/message_test.go
CLAUDE.md
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain CLAUDE.md for AI assistant instructions and technical details
Files:
CLAUDE.md
README.md
📄 CodeRabbit inference engine (CLAUDE.md)
Keep README focused on quick start, API endpoints, and deployment setup
Files:
README.md
🧬 Code graph analysis (4)
internal/service/project_operations.go (4)
internal/domain/models/message.go (3)
GenericFGAMessage(43-47)UpdateAccessData(51-57)DeleteAccessData(61-63)internal/infrastructure/nats/message.go (1)
MessageBuilder(22-24)internal/domain/message.go (1)
MessageBuilder(23-27)pkg/constants/nats.go (2)
FGASyncUpdateAccessSubject(28-28)FGASyncDeleteAccessSubject(33-33)
internal/infrastructure/nats/message_test.go (3)
pkg/constants/nats.go (2)
FGASyncUpdateAccessSubject(28-28)FGASyncDeleteAccessSubject(33-33)internal/domain/models/message.go (3)
GenericFGAMessage(43-47)UpdateAccessData(51-57)DeleteAccessData(61-63)internal/infrastructure/nats/mock.go (1)
MockNATSConn(25-27)
internal/infrastructure/nats/message.go (3)
internal/domain/message.go (1)
MessageBuilder(23-27)internal/domain/models/message.go (1)
GenericFGAMessage(43-47)pkg/constants/app.go (1)
ErrKey(9-9)
internal/domain/models/message_test.go (1)
internal/domain/models/message.go (3)
GenericFGAMessage(43-47)UpdateAccessData(51-57)DeleteAccessData(61-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
- GitHub Check: MegaLinter
- GitHub Check: Build and Test
🔇 Additional comments (11)
README.md (1)
38-87: LGTM! Well-documented access control event format.The new documentation sections clearly explain the generic FGA sync message format with practical JSON examples. The examples correctly demonstrate the
GenericFGAMessageenvelope structure withupdate_accessanddelete_accessoperations, including therelationsandreferencesmaps.CLAUDE.md (1)
163-211: LGTM! Comprehensive technical documentation for AI assistants.The FGA Sync Message Format section provides excellent Go code examples that complement the JSON examples in README.md. The "Key Points" section clearly explains the full-sync semantics (relations not included will be removed), which is critical for downstream consumers to understand.
internal/service/project_operations.go (1)
742-751: LGTM! Clean implementation of delete_access message.The delete operation correctly uses
GenericFGAMessagewithDeleteAccessDatacontaining only the UID, matching the documented format and ensuring all access control tuples for the project are removed.internal/service/project_operations_test.go (1)
157-158: LGTM! Mock expectations correctly updated for new message type.The test mock expectation properly reflects the migration from
ProjectAccessMessagetoGenericFGAMessage, ensuring type compatibility with the new envelope format.internal/domain/models/message.go (1)
41-63: LGTM! Well-designed generic FGA message types.The type definitions are clean and flexible. The
GenericFGAMessageenvelope withanydata type allows the handler to support multiple operation types while the concreteUpdateAccessDataandDeleteAccessDatatypes provide structure for specific operations.The
ExcludeRelationsfield inUpdateAccessData(line 56) is already tested and actively used (seemessage_test.golines 208, 215), confirming it's part of the current implementation for handling excluded relations.Likely an incorrect or invalid review comment.
internal/infrastructure/nats/message.go (1)
154-169: Clean simplification of SendAccessMessage.The refactoring correctly consolidates access message handling to use only
GenericFGAMessage. The type switch is now simpler and the error handling is appropriate.cmd/project-api/service_endpoint_project_test.go (1)
157-157: LGTM!The mock expectation correctly updated to use
models.GenericFGAMessage, aligning with the refactoredSendAccessMessageimplementation.internal/infrastructure/nats/message_test.go (2)
241-303: Well-structured async access message tests.The test cases properly cover the key scenarios:
- Successful update and delete access messages with the new
GenericFGAMessageformat- Unsupported message type error handling
- NATS publish error propagation
The mock expectations correctly verify the new subjects and payload structure.
329-391: Sync mode tests align with async tests.The sync test cases mirror the async ones appropriately, using
Requestinstead ofPublishand verifying the same message structures. Good coverage of the error path with NATS request timeout.internal/domain/models/message_test.go (1)
122-188: LGTM! Comprehensive tests for GenericFGAMessage.The tests properly validate both
update_accessanddelete_accessoperations, including type assertions on the nestedDatafield. The coverage ofRelationsandReferencesmaps ensures the new structure is well-tested.pkg/constants/nats.go (1)
25-33: LGTM! Clean migration to generic FGA sync subjects.The new constants follow a consistent, resource-agnostic naming convention that aligns with the PR objectives. Documentation is clear and matches the existing style.
Verification confirms:
- No remaining references to the removed constants (UpdateAccessProjectSubject, UpdateAccessProjectSettingsSubject, DeleteAllAccessSubject, DeleteAllAccessProjectSettingsSubject) in the codebase
- Code passes Go vet analysis (0 issues)
- Constants are properly documented and defined
There was a problem hiding this comment.
Pull request overview
This pull request migrates the project service from project-specific FGA sync subjects to a new generic, resource-agnostic FGA sync message format. This standardization aligns with the FGA sync client guide used across all LFX services.
Changes:
- Introduced new generic NATS subjects (
lfx.fga-sync.update_access,lfx.fga-sync.delete_access) replacing old project-specific subjects - Implemented
GenericFGAMessageenvelope format withUpdateAccessDataandDeleteAccessDatapayloads for type-safe FGA operations - Transformed access control data from flat arrays (Writers, Auditors, MeetingCoordinators) into a flexible relations map structure
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/constants/nats.go |
Updated NATS subject constants to use generic FGA sync subjects |
internal/domain/models/message.go |
Added GenericFGAMessage, UpdateAccessData, and DeleteAccessData types; removed deprecated ProjectAccessMessage and ProjectAccessData |
internal/infrastructure/nats/message.go |
Updated SendAccessMessage to handle GenericFGAMessage instead of ProjectAccessMessage |
internal/service/project_operations.go |
Transformed access control data building in CreateProject, UpdateProjectBase, and UpdateProjectSettings to use new message format |
internal/infrastructure/nats/message_test.go |
Updated all access message tests to use GenericFGAMessage format |
internal/domain/models/message_test.go |
Added comprehensive tests for GenericFGAMessage, UpdateAccessData, and DeleteAccessData |
internal/service/project_operations_test.go |
Updated mock expectations to use GenericFGAMessage type |
cmd/project-api/service_endpoint_project_test.go |
Updated mock expectations to use GenericFGAMessage type |
README.md |
Added detailed documentation of new FGA sync message format with examples |
CLAUDE.md |
Added FGA sync message format section with Go code examples and key points |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Align struct field formatting in NATS message tests for consistency with Go formatting standards. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/infrastructure/nats/message_test.go (1)
231-327: Consider adding test cases forExcludeRelationsand edge cases.The current test suite covers the primary scenarios well, but per the coding guidelines recommending table-driven tests for coverage, consider adding cases for:
UpdateAccessDatawithExcludeRelationspopulated- Empty
RelationsandReferencesmaps (edge case)GenericFGAMessagewithnilData field (if that's a valid error case)This would ensure complete coverage of the
UpdateAccessDatastruct fields defined in the model.💡 Example additional test case
{ name: "successful send update access message with exclude relations", subject: constants.FGASyncUpdateAccessSubject, message: models.GenericFGAMessage{ ObjectType: "project", Operation: "update_access", Data: models.UpdateAccessData{ UID: "test-uid", Public: false, Relations: map[string][]string{ "writer": {"user1"}, }, ExcludeRelations: []string{"auditor", "meeting_coordinator"}, }, }, setupMocks: func(mockConn *MockNATSConn) { mockConn.On("Publish", constants.FGASyncUpdateAccessSubject, mock.AnythingOfType("[]uint8")).Return(nil) }, setupCtx: backgroundCtx, wantErr: false, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/infrastructure/nats/message_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Place unit tests alongside implementation with the same filename and *_test.go suffix
Each production function must have exactly one corresponding TestFunction test (table with multiple cases allowed)
Use table-driven tests for coverage (subtests over a single TestFunction)
Mock all external dependencies (e.g., repositories, message builders) in unit tests
Focus unit tests on exported package functions
Files:
internal/infrastructure/nats/message_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure code formatting and linting pass (make fmt, make lint, make check)
Files:
internal/infrastructure/nats/message_test.go
🧬 Code graph analysis (1)
internal/infrastructure/nats/message_test.go (3)
pkg/constants/nats.go (2)
FGASyncUpdateAccessSubject(28-28)FGASyncDeleteAccessSubject(33-33)internal/domain/models/message.go (3)
GenericFGAMessage(43-47)UpdateAccessData(51-57)DeleteAccessData(61-63)internal/infrastructure/nats/mock.go (1)
MockNATSConn(25-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
🔇 Additional comments (3)
internal/infrastructure/nats/message_test.go (3)
241-263: LGTM! Well-structured test case for the new message format.The test correctly validates the new
GenericFGAMessagestructure withUpdateAccessData, includingRelationsandReferencesmaps. The message format aligns with the model definitions.Consider adding a test case that includes
ExcludeRelationsto ensure complete coverage of theUpdateAccessDatastruct fields.
264-303: Test cases properly cover delete and error scenarios.The delete access message test correctly uses
DeleteAccessDatawith just theUIDfield. Error handling tests appropriately validate unsupported types and NATS publish failures.
338-391: Sync access message tests correctly implement request-response pattern.The tests properly validate the synchronous message flow using
Requestinstead ofPublish, with appropriate timeout handling. The test cases cover success paths for both update and delete operations, as well as timeout errors. ThedefaultRequestTimeoutconstant is properly defined in message.go and accessible to the test file.
…unction Extract the duplicated code for building FGA sync messages (relations and references maps) from CreateProject, UpdateProjectBase, and UpdateProjectSettings into a reusable helper function buildFGAUpdateAccessMessage in converters.go. This reduces ~87 lines of duplicated code to a single 32-line function, improving maintainability and reducing the risk of inconsistencies. Changes: - Add buildFGAUpdateAccessMessage helper function to converters.go - Replace three duplicated code blocks in project_operations.go with calls to helper - Remove unused fmt import from project_operations.go 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/service/converters.go (1)
441-472: Add nil checks for defensive coding.The function dereferences
projectDBandprojectSettingsDBwithout nil checks. While current callers inproject_operations.goensure non-nil inputs, adding defensive checks prevents potential panics from future misuse.♻️ Suggested defensive nil checks
func buildFGAUpdateAccessMessage(projectDB *models.ProjectBase, projectSettingsDB *models.ProjectSettings) models.GenericFGAMessage { + if projectDB == nil { + projectDB = &models.ProjectBase{} + } + if projectSettingsDB == nil { + projectSettingsDB = &models.ProjectSettings{} + } + // Build relations map for FGA sync relations := make(map[string][]string)internal/service/project_operations.go (1)
660-669: LGTM!The delete operation correctly constructs a
GenericFGAMessagewithDeleteAccessDataand uses the appropriateFGASyncDeleteAccessSubjectconstant. The inline construction is reasonable here since the delete message is simpler than update messages.Consider extracting the string literals
"project","update_access", and"delete_access"into constants (e.g., inpkg/constants) to avoid typos and maintain consistency across the codebase. This is optional and can be deferred.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/service/converters.gointernal/service/project_operations.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure code formatting and linting pass (make fmt, make lint, make check)
Files:
internal/service/project_operations.gointernal/service/converters.go
🧬 Code graph analysis (2)
internal/service/project_operations.go (4)
internal/infrastructure/nats/message.go (1)
MessageBuilder(22-24)internal/domain/message.go (1)
MessageBuilder(23-27)pkg/constants/nats.go (2)
FGASyncUpdateAccessSubject(28-28)FGASyncDeleteAccessSubject(33-33)internal/domain/models/message.go (2)
GenericFGAMessage(43-47)DeleteAccessData(61-63)
internal/service/converters.go (3)
api/project/v1/design/types.go (2)
ProjectBase(57-61)ProjectSettings(91-95)internal/domain/models/project.go (2)
ProjectBase(20-44)ProjectSettings(47-56)internal/domain/models/message.go (2)
GenericFGAMessage(43-47)UpdateAccessData(51-57)
🔇 Additional comments (4)
internal/service/converters.go (1)
7-7: LGTM!The
fmtimport is correctly added for use in thebuildFGAUpdateAccessMessagefunction.internal/service/project_operations.go (3)
177-180: LGTM!The FGA message construction using the helper function is clean and correctly uses the new
FGASyncUpdateAccessSubjectconstant. BothprojectDBandprojectSettingsDBare validated before this point.
433-436: LGTM!Consistent usage of the helper function with proper error handling ensuring non-nil inputs.
564-567: LGTM!Consistent usage of the helper function for settings updates with proper validation flow.
Summary
Migrates the project service from project-specific FGA sync subjects to the new generic, resource-agnostic FGA sync handlers. This change aligns with the standardized FGA sync client guide and enables consistent access control management across all LFX services.
Changes
lfx.update_access.project,lfx.delete_all_access.project) with new generic subjects (lfx.fga-sync.update_access,lfx.fga-sync.delete_access)GenericFGAMessageenvelope format withobject_type,operation, anddatafieldsUpdateAccessDataandDeleteAccessDatafor type-safe FGA operationsproject:<uid>)ProjectAccessMessageandProjectAccessDatatypes and constantsMessage Format Example
Update Access
{ "object_type": "project", "operation": "update_access", "data": { "uid": "project-uid", "public": true, "relations": { "writer": ["username1", "username2"], "auditor": ["username3"], "meeting_coordinator": ["username4"] }, "references": { "parent": ["project:parent-uid"] } } }Delete Access
{ "object_type": "project", "operation": "delete_access", "data": { "uid": "project-uid" } }Benefits
Testing
GenericFGAMessage,UpdateAccessData, andDeleteAccessDataTicket
LFXV2-976
🤖 Generated with Claude Code