get_by_name() to return Result<Branch, OxenError> instead of Result<Option<Branch>, OxenError>#407
get_by_name() to return Result<Branch, OxenError> instead of Result<Option<Branch>, OxenError>#407
get_by_name() to return Result<Branch, OxenError> instead of Result<Option<Branch>, OxenError>#407Conversation
…`Result<Option<Branch>, OxenError>`
📝 WalkthroughWalkthroughRefactored branch lookup semantics: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Comment |
CleanCut
left a comment
CleanCut
left a comment
There was a problem hiding this comment.
get_by_name is a good improvement!
The old form (and the current get_by_name_maybe) provides no value. Anything that uses get_by_name_maybe should either change to use get_by_name to be simpler, or use with_ref_manager directly otherwise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/lib/src/core/v_latest/commits.rs`:
- Around line 132-135: In get_commit_by_branch, don't convert Result errors to
None via .ok().flatten(); instead preserve and propagate errors from
repositories::branches::get_by_name_maybe and get_by_id. Replace the chained
.ok().flatten() calls with explicit error handling: call
repositories::branches::get_by_name_maybe(repo, branch_name)? to get a
Option<branch>, return Ok(None) if no branch, and if Some(branch) call
get_by_id(repo, &branch.commit_id)? and wrap that result in Ok(Some(commit));
this keeps errors from get_by_name_maybe and get_by_id (and the function
signature of get_commit_by_branch) intact.
In `@crates/server/src/controllers/branches.rs`:
- Around line 183-184: The code currently converts a missing from_name into a
generic OxenHttpError::NotFound; instead preserve the branch-specific 404 by
mapping None to remote_branch_not_found(repo, &data.from_name). Replace the
.ok_or(OxenHttpError::NotFound)? on the
repositories::branches::get_by_name_maybe(...) result with
.ok_or(remote_branch_not_found(repo, &data.from_name))? (or ok_or_else with that
call) so the handler returns the same remote_branch_not_found(...) error payload
used by other branch endpoints.
🪄 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: Pro
Run ID: 867baebf-cfc3-4ae3-81cd-b603a4db5f6f
📒 Files selected for processing (17)
crates/lib/src/api/client/repositories.rscrates/lib/src/core/v_latest/branches.rscrates/lib/src/core/v_latest/commits.rscrates/lib/src/core/v_latest/merge.rscrates/lib/src/core/v_latest/push.rscrates/lib/src/core/v_latest/workspaces/commit.rscrates/lib/src/repositories/branches.rscrates/lib/src/repositories/checkout.rscrates/lib/src/repositories/entries.rscrates/lib/src/repositories/load.rscrates/lib/src/repositories/merge.rscrates/lib/src/repositories/revisions.rscrates/oxen-py/src/py_repo.rscrates/server/src/controllers/branches.rscrates/server/src/controllers/workspaces.rscrates/server/src/controllers/workspaces/data_frames.rscrates/server/src/params.rs
💤 Files with no reviewable changes (1)
- crates/lib/src/repositories/revisions.rs
| repositories::branches::get_by_name_maybe(repo, branch_name) | ||
| .ok() | ||
| .flatten() | ||
| .and_then(|branch| get_by_id(repo, &branch.commit_id).ok().flatten()) |
There was a problem hiding this comment.
Do not swallow lookup/storage errors in get_commit_by_branch.
Using .ok().flatten() converts real errors into None, which can misreport backend/ref failures as “Commit not found”.
Proposed fix (preserve error context)
-fn get_commit_by_ref<S: AsRef<str> + Clone>(
+fn get_commit_by_ref<S: AsRef<str> + Clone>(
repo: &LocalRepository,
ref_name: S,
) -> Result<Commit, OxenError> {
- get_by_id(repo, ref_name.clone())?
- .or_else(|| get_commit_by_branch(repo, ref_name.as_ref()))
- .ok_or_else(|| OxenError::basic_str("Commit not found"))
+ if let Some(commit) = get_by_id(repo, ref_name.clone())? {
+ return Ok(commit);
+ }
+ if let Some(commit) = get_commit_by_branch(repo, ref_name.as_ref())? {
+ return Ok(commit);
+ }
+ Err(OxenError::basic_str("Commit not found"))
}
-fn get_commit_by_branch(repo: &LocalRepository, branch_name: &str) -> Option<Commit> {
- repositories::branches::get_by_name_maybe(repo, branch_name)
- .ok()
- .flatten()
- .and_then(|branch| get_by_id(repo, &branch.commit_id).ok().flatten())
+fn get_commit_by_branch(
+ repo: &LocalRepository,
+ branch_name: &str,
+) -> Result<Option<Commit>, OxenError> {
+ let Some(branch) = repositories::branches::get_by_name_maybe(repo, branch_name)? else {
+ return Ok(None);
+ };
+ get_by_id(repo, &branch.commit_id)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lib/src/core/v_latest/commits.rs` around lines 132 - 135, In
get_commit_by_branch, don't convert Result errors to None via .ok().flatten();
instead preserve and propagate errors from
repositories::branches::get_by_name_maybe and get_by_id. Replace the chained
.ok().flatten() calls with explicit error handling: call
repositories::branches::get_by_name_maybe(repo, branch_name)? to get a
Option<branch>, return Ok(None) if no branch, and if Some(branch) call
get_by_id(repo, &branch.commit_id)? and wrap that result in Ok(Some(commit));
this keeps errors from get_by_name_maybe and get_by_id (and the function
signature of get_commit_by_branch) intact.
| let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)? | ||
| .ok_or(OxenHttpError::NotFound)?; |
There was a problem hiding this comment.
Preserve the branch-specific 404 here.
from_name now falls back to a generic OxenHttpError::NotFound, while the other branch endpoints in this controller return remote_branch_not_found(...) for the same missing-branch condition. That makes POST /branches emit a different error payload/message than the rest of the branch API.
Suggested fix
- let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)?
- .ok_or(OxenHttpError::NotFound)?;
+ let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)?
+ .ok_or_else(|| OxenError::remote_branch_not_found(&data.from_name))?;📝 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.
| let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)? | |
| .ok_or(OxenHttpError::NotFound)?; | |
| let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)? | |
| .ok_or_else(|| OxenError::remote_branch_not_found(&data.from_name))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/server/src/controllers/branches.rs` around lines 183 - 184, The code
currently converts a missing from_name into a generic OxenHttpError::NotFound;
instead preserve the branch-specific 404 by mapping None to
remote_branch_not_found(repo, &data.from_name). Replace the
.ok_or(OxenHttpError::NotFound)? on the
repositories::branches::get_by_name_maybe(...) result with
.ok_or(remote_branch_not_found(repo, &data.from_name))? (or ok_or_else with that
call) so the handler returns the same remote_branch_not_found(...) error payload
used by other branch endpoints.
|
I suggest just removing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/lib/src/repositories/branches.rs`:
- Around line 56-58: The current exists(repo, name) function swallows all errors
from get_by_name(repo, name).is_ok(), treating I/O/db failures the same as
BranchNotFound; fix by either (A) changing exists signature to return
Result<bool, OxenError> and propagate get_by_name(repo, name)? so callers (e.g.,
the checkout logic that calls exists) can handle real errors, or (B) keep the
bool signature but explicitly match the error from get_by_name and return false
only for the BranchNotFound variant while propagating or returning an error for
other cases (i.e., call get_by_name and match Err(BranchNotFound) => false,
Err(other) => propagate/log/return error). Update the caller in checkout to use
the new Result<bool, OxenError> or handle the propagated error accordingly.
🪄 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: Pro
Run ID: 95907e74-a290-41fe-9ac0-31b7c4813595
📒 Files selected for processing (10)
crates/lib/src/core/v_latest/commits.rscrates/lib/src/core/v_latest/workspaces/commit.rscrates/lib/src/repositories/branches.rscrates/lib/src/repositories/checkout.rscrates/lib/src/repositories/entries.rscrates/lib/src/repositories/revisions.rscrates/server/src/controllers/branches.rscrates/server/src/controllers/workspaces.rscrates/server/src/controllers/workspaces/data_frames.rscrates/server/src/params.rs
💤 Files with no reviewable changes (1)
- crates/lib/src/core/v_latest/commits.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/lib/src/repositories/entries.rs
- crates/lib/src/repositories/checkout.rs
- crates/lib/src/core/v_latest/workspaces/commit.rs
- crates/lib/src/repositories/revisions.rs
- crates/server/src/controllers/workspaces/data_frames.rs
- crates/server/src/controllers/branches.rs
| pub fn exists(repo: &LocalRepository, name: &str) -> bool { | ||
| get_by_name(repo, name).is_ok() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check usages of branches::exists to see if callers handle errors
rg -n -C3 'branches::exists'Repository: Oxen-AI/Oxen
Length of output: 730
🏁 Script executed:
# Find get_by_name implementation
rg -A15 'fn get_by_name' crates/lib/src/repositories/branches.rs | head -40Repository: Oxen-AI/Oxen
Length of output: 922
🏁 Script executed:
# Also check OxenError types to understand BranchNotFound
rg -B2 -A2 'BranchNotFound' crates/lib/src/ --type rust | head -30Repository: Oxen-AI/Oxen
Length of output: 1971
exists() silently treats lookup errors as "not found".
The implementation returns false for any error from get_by_name(), including I/O or database failures, not just BranchNotFound. While this matches the single usage pattern (as a simple boolean guard in checkout.rs:23), it does mask underlying errors.
However, the current design appears intentional: the API is a simple existence check, and callers that need the actual branch follow up with get_by_name()? for error handling. The suggested alternative of returning Result<bool, OxenError> would require updating the existing caller's usage pattern. If preserving error distinction is important, consider documenting the error-suppression behavior or slightly refactoring the checkout logic to propagate lookup errors more explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lib/src/repositories/branches.rs` around lines 56 - 58, The current
exists(repo, name) function swallows all errors from get_by_name(repo,
name).is_ok(), treating I/O/db failures the same as BranchNotFound; fix by
either (A) changing exists signature to return Result<bool, OxenError> and
propagate get_by_name(repo, name)? so callers (e.g., the checkout logic that
calls exists) can handle real errors, or (B) keep the bool signature but
explicitly match the error from get_by_name and return false only for the
BranchNotFound variant while propagating or returning an error for other cases
(i.e., call get_by_name and match Err(BranchNotFound) => false, Err(other) =>
propagate/log/return error). Update the caller in checkout to use the new
Result<bool, OxenError> or handle the propagated error accordingly.
| Some(_) => Ok(true), | ||
| None => Ok(false), | ||
| } | ||
| pub fn exists(repo: &LocalRepository, name: &str) -> bool { |
There was a problem hiding this comment.
I could also see this being:
match get_by_name(repo,name) {
Ok(_) => Ok(true),
Err(OxenError::BranchNotFound(_)) => Ok(false),
error => error,
}
And keeping it as Result<bool, OxenError> and adding the following to the docstring:
/// Check if a branch exists. Returns Ok(false) if the branch doesn't exist. Returns Err() if there was some other problem accessing the local repository.
But a-ok to keep it as -> bool too. I guess if there's some other error we will encounter that somewhere else?
3 participants
There was a lot of boiler plate that we were handling None to return
OxenError::repo_not_found()this PR simplifies that.Important change is this:
https://github.com/Oxen-AI/Oxen/pull/407/changes#diff-36d674fb2bcad4130c6b6fff3f561bbcbfe2a28ed81a632f375c2ae7b9dbf442R26