[CHORE]: Consolidate all attached function getters #5884
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Chore: Consolidate Attached-Function Getters into a Single Filter-Based API Across Go, Rust & gRPC This PR removes all specialised "get attached functions" helpers and replaces them with a single, filter-driven endpoint that is surfaced consistently in the data layer, gRPC service and CLI. By centralising the logic, we eliminate duplicated code paths, ensure uniform permission checks and make it easier to extend the query surface (e.g., new filters) in the future. All internal callers, generated mocks and tests have been migrated; external clients must switch to the new ListAttachedFunctions RPC and regenerate proto stubs. Key Changes• Deleted legacy methods GetAttachedFunctionsByCollection/Tenant/Task in both Go and Rust DAOs Affected Areas• SysDb / DAO layer (Go & Rust) This summary was automatically generated by @propel-code-bot |
tanujnay112
force-pushed
the
attach_fn2
branch
2 times, most recently
from
e713158 to
b6edf1a
Compare
tanujnay112
force-pushed
the
consolidate_getters
branch
from
2d9b719 to
151680b
Compare
This comment has been minimized.
This comment has been minimized.
tanujnay112
force-pushed
the
graphite-base/5884
branch
from
b6edf1a to
4d9d369
Compare
tanujnay112
force-pushed
the
consolidate_getters
branch
from
151680b to
6cfa1f5
Compare
| err := s.catalog.txImpl.Transaction(ctx, func(txCtx context.Context) error { | ||
| // Double-check attached function doesn't exist (check both ready and not-ready) | ||
| concurrentAttachedFunction, err := s.catalog.metaDomain.AttachedFunctionDb(txCtx).GetAnyByName(req.InputCollectionId, req.Name) | ||
| results, err := s.catalog.metaDomain.AttachedFunctionDb(txCtx).GetAttachedFunctions(nil, &req.Name, &req.InputCollectionId, false) |
There was a problem hiding this comment.
Two UUID params here, can we strongly type?
| optional string id = 1; | ||
| optional string name = 2; | ||
| optional string input_collection_id = 3; | ||
| optional bool only_ready = 4; |
There was a problem hiding this comment.
Default behavior should be we get ready things.
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
tanujnay112
force-pushed
the
graphite-base/5884
branch
from
4d9d369 to
b45774b
Compare
tanujnay112
force-pushed
the
consolidate_getters
branch
from
6cfa1f5 to
965e4f1
Compare
| collection_id: CollectionUuid, | ||
| ) -> Result<Vec<chroma_proto::AttachedFunction>, ListAttachedFunctionsError> { | ||
| options: crate::GetAttachedFunctionsOptions, | ||
| ) -> Result<Vec<chroma_types::AttachedFunction>, ListAttachedFunctionsError> { |
There was a problem hiding this comment.
[BestPractice]
The new get_attached_functions function returns a ListAttachedFunctionsError. To fully align with the refactoring and the new function name, it would be more consistent to rename ListAttachedFunctionsError to GetAttachedFunctionsError.
This might require changes in the chroma_types crate, so it could be addressed in this PR or a follow-up.
Context for Agents
The new `get_attached_functions` function returns a `ListAttachedFunctionsError`. To fully align with the refactoring and the new function name, it would be more consistent to rename `ListAttachedFunctionsError` to `GetAttachedFunctionsError`.
This might require changes in the `chroma_types` crate, so it could be addressed in this PR or a follow-up.
File: rust/sysdb/src/sysdb.rs
Line: 463
This comment has been minimized.
This comment has been minimized.
tanujnay112
force-pushed
the
graphite-base/5884
branch
from
b45774b to
fa6f167
Compare
tanujnay112
force-pushed
the
consolidate_getters
branch
from
965e4f1 to
43b8144
Compare
tanujnay112
force-pushed
the
consolidate_getters
branch
from
43b8144 to
97cbe8d
Compare
| log.Info("GetAttachedFunctions", | ||
| zap.Stringp("id", req.Id), | ||
| zap.Stringp("name", req.Name), | ||
| zap.Stringp("input_collection_id", req.InputCollectionId)) |
There was a problem hiding this comment.
[BestPractice]
For better observability, it would be helpful to include the only_ready filter in this log message as well.
| log.Info("GetAttachedFunctions", | |
| zap.Stringp("id", req.Id), | |
| zap.Stringp("name", req.Name), | |
| zap.Stringp("input_collection_id", req.InputCollectionId)) | |
| log.Info("GetAttachedFunctions", | |
| zap.Stringp("id", req.Id), | |
| zap.Stringp("name", req.Name), | |
| zap.Stringp("input_collection_id", req.InputCollectionId), | |
| zap.Boolp("only_ready", req.OnlyReady)) |
Context for Agents
For better observability, it would be helpful to include the `only_ready` filter in this log message as well.
```suggestion
log.Info("GetAttachedFunctions",
zap.Stringp("id", req.Id),
zap.Stringp("name", req.Name),
zap.Stringp("input_collection_id", req.InputCollectionId),
zap.Boolp("only_ready", req.OnlyReady))
```
File: go/pkg/sysdb/grpc/task_service.go
Line: 32| .input_collection_id | ||
| .map(|id| id.0.to_string()) | ||
| .unwrap_or_else(|| "unknown".to_string()); | ||
| ListAttachedFunctionsError::NotFound(id_str) |
There was a problem hiding this comment.
[BestPractice]
The NotFound error message is based only on input_collection_id, which might be confusing if the query was filtered by other criteria like id or name. We could construct a more descriptive error message based on the filters provided in options.
| .input_collection_id | |
| .map(|id| id.0.to_string()) | |
| .unwrap_or_else(|| "unknown".to_string()); | |
| ListAttachedFunctionsError::NotFound(id_str) | |
| let not_found_msg = if let Some(id) = options.id { | |
| format!("id: {}", id) | |
| } else if let Some(name) = &options.name { | |
| format!("name: '{}'", name) | |
| } else if let Some(coll_id) = options.input_collection_id { | |
| format!("collection_id: {}", coll_id) | |
| } else { | |
| "(no filters provided)".to_string() | |
| }; | |
| ListAttachedFunctionsError::NotFound(not_found_msg) |
Context for Agents
The `NotFound` error message is based only on `input_collection_id`, which might be confusing if the query was filtered by other criteria like `id` or `name`. We could construct a more descriptive error message based on the filters provided in `options`.
```suggestion
let not_found_msg = if let Some(id) = options.id {
format!("id: {}", id)
} else if let Some(name) = &options.name {
format!("name: '{}'", name)
} else if let Some(coll_id) = options.input_collection_id {
format!("collection_id: {}", coll_id)
} else {
"(no filters provided)".to_string()
};
ListAttachedFunctionsError::NotFound(not_found_msg)
```
File: rust/sysdb/src/sysdb.rs
Line: 1328|
Found 1 test failure on Blacksmith runners: Failure
|
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the _docs section?_