HYPERFLEET-1471 - feat: Server-set tenancy on the resource write path - #339
HYPERFLEET-1471 - feat: Server-set tenancy on the resource write path#339mliptak0 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughSummary by CodeRabbit
WalkthroughResources now derive tenancy from tenant context during creation. System identities and absent tenant context produce empty tenancy JSON. Patch operations preserve existing tenancy. Resource presenters convert stored tenancy JSON into API tenancy maps. Client-supplied tenancy is ignored during conversion and rejected in PATCH payloads. The API specification dependency was upgraded from v1.0.26 to v1.0.27. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant RequestContext
participant ResourceService
participant ResourceStore
participant ResourcePresenter
RequestContext->>ResourceService: provide resolved tenant
ResourceService->>ResourceStore: persist resource with tenancy JSON
ResourceStore-->>ResourceService: return persisted resource
ResourceService->>ResourcePresenter: present persisted resource
ResourcePresenter-->>RequestContext: return tenancy map
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 264 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
| func presentTenancy(t datatypes.JSON) *map[string]string { | ||
| if len(t) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
presentTenancy returns nil for zero-length JSON but {} for stored "{}". Worth normalizing so empty tenancy always serializes the same way.
| func TestPresentResource_WithTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| now := time.Now() | ||
| resource := &api.Resource{ | ||
| Meta: api.Meta{ID: "id", CreatedTime: now, UpdatedTime: now}, | ||
| Kind: "Channel", | ||
| Name: "test", | ||
| Spec: datatypes.JSON(`{}`), | ||
| Tenancy: datatypes.JSON(`{"org":"acme"}`), | ||
| CreatedBy: "user@test.com", | ||
| UpdatedBy: "user@test.com", | ||
| } | ||
|
|
||
| resp := PresentResource(resource) | ||
| Expect(resp.Tenancy).ToNot(BeNil()) | ||
| Expect(*resp.Tenancy).To(HaveKeyWithValue("org", "acme")) | ||
| } | ||
|
|
||
| func TestPresentResource_EmptyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| now := time.Now() |
There was a problem hiding this comment.
Consider consolidating these into a table-driven test. The cases share the same setup and assertions, and a single table will scale better as we add more tenancy presentation scenarios.
| func TestResourceService_Create_StampsTenancyFromContext(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
|
|
||
| ctx := tenant.WithTenant(context.Background(), &tenant.ResolvedTenant{ | ||
| Dimensions: map[string]string{"org": "acme"}, | ||
| }) | ||
| resource := testResource("Channel", "ch-1", "stable") | ||
|
|
||
| result, svcErr := svc.Create(ctx, "Channel", resource, nil) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(string(result.Tenancy)).To(MatchJSON(`{"org":"acme"}`)) | ||
| } | ||
|
|
||
| func TestResourceService_Create_SystemIdentityGetsEmptyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
|
|
||
| ctx := tenant.WithTenant(context.Background(), &tenant.ResolvedTenant{ | ||
| System: true, | ||
| Dimensions: map[string]string{"org": "acme"}, | ||
| }) | ||
| resource := testResource("Channel", "ch-1", "stable") | ||
|
|
||
| result, svcErr := svc.Create(ctx, "Channel", resource, nil) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(string(result.Tenancy)).To(MatchJSON(`{}`)) | ||
| } | ||
|
|
||
| func TestResourceService_Create_NoTenantContext_GetsEmptyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) |
There was a problem hiding this comment.
Consider consolidating the three Create tenancy tests into one table-driven test. They share the same setup and only differ by context input and expected Tenancy JSON, similar to TestTenancyJSON in pkg/tenant/context_test.go.
| Expect(result.UpdatedBy).To(Equal("user@test.com")) | ||
| } | ||
|
|
||
| func TestResourceService_Create_StampsTenancyFromContext(t *testing.T) { |
There was a problem hiding this comment.
Consider a test that pre-sets resource.Tenancy before calling Create and asserts it is overwritten by TenancyJSON(ctx). Create already does this unconditionally, but it is not covered today.
2 participants
Summary
HYPERFLEET-1471
pkg/tenant), never from the request bodyResourcePatchhas no tenancy field; enforced by schema validation and covered by regression tests)hyperfleet-api-specto v1.0.27 to generate thetenancyresponse fieldTest Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Task link: https://redhat.atlassian.net/browse/HYPERFLEET-1471