[ENH] Add RPC for setting tenant resource_name and getting collection by CRN #4735

drewkim · 2025-06-03T20:50:37Z

Description of changes

Summarize the changes made by this PR.

Improvements & Bug fixes
- None
New functionality
- Exposes a new RPC/query for setting the resource_name on a tenant
- Updates the Tenant model to include resource_name, expressed on GetTenant
- Exposes a new RPC/query for getting a collection by CRN (tenant_resource_name, database, name)

Test plan

How are these changes tested?

Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

github-actions · 2025-06-03T20:50:51Z

drewkim · 2025-06-03T20:51:00Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

propel-code-bot · 2025-06-03T20:52:00Z

Add Support for Tenant Resource Name and Get Collection by CRN RPCs

This major PR introduces new SysDB APIs allowing tenants to have a unique, immutable resource_name (customer resource name, CRN), and exposes a new RPC to retrieve a collection by this resource name, database, and collection name. The changes flow from protobuf contract updates to full server, data access, test, and model implementations, and enforce the one-time-set-only semantics for resource names. All impacted code paths have thorough new tests (datastore, API, end-to-end), codegen, and documentation updates.

Key Changes:
• Extend the Tenant proto/model with a nullable resource_name, and add relevant fields to all API layers.
• Add SetTenantResourceName RPC/method: only allows setting resource_name if unset.
• Add GetCollectionByResourceName RPC/method: retrieve a collection based on tenant_resource_name, database, and name.
• Implement data access methods enforcing immutability of resource_name and correct error handling (e.g., resource_name can only be set once).
• Update .proto files, code generation, and test cases in all layers (DAO, coordinator, gRPC server, E2E).
• Expose the new fields in GetTenant responses.
• Extensively test positive/negative flows for new functionality at database, API, and contract levels.
• Expand error surface: introduce ErrTenantResourceNameAlreadySet error.

Affected Areas:
• Protobuf contracts (coordinator.proto, chroma.proto)
• Tenant model in Go
• tenants DAO/data access (set/get resource_name)
• Coordinator and Catalog logic
• gRPC service implementations for tenant and collection
• Test suites (DAO, service, coordinator, integration)
• Mocks/codegen

Potential Impact:

Functionality: Introduces a new pattern for uniquely identifying tenants and collections by externally meaningful resource_name, enabling clients to resolve resources through CRN patterns as well as legacy IDs. Changes GetTenant and collection flows with new properties and lookup semantics. Tenant resource_name immutability is strictly enforced.

Performance: No significant runtime performance impact; new lookups are single-row lookups or add predictable DB joins.

Security: If resource_name is leaked or misused, could permit unintended resource access; implementation enforces immutability and uniqueness to minimize risk.

Scalability: Allows for large cross-tenant queries and managed directory structures at scale, but no new hot paths or scaling bottlenecks introduced.

Review Focus:
• Correctness and safety of the one-time-set resource_name (verify update logic null check).
• Error paths: correct distinctions between not found, already set, and generic errors.
• Whether all new/affected APIs and models propagate the resource_name correctly throughout schema and responses.
• gRPC, proto, and contract changes for clarity and future extensibility.
• Database uniqueness enforcement: ensure proper uniqueness and null-ness constraints at the DB and ORM levels.
• Tests for edge cases of nulls/missing/non-unique resource_name.
• Backwards compatibility (e.g., Tenant and collection APIs should continue to work with legacy IDs).

Testing Needed

• Exercise all new RPCs for positive and error paths (including trying to re-set resource_name).
• Perform get/set flows using both old tenant IDs and new resource_name method, verifying correctness and error conditions.
• Test collection retrieval by CRN with various input combinations (valid, non-existent tenant, non-existent collection/db/name).
• Verify immutability: resource_name is never updated after set.
• Review added and extended unit/integration tests for completeness.

Code Quality Assessment

go/pkg/sysdb/metastore/db/dao/tenant.go: Correctly enforces immutability via SQL 'IS NULL' checks; handles all error scenarios; robust logging.

go/pkg/sysdb/metastore/db/dao/collection.go: Adds CRN query path safely; joins tenant table by resource_name and reuses shared listing methods.

idl/chromadb/proto/coordinator.proto: Clearly documents new RPCs/fields; keeps backwards compatibility; all affected messages updated.

go/pkg/sysdb/grpc/tenant_database_service.go: Wires through new RPC cleanly; consistent error mapping.

tests: Thorough coverage; valid error case assertions; covers all branches of new logic.

go/pkg/sysdb/coordinator/table_catalog.go: Implements new methods with clear error handling and correct propagation; follows established catalog patterns.

