feat(rest-api): Infer Provider/Tenant from caller's org #2220
Draft
parmani-nv wants to merge 1 commit into
Draft
feat(rest-api): Infer Provider/Tenant from caller's org #2220parmani-nv wants to merge 1 commit into
parmani-nv wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis PR refactors Tenant Account operations to infer Infrastructure Provider context from caller organization membership, deprecating explicit provider and tenant ID parameters. The service account handler now transactionally persists status details. Tenant account queries are optimized with bulk allocation counting and simplified authorization via provider/tenant context derivation. ChangesTenant Account Refactoring
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 |
parmani-nv
force-pushed
the
feat/tenant-handler-inferid
branch
from
7bffadf to
8dfa55a
Compare
…andlers Infer the Infrastructure Provider and Tenant from the caller's org membership across the Tenant Account handlers, rather than requiring the client to pass `infrastructureProviderId`/`tenantId`. - Create: `infrastructureProviderId` in the request body is now optional and inferred from the org's Infrastructure Provider when omitted; when provided it must still match the org's provider. Validation no longer marks it required. - Get-all / Get-by-id: roles are inferred from org membership via `common.IsProviderOrTenant`. `infrastructureProviderId`/`tenantId` query params are deprecated; the Provider-side `tenantId` narrowing filter is preserved, while the Tenant side always pins to its own tenant. Results from the Provider and Tenant perspectives are merged. - Allocation counts are computed in bounded per-provider queries instead of one query per row. - Service-account bootstrap now records an initial "Ready" status detail for the auto-created Tenant Account inside a single transaction. - `StatusHistory` is always serialized as an empty slice (never nil). - Added a `TenantAccountIDs` filter to the Tenant Account DAO. - CLI no longer prompts for the Infrastructure Provider ID on create. - openapi spec and the generated standard SDK updated to reflect the deprecations and the now-optional `infrastructureProviderId`. - [x] **Change** - Changes in existing functionality - [ ] This PR contains breaking changes Backward compatible: the deprecated `infrastructureProviderId`/`tenantId` params are still accepted when supplied. - [x] Unit tests added/updated `docs/index.html` (generated) is excluded from this changeset and should be regenerated from the updated `openapi/spec.yaml`. Signed-off-by: Parham Armani <parmani@nvidia.com>
parmani-nv
force-pushed
the
feat/tenant-handler-inferid
branch
from
8dfa55a to
ea4925d
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rest-api/api/pkg/api/handler/serviceaccount.go (1)
123-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the read-then-create race when bootstrapping Tenant Account.
Line 123 performs the existence check outside the transaction, while Lines 134-157 do the create path without an advisory lock or in-tx re-check. Concurrent requests can both pass pre-check and then contend (duplicate create or 500 on conflict). Put the final existence check inside the tx and lock first.
Suggested fix
if len(tas) == 0 { sdDAO := cdbm.NewStatusDetailDAO(gcsah.dbSession) serr = cdb.WithTx(ctx, gcsah.dbSession, func(tx *cdb.Tx) error { + lockKey := "tenant-account:" + ip.ID.String() + ":" + tn.ID.String() + derr := tx.TryAcquireAdvisoryLock(ctx, cdb.GetAdvisoryLockIDFromString(lockKey), nil) + if derr != nil { + logger.Error().Err(derr).Msg("failed to acquire advisory lock for tenant account bootstrap") + return cutil.NewAPIError(http.StatusInternalServerError, "Failed to create Tenant Account for org's Tenant, DB error", nil) + } + + existing, _, derr := taDAO.GetAll(ctx, tx, cdbm.TenantAccountFilterInput{ + InfrastructureProviderID: &ip.ID, + TenantIDs: []uuid.UUID{tn.ID}, + }, cdbp.PageInput{}, nil) + if derr != nil { + logger.Error().Err(derr).Msg("error re-checking Tenant Account in transaction") + return cutil.NewAPIError(http.StatusInternalServerError, "Failed to retrieve Tenant Account for org's Tenant, DB error", nil) + } + if len(existing) > 0 { + return nil + } + - ta, derr := taDAO.Create(ctx, tx, cdbm.TenantAccountCreateInput{ + ta, derr := taDAO.Create(ctx, tx, cdbm.TenantAccountCreateInput{ AccountNumber: common.GenerateAccountNumber(), TenantID: &tn.ID, TenantOrg: org, InfrastructureProviderID: ip.ID, InfrastructureProviderOrg: ip.Org, Status: cdbm.TenantAccountStatusReady, CreatedBy: dbUser.ID, })As per coding guidelines: “Acquire an advisory lock at the top of transaction closures for TOCTOU-prone flows using
tx.TryAcquireAdvisoryLock(); re-read pre-checked values inside the tx.”🤖 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 `@rest-api/api/pkg/api/handler/serviceaccount.go` around lines 123 - 157, The pre-check for existing TenantAccount (taDAO.GetAll) is done outside the transaction causing a TOCTOU race; move the final existence check into the transaction started by cdb.WithTx, acquire the advisory lock via tx.TryAcquireAdvisoryLock() at the start of the tx closure, re-query TenantAccount (using taDAO.GetAll or a GetByTenant+Provider method) inside that tx, and only call taDAO.Create and sdDAO.CreateFromParams if the in-tx re-check still finds zero records; ensure any errors from TryAcquireAdvisoryLock or the in-transaction read are handled and return proper API errors so concurrent requests don’t create duplicates or cause conflicts.Source: Coding guidelines
rest-api/sdk/standard/model_tenant_account_create_request.go (1)
36-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep the deprecated constructor path source-compatible.
infrastructureProviderIdis only deprecated on the wire, but this change removes it fromNewTenantAccountCreateRequest. Existing SDK callers that still pass the deprecated field will now fail to compile even though the server still accepts that payload. Please preserve a compatibility constructor/shim until the field is actually removed from the API.🤖 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 `@rest-api/sdk/standard/model_tenant_account_create_request.go` around lines 36 - 43, NewTenantAccountCreateRequest currently drops the deprecated infrastructureProviderId parameter which breaks existing callers; add a backward-compatible constructor/shim (e.g., NewTenantAccountCreateRequestWithDeprecatedProvider or overload NewTenantAccountCreateRequest that accepts an extra infrastructureProviderId string) that sets TenantAccountCreateRequest.InfrastructureProviderId when provided and delegates to the current constructor logic, mark it deprecated in the comment, and keep the original NewTenantAccountCreateRequest(tenantOrg string) intact so existing callers compiling against SDK continue to work while the field remains wire-deprecated.
🧹 Nitpick comments (3)
rest-api/db/pkg/db/model/tenantaccount.go (1)
347-354: ⚡ Quick winRefine tracer attribute naming for consistency.
The tracer attribute is set to
"id"(line 353), which is less specific than the naming convention used by other filters in this function. Other filters use descriptive names:"tenant_id"(line 320),"infrastructure_provider_id"(line 335),"status"(line 344),"tenant_org"(line 330). Using a generic"id"key reduces trace clarity and could conflict with other ID attributes.🔍 Proposed refinement
if filter.TenantAccountIDs != nil { if len(filter.TenantAccountIDs) == 1 { query = query.Where("ta.id = ?", filter.TenantAccountIDs[0]) } else { query = query.Where("ta.id IN (?)", bun.In(filter.TenantAccountIDs)) } - tasd.tracerSpan.SetAttribute(tnaDAOSpan, "id", filter.TenantAccountIDs) + tasd.tracerSpan.SetAttribute(tnaDAOSpan, "tenant_account_ids", filter.TenantAccountIDs) }🤖 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 `@rest-api/db/pkg/db/model/tenantaccount.go` around lines 347 - 354, The tracer attribute key for tenant account IDs is too generic ("id"); update the SetAttribute call in the block handling filter.TenantAccountIDs to use a descriptive key such as "tenant_account_id" (i.e., replace the current tasd.tracerSpan.SetAttribute(tnaDAOSpan, "id", filter.TenantAccountIDs) with a call that uses "tenant_account_id") so it follows the existing naming convention used by other filters (e.g., "tenant_id", "infrastructure_provider_id", "status", "tenant_org").rest-api/api/pkg/api/handler/tenantaccount_test.go (1)
260-261: ⚡ Quick winIsolate the inferred-provider success path to one behavior.
Line 260 and Lines 387-395 currently validate provider inference and “tenant does not exist” in the same case (
expectedTenantID: nil). Please make this case use an existing tenant org so it validates only inference behavior.♻️ Suggested adjustment
@@ tn2 := testTenantAccountBuildTenant(t, dbSession, tnOrg2, "Tenant Org 2", tnu) assert.NotNil(t, tn2) + tnInfer := testTenantAccountBuildTenant(t, dbSession, "test-tn-org-infer", "Tenant Org Infer", tnu) + assert.NotNil(t, tnInfer) @@ - okBodyInferIP, err := json.Marshal(model.APITenantAccountCreateRequest{TenantOrg: cutil.GetPtr("test-tn-org-infer")}) + okBodyInferIP, err := json.Marshal(model.APITenantAccountCreateRequest{TenantOrg: cutil.GetPtr(tnInfer.Org)}) assert.Nil(t, err) @@ { name: "success when infrastructureProviderId is omitted (inferred from org)", reqOrgName: ipOrg1, reqBody: string(okBodyInferIP), user: ipu, expectedErr: false, expectedStatus: http.StatusCreated, - expectedTenantID: nil, + expectedTenantID: &tnInfer.ID, },Based on learnings: “In handler tests, ensure each failure-path test intentionally targets a single validation branch.”
Also applies to: 387-395
🤖 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 `@rest-api/api/pkg/api/handler/tenantaccount_test.go` around lines 260 - 261, The test case that builds okBodyInferIP uses model.APITenantAccountCreateRequest{TenantOrg: cutil.GetPtr("test-tn-org-infer")} which currently exercises both provider-inference and "tenant does not exist" checks because expectedTenantID is nil; change the TenantOrg value to an existing test tenant org (e.g. the fixture org used elsewhere in this file) and set the test case's expectedTenantID to that tenant's ID (non-nil) so this case only validates provider inference; update the okBodyInferIP construction and the corresponding expectedTenantID entry in the same test case accordingly.Source: Learnings
rest-api/sdk/standard/api_tenant_account.go (1)
288-288: 💤 Low valueHTML entity in deprecation comments.
The deprecation doc comments use
'(HTML entity for apostrophe) instead of a plain single quote'. While Go doc tools may handle this, it reads awkwardly in the source and ingodocoutput.Since this is generated code, the fix should likely be applied to the OpenAPI spec or the generator template to emit plain quotes.
Also applies to: 475-475
🤖 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 `@rest-api/sdk/standard/api_tenant_account.go` at line 288, Replace the HTML entity &`#39`; in the deprecation comments with a plain apostrophe; update the generator template or the OpenAPI spec so comments like "Filter TenantAccounts by Infrastructure Provider ID. Deprecated: Infrastructure Provider is now inferred from the org&`#39`;s membership." (in rest-api/sdk/standard/api_tenant_account.go) are emitted as "org's" instead of "org&`#39`;s". Locate the comment block(s) tied to TenantAccount filtering/deprecation in the generator templates or the OpenAPI spec and change the encoding to output a plain single quote, then regenerate the file (affects the same deprecation comment at the second occurrence as well).
🤖 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 `@rest-api/api/pkg/api/handler/tenantaccount.go`:
- Around line 410-425: The current logic uses aDAO.GetAll with a fixed allocPage
(cdbp.TotalLimit) and then tallies allocations into allocCountByProviderTenant,
which truncates counts when a provider has more allocations than TotalLimit;
change this to a grouped count query per tenant (or fully exhaust pagination)
instead of GetAll: add/use a DAO method that returns counts grouped by TenantID
when given InfrastructureProviderID and TenantIDs (e.g., aDAO.CountByTenant or
GetCountsByProviderTenant) and populate allocCountByProviderTenant[ipID] from
that grouped-count result (or implement a loop that pages through aDAO.GetAll
until no more results and accumulates counts), updating the allocation-count
aggregation code that currently iterates over als and increments countByTenant.
- Around line 340-371: The prequeries that build mergedTenantAccountIDs use
totalLimitPage (Limit: cdbp.TotalLimit) which caps the ID universe; change those
prequeries to fetch all matching TenantAccount IDs without a Limit (e.g., use an
unbounded PageInput / nil Limit or a DAO method that returns IDs for a filter)
so mergedTenantAccountIDs contains the full union, then perform the final
paginated query with taDAO.GetAll using pageRequest.ConvertToDB() and
qIncludeRelations; update the calls that set totalLimitPage and the
provider/tenant GetAll invocations (taDAO.GetAll) to use the unbounded fetch or
a dedicated GetIDs method to avoid truncating total and pages.
In `@rest-api/openapi/spec.yaml`:
- Around line 539-540: The tenantId parameter is only updated in the description
but not flagged deprecated for the get-all-tenant-account operation; update the
OpenAPI spec so the parameter named tenantId used by operationId
get-all-tenant-account includes deprecated: true (and keep or append the
deprecation note in the description), ensuring the parameter block for tenantId
has the deprecated flag to match the get-tenant-account deprecation metadata.
---
Outside diff comments:
In `@rest-api/api/pkg/api/handler/serviceaccount.go`:
- Around line 123-157: The pre-check for existing TenantAccount (taDAO.GetAll)
is done outside the transaction causing a TOCTOU race; move the final existence
check into the transaction started by cdb.WithTx, acquire the advisory lock via
tx.TryAcquireAdvisoryLock() at the start of the tx closure, re-query
TenantAccount (using taDAO.GetAll or a GetByTenant+Provider method) inside that
tx, and only call taDAO.Create and sdDAO.CreateFromParams if the in-tx re-check
still finds zero records; ensure any errors from TryAcquireAdvisoryLock or the
in-transaction read are handled and return proper API errors so concurrent
requests don’t create duplicates or cause conflicts.
In `@rest-api/sdk/standard/model_tenant_account_create_request.go`:
- Around line 36-43: NewTenantAccountCreateRequest currently drops the
deprecated infrastructureProviderId parameter which breaks existing callers; add
a backward-compatible constructor/shim (e.g.,
NewTenantAccountCreateRequestWithDeprecatedProvider or overload
NewTenantAccountCreateRequest that accepts an extra infrastructureProviderId
string) that sets TenantAccountCreateRequest.InfrastructureProviderId when
provided and delegates to the current constructor logic, mark it deprecated in
the comment, and keep the original NewTenantAccountCreateRequest(tenantOrg
string) intact so existing callers compiling against SDK continue to work while
the field remains wire-deprecated.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/tenantaccount_test.go`:
- Around line 260-261: The test case that builds okBodyInferIP uses
model.APITenantAccountCreateRequest{TenantOrg:
cutil.GetPtr("test-tn-org-infer")} which currently exercises both
provider-inference and "tenant does not exist" checks because expectedTenantID
is nil; change the TenantOrg value to an existing test tenant org (e.g. the
fixture org used elsewhere in this file) and set the test case's
expectedTenantID to that tenant's ID (non-nil) so this case only validates
provider inference; update the okBodyInferIP construction and the corresponding
expectedTenantID entry in the same test case accordingly.
In `@rest-api/db/pkg/db/model/tenantaccount.go`:
- Around line 347-354: The tracer attribute key for tenant account IDs is too
generic ("id"); update the SetAttribute call in the block handling
filter.TenantAccountIDs to use a descriptive key such as "tenant_account_id"
(i.e., replace the current tasd.tracerSpan.SetAttribute(tnaDAOSpan, "id",
filter.TenantAccountIDs) with a call that uses "tenant_account_id") so it
follows the existing naming convention used by other filters (e.g., "tenant_id",
"infrastructure_provider_id", "status", "tenant_org").
In `@rest-api/sdk/standard/api_tenant_account.go`:
- Line 288: Replace the HTML entity &`#39`; in the deprecation comments with a
plain apostrophe; update the generator template or the OpenAPI spec so comments
like "Filter TenantAccounts by Infrastructure Provider ID. Deprecated:
Infrastructure Provider is now inferred from the org&`#39`;s membership." (in
rest-api/sdk/standard/api_tenant_account.go) are emitted as "org's" instead of
"org&`#39`;s". Locate the comment block(s) tied to TenantAccount
filtering/deprecation in the generator templates or the OpenAPI spec and change
the encoding to output a plain single quote, then regenerate the file (affects
the same deprecation comment at the second occurrence as well).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a39dded2-2f37-45ae-91df-43769338fc30
📒 Files selected for processing (12)
rest-api/api/pkg/api/handler/serviceaccount.gorest-api/api/pkg/api/handler/serviceaccount_test.gorest-api/api/pkg/api/handler/tenantaccount.gorest-api/api/pkg/api/handler/tenantaccount_test.gorest-api/api/pkg/api/model/tenantaccount.gorest-api/api/pkg/api/model/tenantaccount_test.gorest-api/cli/tui/commands.gorest-api/db/pkg/db/model/tenantaccount.gorest-api/db/pkg/migrations/20260604120000_backfill_tenant_account_status_detail.gorest-api/openapi/spec.yamlrest-api/sdk/standard/api_tenant_account.gorest-api/sdk/standard/model_tenant_account_create_request.go
Comment on lines
+340
to
+371
| totalLimitPage := cdbp.PageInput{Limit: cutil.GetPtr(cdbp.TotalLimit)} | ||
|
|
||
| if provider != nil { | ||
| providerFilter := sharedFilter | ||
| providerFilter.InfrastructureProviderID = &provider.ID | ||
| providerFilter.TenantIDs = filterTenantIDs | ||
| tenantAccountsFromProviderPerspective, _, err := taDAO.GetAll(ctx, nil, providerFilter, totalLimitPage, nil) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error getting TenantAccounts from Provider perspective") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve TenantAccounts, DB error", nil) | ||
| } | ||
| for _, tenantAccount := range tenantAccountsFromProviderPerspective { | ||
| mergedTenantAccountIDs.Add(tenantAccount.ID) | ||
| } | ||
| } | ||
|
|
||
| filter := cdbm.TenantAccountFilterInput{ | ||
| InfrastructureProviderID: infrastructureProviderID, | ||
| TenantIDs: tenantIDs, | ||
| Statuses: statuses, | ||
| SearchQuery: searchQuery, | ||
| if tenant != nil { | ||
| tenantFilter := sharedFilter | ||
| tenantFilter.TenantIDs = []uuid.UUID{tenant.ID} | ||
| tenantAccountsFromTenantPerspective, _, err := taDAO.GetAll(ctx, nil, tenantFilter, totalLimitPage, nil) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("error getting TenantAccounts from Tenant perspective") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to retrieve TenantAccounts, DB error", nil) | ||
| } | ||
| for _, tenantAccount := range tenantAccountsFromTenantPerspective { | ||
| mergedTenantAccountIDs.Add(tenantAccount.ID) | ||
| } | ||
| } | ||
|
|
||
| tas, total, err := taDAO.GetAll(ctx, nil, filter, cdbp.PageInput{ | ||
| Offset: pageRequest.Offset, | ||
| Limit: pageRequest.Limit, | ||
| OrderBy: pageRequest.OrderBy, | ||
| }, qIncludeRelations) | ||
| tas, total, err := taDAO.GetAll(ctx, nil, cdbm.TenantAccountFilterInput{ | ||
| TenantAccountIDs: mergedTenantAccountIDs.ToSlice(), | ||
| }, pageRequest.ConvertToDB(), qIncludeRelations) |
Contributor
There was a problem hiding this comment.
Don't let TotalLimit define the tenant-account universe.
The provider/tenant prequeries build mergedTenantAccountIDs with Limit: cdbp.TotalLimit, and the final paginated GetAll is then restricted to that capped ID set. Once a caller has more than TotalLimit matching tenant accounts, later rows disappear from every page and total is understated. This union needs to be computed without a hard cap, or expressed as a single DAO query that paginates after authorization filtering.
🤖 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 `@rest-api/api/pkg/api/handler/tenantaccount.go` around lines 340 - 371, The
prequeries that build mergedTenantAccountIDs use totalLimitPage (Limit:
cdbp.TotalLimit) which caps the ID universe; change those prequeries to fetch
all matching TenantAccount IDs without a Limit (e.g., use an unbounded PageInput
/ nil Limit or a DAO method that returns IDs for a filter) so
mergedTenantAccountIDs contains the full union, then perform the final paginated
query with taDAO.GetAll using pageRequest.ConvertToDB() and qIncludeRelations;
update the calls that set totalLimitPage and the provider/tenant GetAll
invocations (taDAO.GetAll) to use the unbounded fetch or a dedicated GetIDs
method to avoid truncating total and pages.
Contributor
Author
There was a problem hiding this comment.
cdbp.TotalLimit = -1, It's not capped.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "TotalLimit" --type go -C3Length of output: 198394
@parmani-nv, you are correct — apologies for the noise. The paginator resolves TotalLimit = -1 as a sentinel: it queries the total count first and then sets the effective limit to that value, resulting in an unbounded fetch. The pre-queries building mergedTenantAccountIDs are therefore not capped and the concern was unfounded.
(´• ω •`) 🐇
Comment on lines
539
to
+540
| name: tenantId | ||
| description: Filter TenantAccounts by Tenant ID | ||
| description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored).' |
Contributor
There was a problem hiding this comment.
Mark tenantId as deprecated for get-all-tenant-account to keep deprecation metadata consistent.
get-tenant-account deprecates tenantId, but get-all-tenant-account currently only updates its description. This creates an avoidable contract mismatch for docs and generated SDKs.
Suggested patch
- schema:
type: string
format: uuid
in: query
name: tenantId
+ deprecated: true
description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored).'As per coding guidelines, “Review the OpenAPI specification, check for consistency and correctness, check for misspellings and grammatical errors.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: tenantId | |
| description: Filter TenantAccounts by Tenant ID | |
| description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored).' | |
| - schema: | |
| type: string | |
| format: uuid | |
| in: query | |
| name: tenantId | |
| deprecated: true | |
| description: 'Filter TenantAccounts by Tenant ID (Provider role only; for Tenant role the tenant is inferred from org membership and this param is ignored).' |
🤖 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 `@rest-api/openapi/spec.yaml` around lines 539 - 540, The tenantId parameter is
only updated in the description but not flagged deprecated for the
get-all-tenant-account operation; update the OpenAPI spec so the parameter named
tenantId used by operationId get-all-tenant-account includes deprecated: true
(and keep or append the deprecation note in the description), ensuring the
parameter block for tenantId has the deprecated flag to match the
get-tenant-account deprecation metadata.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: Infer Provider/Tenant from caller's org across Tenant Account handlers
Description
Infer the Infrastructure Provider and Tenant from the caller's org
membership across the Tenant Account handlers, rather than requiring the
client to pass
infrastructureProviderId/tenantId.infrastructureProviderIdin the request body is now optionaland inferred from the org's Infrastructure Provider when omitted; when
provided it must still match the org's provider. Validation no longer
marks it required.
common.IsProviderOrTenant.infrastructureProviderId/tenantIdquery params are deprecated; the Provider-side
tenantIdnarrowingfilter is preserved, while the Tenant side always pins to its own
tenant. Results from the Provider and Tenant perspectives are merged.
of one query per row.
for the auto-created Tenant Account inside a single transaction.
StatusHistoryis always serialized as an empty slice (never nil).TenantAccountIDsfilter to the Tenant Account DAO.deprecations and the now-optional
infrastructureProviderId.Type of Change
Breaking Changes
Testing
Additional Notes
Backward compatible: the deprecated
infrastructureProviderId/tenantIdparams are still accepted when supplied.