Support schema sync #95
Conversation
WalkthroughAdds orgId propagation to profile-schema attribute creation (handler→service→store), changes ProfileSchemaSync payload tags for OrgId, replaces SchemaInitEvent with SCIM/local attribute events, broadens SyncProfileSchema event handling, adds CDS enablement guards and CDS_NOT_ENABLED error, introduces HasClientErrorCode, and updates tests and test cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Handler as ProfileSchema Handler
participant Admin as AdminConfigService
participant Service as ProfileSchema Service
participant Store as ProfileSchema Store
Client->>Handler: POST ProfileSchemaSync {orgId, event, attributes}
Handler->>Admin: isCDSEnabled(orgId)?
alt CDS disabled
Admin-->>Handler: false
Handler-->>Client: 400 CDS_NOT_ENABLED
else CDS enabled
Handler->>Handler: inspect event type
alt event is Add/Update/Delete SCIM or UpdateLocalAttribute
Handler->>Service: SyncProfileSchema(orgId) / AddProfileSchemaAttributesForScope(attrs, scope, orgId)
Service->>Store: insert/update/delete using provided orgId
Store-->>Service: result
Service-->>Handler: success
Handler-->>Client: 200 "profile schema sync successful"
else unknown event
Handler-->>Client: 404 "Unknown sync event"
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)internal/profile_schema/handler/profile_schema_handler.go (2)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/profile_schema/store/profile_schema_store.go (1)
92-101: Inconsistent orgId reference in error message and log.Lines 92 and 100-101 reference
attrs[0].OrgIdfor error/log messages, but the actual insertion uses theorgIdparameter. If these differ, the messages will be misleading.if err != nil { - errorMsg := fmt.Sprintf("Failed to insert profile schema attributes for org: %s", attrs[0].OrgId) + errorMsg := fmt.Sprintf("Failed to insert profile schema attributes for org: %s", orgId) logger.Debug(errorMsg, log.Error(err)) return errors.NewServerError(errors.ErrorMessage{ Code: errors.ADD_PROFILE_SCHEMA.Code, Message: errors.ADD_PROFILE_SCHEMA.Message, Description: errorMsg, }, err) } logger.Info(fmt.Sprintf("Successfully added %d profile schema attributes for organization: %s", - len(attrs), attrs[0].OrgId)) + len(attrs), orgId)) return nil
🧹 Nitpick comments (2)
internal/profile_schema/model/profile_schema_api.go (1)
21-25: Consider aligning field name with JSON tag for clarity.The field
OrgIdserializes astenantDomainin JSON/BSON. This mismatch may confuse developers—consider renaming the field toTenantDomainor changing the tag toorgIdfor consistency.type ProfileSchemaSync struct { - OrgId string `json:"tenantDomain" bson:"tenantDomain"` + TenantDomain string `json:"tenantDomain" bson:"tenantDomain"` Event string `json:"event" bson:"event"` IdentityAttributes []ProfileSchemaAttribute `json:"identityAttributes" bson:"identityAttributes"` }internal/profile_schema/handler/profile_schema_handler.go (1)
350-361:IdentityAttributesfrom request payload is unused.The
ProfileSchemaSyncstruct now includesIdentityAttributes, which is decoded at line 341, but theAddAttributebranch only usesschemaAtt.OrgIdand callsSyncProfileSchema(orgId). TheIdentityAttributespayload is ignored. If the intent is to use attributes from the request, this needs to be addressed.Would you like me to propose an implementation that uses
schemaAtt.IdentityAttributesinstead of fetching from the identity server?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/profile_schema/handler/profile_schema_handler.go(2 hunks)internal/profile_schema/model/profile_schema_api.go(1 hunks)internal/profile_schema/service/profile_schema_service.go(3 hunks)internal/profile_schema/store/profile_schema_store.go(2 hunks)internal/system/constants/constants.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/profile_schema/store/profile_schema_store.go (2)
internal/profile_schema/model/profile_schema.go (1)
ProfileSchemaAttribute(21-33)internal/system/constants/constants.go (1)
ValueType(87-87)
internal/profile_schema/model/profile_schema_api.go (2)
internal/system/constants/constants.go (1)
IdentityAttributes(81-81)internal/profile_schema/model/profile_schema.go (1)
ProfileSchemaAttribute(21-33)
internal/profile_schema/service/profile_schema_service.go (2)
internal/profile_schema/store/profile_schema_store.go (1)
AddProfileSchemaAttributesForScope(36-103)internal/profile_schema/model/profile_schema.go (1)
ProfileSchemaAttribute(21-33)
🔇 Additional comments (7)
internal/system/constants/constants.go (1)
33-37: LGTM! New event constants are well-defined.The granular attribute lifecycle events (
AddAttribute,UpdateAttribute,DeleteAttribute) replace the singleSchemaInitEvent. Note that the handler currently only handlesAddAttribute—ensureUpdateAttributeandDeleteAttributeare implemented when needed.internal/profile_schema/store/profile_schema_store.go (2)
36-36: Signature change looks correct.The
orgIdparameter is now explicitly passed, aligning with the org-scoped attribute workflow.
83-85: LGTM on using explicit orgId for insertion.Correctly uses the passed
orgIdparameter instead ofattr.OrgIdfor the tenant identifier in the insert values.internal/profile_schema/handler/profile_schema_handler.go (1)
112-112: LGTM on passing orgId to service layer.The call now correctly propagates the organization identifier through to the service and store layers.
internal/profile_schema/service/profile_schema_service.go (3)
38-38: Interface signature updated correctly.The
AddProfileSchemaAttributesForScopemethod now includesorgIdparameter, aligning with the org-scoped workflow.
59-59: Implementation signature matches interface.The receiver method signature is correctly updated to accept
orgId.
97-97: Store call correctly propagates orgId.The
orgIdparameter is now passed through to the store layer, completing the org-scoped attribute creation flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/profile/handler/profile_handler.go (1)
749-752: Good refinement, but apply consistently across similar patterns.The error handling refinement is correct—ignoring
PROFILE_NOT_FOUNDerrors allows the flow to continue and create a new profile for the user at line 754, which is the intended behavior for thePOST_ADD_USERevent.However, similar patterns at lines 815–819 and 837–841 should apply the same logic for consistency.
Lines 815–819 (
POST_DELETE_USER_WITH_IDevent):
The code handles all errors fromFindProfileByUserId, but checksexistingProfile == nilat line 820. With the current logic, aPROFILE_NOT_FOUNDerror prevents reaching that nil check. Consider:existingProfile, err = profilesService.FindProfileByUserId(profileSync.UserId) if err != nil { - utils.HandleError(writer, err) - return + if !utils.HasClientErrorCode(err, errors2.PROFILE_NOT_FOUND.Code) { + utils.HandleError(writer, err) + return + } } if existingProfile == nil { logger.Debug("No profile found for user: " + profileSync.UserId) return }Lines 837–841 (
POST_SET_USER_CLAIM_VALUES_WITH_IDevent):
Similarly, the code returns on all errors but checksexistingProfile == nilat line 842 to create a new profile. Apply the same refinement:existingProfile, err = profilesService.FindProfileByUserId(profileSync.UserId) if err != nil { - utils.HandleError(writer, err) - return + if !utils.HasClientErrorCode(err, errors2.PROFILE_NOT_FOUND.Code) { + utils.HandleError(writer, err) + return + } } if existingProfile == nil {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/profile/handler/profile_handler.go(2 hunks)internal/system/utils/error_utils.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/profile/handler/profile_handler.go (3)
internal/system/utils/error_utils.go (1)
HasClientErrorCode(68-77)internal/system/errors/error_codes.go (1)
PROFILE_NOT_FOUND(182-186)internal/system/utils/http_utils.go (1)
HandleError(35-62)
internal/system/utils/error_utils.go (1)
internal/system/errors/error.go (2)
ClientError(30-33)ErrorMessage(23-28)
🔇 Additional comments (2)
internal/system/utils/error_utils.go (2)
26-28: LGTM! Clean import addition.The
cdsErrorsalias import is well-chosen for clarity and necessary for the new error checking utility.
67-77: LGTM! Well-implemented error utility.The function correctly uses
errors.Asto detect wrappedClientErrorinstances and safely compares error codes. The nil check and fallback logic are both appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/profile_unification_test.go (1)
57-78: Add validation to ensure attribute OrgId matches the orgId parameter.The service layer validates attributes using
attr.OrgId(line 82-87), but the store layer inserts using theorgIdparameter (line 83-85 in store). There is no validation ensuring these two values match.If they ever diverge, attributes could be inserted to the wrong organization while validation was performed against a different one. This creates a security and data integrity risk.
Add explicit validation in the service layer to ensure
attr.OrgId == orgIdbefore calling the store, or refactor to use a single authoritative source for the organization ID.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/profile/handler/profile_handler.go(5 hunks)internal/system/errors/error_codes.go(1 hunks)test/integration/profile_unification_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/system/errors/error_codes.go (1)
internal/system/errors/error.go (1)
ErrorMessage(23-28)
test/integration/profile_unification_test.go (2)
internal/profile_schema/store/profile_schema_store.go (1)
AddProfileSchemaAttributesForScope(36-103)internal/system/constants/constants.go (2)
IdentityAttributes(81-81)Traits(82-82)
🔇 Additional comments (8)
internal/system/errors/error_codes.go (1)
388-392: LGTM - New error code for CDS enablement check.The error definition is clear and follows existing patterns. Note: the code uses prefix
15xxxwhich elsewhere appears alongside server errors, while this is semantically a client/configuration error. Given existing inconsistencies in the file (e.g., mixed prefixes), this is acceptable, but consider standardizing error code prefixes in a future refactor.internal/profile/handler/profile_handler.go (4)
28-30: LGTM - Import for admin config service.The import is required for the new CDS enablement check functionality.
720-730: LGTM - CDS enablement guard for SyncProfile.The early return pattern with appropriate error response is correct for gating CDS-dependent operations.
775-778: LGTM - Improved error handling for POST_ADD_USER flow.Using
HasClientErrorCodeto specifically check forPROFILE_NOT_FOUNDallows the flow to gracefully continue with profile creation for new users, while still propagating unexpected errors.
1053-1057: Error handling in AdminConfigService.IsCDSEnabled is correct.The
IsCDSEnabledimplementation properly handles errors fromstore.GetAdminConfig()and defaults tofalseon failure, which is the safe default for a feature-flag check. It does not panic and maintains predictable behavior across database failures and config fetch errors.test/integration/profile_unification_test.go (3)
24-25: LGTM! Required imports added correctly.The
testingandtimeimports are properly added to support the test infrastructure and unique organization identifier generation.
52-52: Good practice for test isolation.Using
time.Now().UnixNano()to generate unique organization identifiers ensures test isolation across runs. The nanosecond precision provides sufficient uniqueness for integration tests.
318-322: Same consistency concern applies here.Similar to lines 76-78, the attribute at line 319 contains
OrgId: OtherTenant, which is also passed as the third parameter toAddProfileSchemaAttributesForScopeat line 322. Ensure consistency between these two values as mentioned in the earlier comment.
2 participants
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Refactor
Bug Fixes
New Utilities
Tests
✏️ Tip: You can customize this high-level summary in your review settings.