Best Practices

API Versioning:
• Protobuf optional fields to preserve compatibility
• All new RPCs and fields are additive
• Explicit error messages and pathology checks

Immutability:
• Immutability enforced at data access layer; cannot update non-null resource_name
• Error handling covers both missing tenant and already-set name

Test Coverage:
• Extensive positive and negative test cases at all layers
• Mock regeneration and end-to-end verification

Potential Issues

• If an operator mistakenly assigns resource_name incorrectly, it is immutable and cannot be recovered (by design).
• If multiple tenants are imported/migrated with the same resource_name, unique index will fail (expected, but requires attention in migrations).
• Legacy flows unaware of resource_name could present confusing errors; legacy support is present but phased.
• Potential ambiguity for clients if resource_name and tenant IDs are both supplied/used; caller behavior must be clarified in clients.

This summary was automatically generated by @propel-code-bot

drewkim · 2025-06-04T05:17:21Z

go/pkg/sysdb/metastore/db/dao/tenant.go

+	var tenants []dbmodel.Tenant
+	result := s.db.Model(&tenants).
+		Clauses(clause.Returning{Columns: []clause.Column{{Name: "id"}}}).
+		Where("id = ? AND resource_name IS NULL", tenantID).


The IS NULL check enforces that you can only set the resource_name once

drewkim · 2025-06-04T05:17:45Z

go/pkg/sysdb/metastore/db/dao/tenant.go

+			return common.ErrTenantNotFound
+		}
+		return common.ErrTenantResourceNameAlreadySet
+	}


Check to see if the tenant exists or not in order to return the correct error

idl/chromadb/proto/coordinator.proto

rust/sysdb/src/sysdb.rs

HammadB · 2025-06-10T00:53:37Z

go/pkg/sysdb/coordinator/coordinator.go

@@ -111,6 +111,10 @@ func (s *Coordinator) GetCollections(ctx context.Context, collectionIDs []types.
 	return s.catalog.GetCollections(ctx, collectionIDs, collectionName, tenantID, databaseName, limit, offset, includeSoftDeleted)
 }

+func (s *Coordinator) GetCollectionByResourceName(ctx context.Context, tenantResourceName string, databaseName string, collectionName string) (*model.Collection, error) {


Do we want to make the sysdb the source of truth for this parsing? I think thats a better design / encapsulation of the parsing rather than pushing it to calling services

HammadB · 2025-06-10T00:54:00Z

go/pkg/sysdb/coordinator/coordinator.go

@@ -229,6 +233,10 @@ func (s *Coordinator) GetTenantsLastCompactionTime(ctx context.Context, tenantID
 	return s.catalog.GetTenantsLastCompactionTime(ctx, tenantIDs)
 }

+func (s *Coordinator) SetTenantResourceName(ctx context.Context, tenantID string, resourceName string) error {


Should we instead prefer having a UpdateTenant call ? cc @sanketkedia

HammadB · 2025-06-10T00:54:24Z

go/pkg/sysdb/coordinator/model/tenant.go

@@ -3,7 +3,8 @@ package model
 import "github.com/chroma-core/chroma/go/pkg/types"

 type Tenant struct {
-	Name string
+	Name         string
+	ResourceName *string


would be good to leave a comment on what this is

HammadB · 2025-06-10T00:55:00Z

idl/chromadb/proto/coordinator.proto

@@ -192,6 +192,13 @@ message GetCollectionsResponse {
  reserved "status";
 }

+message GetCollectionByResourceNameRequest {
+  string id = 1;
+  optional string tenant_resource_name = 2;


why are these marked as optional?

HammadB · 2025-06-10T00:55:17Z

idl/chromadb/proto/coordinator.proto

+}
+
+message SetTenantResourceNameResponse {
+  reserved 1;


Why reserve these?

drewkim mentioned this pull request Jun 3, 2025

[ENH] Add resource_name to SysDB tenants table #4730

Open

1 task

drewkim marked this pull request as ready for review June 3, 2025 20:51

drewkim requested review from HammadB and removed request for HammadB June 3, 2025 20:52

drewkim marked this pull request as draft June 3, 2025 21:12

drewkim mentioned this pull request Jun 3, 2025

[ENH] Implement update_tenant API #4742

Open

1 task

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch 2 times, most recently from b17758d to b8f2529 Compare June 4, 2025 04:12

drewkim changed the title ~~[ENH] Add RPC/query for setting tenant static_name~~ [ENH] Add RPC/query for setting tenant resource_name Jun 4, 2025

drewkim marked this pull request as ready for review June 4, 2025 04:59

drewkim requested a review from HammadB June 4, 2025 04:59

drewkim force-pushed the drew/_enh_add_static_name_to_sysdb_tenants_table branch from 48caba8 to 67d8771 Compare June 4, 2025 05:16

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch from f6661cf to 628ecae Compare June 4, 2025 05:16

drewkim commented Jun 4, 2025

View reviewed changes

drewkim requested review from codetheweb and sanketkedia June 4, 2025 05:32

drewkim commented Jun 4, 2025

View reviewed changes

idl/chromadb/proto/coordinator.proto Outdated Show resolved Hide resolved

drewkim force-pushed the drew/_enh_add_static_name_to_sysdb_tenants_table branch from 67d8771 to 1fff500 Compare June 4, 2025 17:31

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch from d28bc91 to 8332e31 Compare June 4, 2025 17:31

drewkim commented Jun 4, 2025

View reviewed changes

rust/sysdb/src/sysdb.rs Outdated Show resolved Hide resolved

drewkim force-pushed the drew/_enh_add_static_name_to_sysdb_tenants_table branch from 1fff500 to e6f6417 Compare June 4, 2025 18:30

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch from dd5547e to 2c288bf Compare June 4, 2025 18:30

codetheweb approved these changes Jun 4, 2025

View reviewed changes

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch 3 times, most recently from 1bc4b1b to 8b83952 Compare June 4, 2025 21:25

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch 4 times, most recently from fb2d83b to 7a5ad5a Compare June 5, 2025 17:34

drewkim added 9 commits June 5, 2025 11:57

[ENH] Add RPC/query for setting tenant static_name

fede98e

update GetTenant

38e7c7a

static_name -> resource_name

af8a238

test case

559429f

enforce set once

d3bcbcf

add resource_name to GetTenantRequest

1cab451

additional proto field

df51602

fix proto optional fields

0590edc

fix test

ad222e3

drewkim force-pushed the drew/_enh_add_static_name_to_sysdb_tenants_table branch from 6d6dd56 to 1fc07be Compare June 5, 2025 18:58

drewkim force-pushed the drew/_enh_add_rpc_query_for_setting_tenant_static_name branch from 7a5ad5a to ad222e3 Compare June 5, 2025 18:58

drewkim added 2 commits June 5, 2025 17:27

update protos

d4e0d59

update protos

07d2c77

drewkim changed the title ~~[ENH] Add RPC/query for setting tenant resource_name~~ [ENH] Add RPCfor setting tenant resource_name and getting collection by CRN Jun 6, 2025

RPC for getting collection by CRN

a756d87

drewkim changed the title ~~[ENH] Add RPCfor setting tenant resource_name and getting collection by CRN~~ [ENH] Add RPC for setting tenant resource_name and getting collection by CRN Jun 6, 2025

HammadB reviewed Jun 10, 2025

View reviewed changes

idl/chromadb/proto/coordinator.proto

}

message SetTenantResourceNameResponse {

reserved 1;

Copy link

Collaborator

HammadB Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why reserve these?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ENH] Add RPC for setting tenant resource_name and getting collection by CRN #4735

[ENH] Add RPC for setting tenant resource_name and getting collection by CRN #4735

drewkim commented Jun 3, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 3, 2025

Uh oh!

drewkim commented Jun 3, 2025 •

edited

Loading

Uh oh!

propel-code-bot bot commented Jun 3, 2025 •

edited

Loading

Uh oh!

drewkim Jun 4, 2025

Uh oh!

drewkim Jun 4, 2025

Uh oh!

Uh oh!

Uh oh!

HammadB Jun 10, 2025

Uh oh!

HammadB Jun 10, 2025

Uh oh!

HammadB Jun 10, 2025

Uh oh!

HammadB Jun 10, 2025

Uh oh!

HammadB Jun 10, 2025

Uh oh!

Uh oh!

[ENH] Add RPC for setting tenant resource_name and getting collection by CRN #4735

Are you sure you want to change the base?

[ENH] Add RPC for setting tenant resource_name and getting collection by CRN #4735

Conversation

drewkim commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Test plan

Documentation Changes

Uh oh!

github-actions bot commented Jun 3, 2025

Reviewer Checklist

Testing, Bugs, Errors, Logs, Documentation

System Compatibility

Quality

Uh oh!

drewkim commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

propel-code-bot bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drewkim Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

drewkim Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HammadB Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

HammadB Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drewkim commented Jun 3, 2025 •

edited

Loading

drewkim commented Jun 3, 2025 •

edited

Loading

propel-code-bot bot commented Jun 3, 2025 •

edited

Loading