feat: add TenantInstanceFetchCommand and fix dedicated URL resolution#184
Merged
feat: add TenantInstanceFetchCommand and fix dedicated URL resolution#184
Conversation
Use new /instance endpoint for dedicated URL resolution instead of TenantFetchCommand, avoiding 403 errors from IAM validation. Also make configurationRepoCredentials nullable in TenantSchema. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds TenantInstance fetch support: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Command as TenantInstanceFetchCommand
participant API as Tenant API
participant Parser as Response Parser
participant Handler as Error Handler
Client->>Command: execute(input: by-id | by-name)
Command->>Command: getMethod() -> "GET"
Command->>Command: getPath() -> /api/v1/tenants/by-id/{id}/instance or /api/v1/tenants/by-name/{name}/instance
Command->>API: GET request
alt 200 OK
API-->>Command: { instance: { status, domain } | null }
Command->>Parser: parseResponse(rawResponse)
Parser-->>Command: TenantInstance
Command-->>Client: return TenantInstance
else 404 Not Found
API-->>Command: 404
Command->>Handler: handleClientError(404)
Handler-->>Client: throw NotFoundException (id or name)
else Other Error
API-->>Command: error
Command->>Handler: handleClientError(error)
Handler-->>Client: rethrow error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/tests/commands/tenant.test.ts (1)
83-97: Add a by-id 404 test for parity with the by-name error path.You already verify 404 for name lookup (Line 114). Adding the same assertion for id lookup would harden
NotFoundExceptionmapping for{ id: ... }.Suggested test addition
+ it("should throw NotFoundException for missing tenant instance by id", async () => { + const tenantId = crypto.randomUUID() + fetchMockerBuilder.get(`/api/v1/tenants/by-id/${tenantId}/instance`) + .respondWith(404, { message: "Tenant not found" }) + + const command = new TenantInstanceFetchCommand({ tenantId }) + + await assertRejects( + () => flowcoreClient.execute(command), + NotFoundException, + `Tenant not found: ${JSON.stringify({ id: tenantId })}`, + ) + })Also applies to: 114-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/commands/tenant.test.ts` around lines 83 - 97, Add a parallel 404 test for the id lookup to match the existing by-name error path: mock fetchMockerBuilder.get(`/api/v1/tenants/by-id/${tenantId}/instance`) to respondWith(404) and assert that executing new TenantInstanceFetchCommand({ tenantId }) via flowcoreClient rejects/throws the same NotFoundException mapping you already verify for the name path; update the test file so the id-based failure mirrors the behavior asserted in the by-name 404 test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/tenant/tenant-instance.fetch.ts`:
- Around line 54-57: The routing and error-filter branches use a truthy check on
this.input.tenantId which fails for valid falsy strings; change the guards to
use a discriminant presence check (e.g. if ("tenantId" in this.input) rather
than if ("tenantId" in this.input && this.input.tenantId")) in the
tenant-instance.fetch code paths (the branch that returns
`/api/v1/tenants/by-id/${this.input.tenantId}/instance`) and update the
corresponding error/filter logic that currently checks this.input.tenantId
truthiness to also use the `"tenantId" in this.input` presence check so empty or
falsy string IDs route and produce metadata correctly.
In `@src/contracts/tenant.ts`:
- Around line 151-160: Add an explicit type annotation to the exported
TenantInstanceSchema to match the other schemas; change the declaration of
TenantInstanceSchema so it is typed as TObject<{ isDedicated: boolean; instance:
{ status: string; domain: string } | null }> (i.e., use the same TObject<...>
generic pattern used by TenantSchema/TenantListItemSchema/TenantUserSchema)
while keeping the existing Type.Object(...) initializer and the symbol name
TenantInstanceSchema.
---
Nitpick comments:
In `@test/tests/commands/tenant.test.ts`:
- Around line 83-97: Add a parallel 404 test for the id lookup to match the
existing by-name error path: mock
fetchMockerBuilder.get(`/api/v1/tenants/by-id/${tenantId}/instance`) to
respondWith(404) and assert that executing new TenantInstanceFetchCommand({
tenantId }) via flowcoreClient rejects/throws the same NotFoundException mapping
you already verify for the name path; update the test file so the id-based
failure mirrors the behavior asserted in the by-name 404 test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90554789-6653-4998-b59a-1b0d96c65f8b
📒 Files selected for processing (10)
src/commands/index.tssrc/commands/tenant/tenant-instance.fetch.tssrc/commands/tenant/tenant.fetch.tssrc/common/command.tssrc/common/tenant.cache.tssrc/contracts/index.tssrc/contracts/tenant.tstest/tests/commands/data-core.test.tstest/tests/commands/tenant.test.tstest/tests/common/command.test.ts
Comment on lines
+54
to
+57
| if ("tenantId" in this.input && this.input.tenantId) { | ||
| return `/api/v1/tenants/by-id/${this.input.tenantId}/instance` | ||
| } | ||
| return `/api/v1/tenants/by-name/${this.input.tenant}/instance` |
Contributor
There was a problem hiding this comment.
Avoid truthy checks for tenantId in routing and error filters.
At Line 54 and Line 73, using this.input.tenantId as a truthy guard can pick the wrong branch for falsy string values, producing incorrect paths/error metadata. Use discriminant presence checks consistently.
Proposed fix
protected override getPath(): string {
- if ("tenantId" in this.input && this.input.tenantId) {
+ if ("tenantId" in this.input) {
return `/api/v1/tenants/by-id/${this.input.tenantId}/instance`
}
return `/api/v1/tenants/by-name/${this.input.tenant}/instance`
}
@@
protected override handleClientError(error: ClientError): void {
if (error.status === 404) {
+ const isById = "tenantId" in this.input
throw new NotFoundException("Tenant", {
- [this.input.tenantId ? "id" : "name"]: this.input.tenantId ?? this.input.tenant,
+ [isById ? "id" : "name"]: isById ? this.input.tenantId : this.input.tenant,
})
}
throw error
}Also applies to: 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/tenant/tenant-instance.fetch.ts` around lines 54 - 57, The
routing and error-filter branches use a truthy check on this.input.tenantId
which fails for valid falsy strings; change the guards to use a discriminant
presence check (e.g. if ("tenantId" in this.input) rather than if ("tenantId" in
this.input && this.input.tenantId")) in the tenant-instance.fetch code paths
(the branch that returns
`/api/v1/tenants/by-id/${this.input.tenantId}/instance`) and update the
corresponding error/filter logic that currently checks this.input.tenantId
truthiness to also use the `"tenantId" in this.input` presence check so empty or
falsy string IDs route and produce metadata correctly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/contracts/tenant.ts (1)
47-47: Aligntenant.createresponse parsing with the newly nullable contract.Line 47 and Line 70 now allow
configurationRepoCredentialsto benull, butsrc/commands/tenant/tenant.create.ts:21-37still expectsType.String(). That parser can reject valid responses if create returnsnull.Suggested follow-up diff (outside this file)
- configurationRepoCredentials: Type.String(), + configurationRepoCredentials: Type.Union([Type.String(), Type.Null()]),Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/tenant.ts` at line 47, The response parser in tenant.create (src/commands/tenant/tenant.create.ts) still expects configurationRepoCredentials as Type.String(), but the contract (configurationRepoCredentials) is now nullable (TUnion<[TString, TNull]>), so update the parser to accept both string and null (e.g., use Type.Union([Type.String(), Type.Null()]) or equivalent) and then adjust downstream handling for when configurationRepoCredentials is null; repeat the same change for the other parser occurrence referenced around line 70 so both parsing sites match the nullable contract.src/common/command.ts (1)
62-67: Consider addingtenantIdsupport togetDedicatedBaseUrlfor future commands that may accept ID-only inputs.Lines 62–67 currently extract only
input.tenantfor dedicated URL resolution. While all existing dedicated-subdomain commands provide atenantfield,TenantInstanceFetchCommandsupports both by-name and by-id lookup (evident from itsgetPath()method). If a future command accepts onlytenantIdwithout atenantfield, the dedicated routing would be silently skipped. To maintain consistency with the command's broader capabilities, consider branching to the by-id fetch when onlytenantIdis available:const inputTenant = /* extract tenant name */ const inputTenantId = /* extract tenantId if tenant missing */ if (!inputTenant && !inputTenantId) { return null } let tenant = tenantCache.get(inputTenant ?? inputTenantId) if (!tenant) { const { TenantInstanceFetchCommand } = await import(...) tenant = await client.execute( new TenantInstanceFetchCommand( inputTenant ? { tenant: inputTenant } : { tenantId: inputTenantId } ) ) tenantCache.set(inputTenant ?? inputTenantId, tenant) }Also applies to: 72–76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/command.ts` around lines 62 - 67, getDedicatedBaseUrl currently only checks this.input.tenant and returns null if absent, which skips dedicated routing for commands that supply only a tenantId; update the extraction to also check for a string tenantId from this.input (e.g. const inputTenantId = typeof this.input === "object" && "tenantId" in this.input && typeof this.input.tenantId === "string" && this.input.tenantId), treat the presence of either inputTenant or inputTenantId as valid, and pass the correct shape into TenantInstanceFetchCommand when resolving the tenant via client.execute (use tenantCache.get(inputTenant ?? inputTenantId) and cache with the same key); ensure the code paths and cache keys consistently use inputTenant ?? inputTenantId so both name- and id-only commands work with getDedicatedBaseUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/common/command.ts`:
- Around line 62-67: getDedicatedBaseUrl currently only checks this.input.tenant
and returns null if absent, which skips dedicated routing for commands that
supply only a tenantId; update the extraction to also check for a string
tenantId from this.input (e.g. const inputTenantId = typeof this.input ===
"object" && "tenantId" in this.input && typeof this.input.tenantId === "string"
&& this.input.tenantId), treat the presence of either inputTenant or
inputTenantId as valid, and pass the correct shape into
TenantInstanceFetchCommand when resolving the tenant via client.execute (use
tenantCache.get(inputTenant ?? inputTenantId) and cache with the same key);
ensure the code paths and cache keys consistently use inputTenant ??
inputTenantId so both name- and id-only commands work with getDedicatedBaseUrl.
In `@src/contracts/tenant.ts`:
- Line 47: The response parser in tenant.create
(src/commands/tenant/tenant.create.ts) still expects
configurationRepoCredentials as Type.String(), but the contract
(configurationRepoCredentials) is now nullable (TUnion<[TString, TNull]>), so
update the parser to accept both string and null (e.g., use
Type.Union([Type.String(), Type.Null()]) or equivalent) and then adjust
downstream handling for when configurationRepoCredentials is null; repeat the
same change for the other parser occurrence referenced around line 70 so both
parsing sites match the nullable contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 736ecda2-c1e9-40d8-9918-4a607ff33e34
📒 Files selected for processing (2)
src/common/command.tssrc/contracts/tenant.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
TenantInstanceFetchCommandusing the new/instanceendpoint (by-name/{name}/instanceandby-id/{id}/instance) that requires only authentication, no IAM validationgetDedicatedBaseUrl()to use the new command, eliminating the 403 catch that silently cachedisDedicated: falseconfigurationRepoCredentialsnullable inTenantSchemaandTenantFetchCommandresponse schemaTest plan
TenantInstanceFetchCommand: by-name, by-id, non-dedicated, and 404 casescommand.test.tsto verify/instanceendpointdeno test -A)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests