add optional metadata to the workspace object - #455
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds pagination support to workspace listing across the CLI, API client, server controllers, and Python bindings, while introducing workspace metadata management. Changes include new metadata fields in data models, a new server endpoint for updating metadata, updates to error handling for workspace conflicts, and enhancements to workspace name index logic with backfilling capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| @@ -53,25 +49,44 @@ pub async fn get_by_name( | |||
| name: impl AsRef<str>, | |||
| ) -> Result<Option<WorkspaceResponse>, OxenError> { | |||
| let name = name.as_ref(); | |||
| let url = api::endpoint::url_from_repo(remote_repo, &format!("/workspaces?name={name}"))?; | |||
| let response = list_with_params(remote_repo, None, Some(name)).await?; | |||
| match response.entries.len() { | |||
There was a problem hiding this comment.
are we sure names are unique?
There was a problem hiding this comment.
We want them to be..but yeah let me double check
|
👀 |
Does this have anything to do with the metadata? I'm super confused. I don't understand what the metadata is for. I don't understand the design for the workspaces, or how they're used on the server vs the client. From what I can tell on the server a workspace is just supposed to be a staging area for composing a remote commit...so why would there ever be a need to have multiple pages of them? Why do we need to store metadata on them? Help me out, here. 🏳️ |
Yes let's do a workspace deep dive! It is a different concept than lives in most VCS so want to get your brain on it too. |
4e7cf10 to
35a35f4
Compare
|
Other branch was merged. Rebased this against main. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
oxen-python/python/oxen/remote_repo.py (1)
548-559: Consider adding a workspace iterator helper to avoid partial-list surprises.With Line 548 now page-scoped, many callers will still expect “all workspaces.” A
scan_workspaces(page_size=...)helper (like yourscan()pattern) would preserve ergonomics without changing this paginated API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxen-python/python/oxen/remote_repo.py` around lines 548 - 559, The current list_workspaces method returns a single page via _repo.list_workspaces(page_num, page_size), which can surprise callers expecting all workspaces; add a scan_workspaces(self, page_size=100) generator that iterates pages using _repo.list_workspaces and yields individual workspace items (mimic the existing scan() pattern), handling pagination until no more results and exposing page_size as the only argument so callers can easily iterate all workspaces without manually managing page_num.
🤖 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/cli/src/cmd/workspace/list.rs`:
- Around line 69-72: The current early-return uses paginated.entries.is_empty()
which is true for out-of-range pages; instead check
paginated.pagination.total_entries == 0 to decide the global "No workspaces
found" message. Update the logic around the call to
api::client::workspaces::list so that if paginated.pagination.total_entries == 0
you print "No workspaces found" and return Ok(()). If total_entries > 0 but
paginated.entries.is_empty(), do not print the empty-state message — render the
page metadata (pagination info) and show an empty page result instead. Keep
references to paginated, paginated.entries.is_empty(), and
paginated.pagination.total_entries when making the change.
In `@crates/lib/src/api/client/workspaces.rs`:
- Around line 67-79: The query string builder pushes raw user input for the name
filter (query_params and uri construction), which breaks on reserved characters;
change the branch that does format!("name={name}") to percent-encode the name
before appending it to query_params (e.g., use a URL-encoding helper such as
url::form_urlencoded or percent_encoding::utf8_percent_encode) so get_by_name /
the name variable is safely encoded, then join query_params and build uri as
before.
In `@crates/lib/src/repositories/workspaces.rs`:
- Around line 531-567: The workspace metadata can contain nested nulls that
toml::to_string cannot serialize; update handling so metadata is stored as raw
JSON instead of passing a serde_json::Value through TOML. Change
WorkspaceConfig.metadata to an Option<String> (JSON string), update
update_metadata to convert the incoming Option<serde_json::Value> to
Option<String> via serde_json::to_string(), update read_workspace_config to
parse the TOML metadata string back to serde_json::Value where needed (or leave
as string), and update write_workspace_config to write the JSON string into the
config TOML (ensuring toml::to_string succeeds). Adjust any callers that expect
WorkspaceConfig.metadata to use the new string representation.
In `@crates/lib/src/view/workspaces.rs`:
- Around line 47-48: The OpenAPI schema attribute on the serde_json::Value
fields is too restrictive—update the schema attribute for the metadata fields
(the struct fields named `metadata` typed as `Option<Value>`) by replacing
`#[schema(value_type = Object, nullable = true)]` with `#[schema(value_type =
Value, nullable = true)]` (apply the same change to both occurrences in this
file) so the generated schema accepts arbitrary JSON values rather than only
objects.
In `@crates/oxen-py/src/py_remote_repo.rs`:
- Around line 121-132: In list_workspaces, validate the pagination args page_num
and page_size at the Python boundary (before creating PaginateOpts) and reject
non-positive values by returning a PyValueError instead of proceeding;
specifically, check if page_num == 0 || page_size == 0 and return an Err
wrapping pyo3::exceptions::PyValueError with a clear message so invalid
pagination never reaches api::client::workspaces::list. Use the function name
list_workspaces and variables page_num/page_size to locate where to add the
guard.
In `@crates/server/src/params/workspace_list_query.rs`:
- Around line 5-8: The WorkspaceListQuery struct allows page and page_size to be
zero which breaks pagination; update validation so these fields reject zero at
extraction time by changing their types to Option<NonZeroUsize> (or add explicit
validation in the extractor) and update all usages to map with
.map(NonZeroUsize::get) before applying unwrap_or/defaults; ensure any
controllers or functions referencing WorkspaceListQuery::page and
WorkspaceListQuery::page_size are adjusted to handle Option<NonZeroUsize> and
fall back to defaults only when None.
---
Nitpick comments:
In `@oxen-python/python/oxen/remote_repo.py`:
- Around line 548-559: The current list_workspaces method returns a single page
via _repo.list_workspaces(page_num, page_size), which can surprise callers
expecting all workspaces; add a scan_workspaces(self, page_size=100) generator
that iterates pages using _repo.list_workspaces and yields individual workspace
items (mimic the existing scan() pattern), handling pagination until no more
results and exposing page_size as the only argument so callers can easily
iterate all workspaces without manually managing page_num.
🪄 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: dc157dfc-e79f-4434-8461-181a7a571aa3
📒 Files selected for processing (14)
crates/cli/src/cmd/workspace/list.rscrates/lib/src/api/client/workspaces.rscrates/lib/src/error.rscrates/lib/src/model/workspace.rscrates/lib/src/repositories/workspaces.rscrates/lib/src/view/workspaces.rscrates/oxen-py/src/py_remote_repo.rscrates/server/src/controllers/workspaces.rscrates/server/src/errors.rscrates/server/src/main.rscrates/server/src/params.rscrates/server/src/params/workspace_list_query.rscrates/server/src/services/workspaces.rsoxen-python/python/oxen/remote_repo.py
| let paginated = api::client::workspaces::list(&remote_repo, &page_opts).await?; | ||
| if paginated.entries.is_empty() { | ||
| println!("No workspaces found"); | ||
| return Ok(()); |
There was a problem hiding this comment.
Don't report an out-of-range page as "no workspaces".
paginated.entries.is_empty() is also true when the repo has workspaces but the caller asks for a page past total_pages. In that case this prints a misleading empty-state message instead of the pagination context. Check paginated.pagination.total_entries == 0 before printing No workspaces found and still render the page metadata for empty pages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cli/src/cmd/workspace/list.rs` around lines 69 - 72, The current
early-return uses paginated.entries.is_empty() which is true for out-of-range
pages; instead check paginated.pagination.total_entries == 0 to decide the
global "No workspaces found" message. Update the logic around the call to
api::client::workspaces::list so that if paginated.pagination.total_entries == 0
you print "No workspaces found" and return Ok(()). If total_entries > 0 but
paginated.entries.is_empty(), do not print the empty-state message — render the
page metadata (pagination info) and show an empty page result instead. Keep
references to paginated, paginated.entries.is_empty(), and
paginated.pagination.total_entries when making the change.
| let mut query_params = Vec::new(); | ||
| if let Some(page_opts) = page_opts { | ||
| query_params.push(format!("page={}", page_opts.page_num)); | ||
| query_params.push(format!("page_size={}", page_opts.page_size)); | ||
| } | ||
| if let Some(name) = name { | ||
| query_params.push(format!("name={name}")); | ||
| } | ||
|
|
||
| let mut uri = "/workspaces".to_string(); | ||
| if !query_params.is_empty() { | ||
| uri.push('?'); | ||
| uri.push_str(&query_params.join("&")); |
There was a problem hiding this comment.
URL-encode the name filter before building the query string.
format!("name={name}") breaks as soon as a workspace name contains reserved characters like spaces, &, or =. Since names are user-provided, get_by_name("foo&bar") will send the wrong query here. Build the query string with percent-encoding instead of manual concatenation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lib/src/api/client/workspaces.rs` around lines 67 - 79, The query
string builder pushes raw user input for the name filter (query_params and uri
construction), which breaks on reserved characters; change the branch that does
format!("name={name}") to percent-encode the name before appending it to
query_params (e.g., use a URL-encoding helper such as url::form_urlencoded or
percent_encoding::utf8_percent_encode) so get_by_name / the name variable is
safely encoded, then join query_params and build uri as before.
| pub fn update_metadata( | ||
| workspace: &Workspace, | ||
| metadata: Option<serde_json::Value>, | ||
| ) -> Result<(), OxenError> { | ||
| let config_path = workspace.config_path(); | ||
| let mut config = read_workspace_config(&config_path)?; | ||
|
|
||
| log::debug!("Updating workspace {} metadata", workspace.id); | ||
| config.metadata = metadata; | ||
|
|
||
| write_workspace_config(&config_path, &config)?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn read_workspace_config(config_path: &Path) -> Result<WorkspaceConfig, OxenError> { | ||
| if !config_path.exists() { | ||
| log::error!("Workspace config not found: {config_path:?}"); | ||
| return Err(OxenError::WorkspaceNotFound( | ||
| format!("{config_path:?}").into(), | ||
| )); | ||
| } | ||
|
|
||
| let config_contents = util::fs::read_from_path(config_path)?; | ||
| toml::from_str(&config_contents).map_err(|e| { | ||
| log::error!("Failed to parse workspace config: {config_path:?}, err: {e}"); | ||
| OxenError::basic_str(format!("Failed to parse workspace config: {e}")) | ||
| }) | ||
| } | ||
|
|
||
| fn write_workspace_config(config_path: &Path, config: &WorkspaceConfig) -> Result<(), OxenError> { | ||
| let toml_string = toml::to_string(config).map_err(|e| { | ||
| log::error!("Failed to serialize workspace config to TOML: {config_path:?}, err: {e}"); | ||
| OxenError::basic_str(format!("Failed to serialize workspace config to TOML: {e}")) | ||
| })?; | ||
|
|
||
| util::fs::write_to_path(&config_path, toml_string)?; | ||
|
|
||
| util::fs::write_to_path(config_path, toml_string)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can Rust/TOML serialization losslessly represent arbitrary serde_json::Value payloads, including nested nulls such as {"a": null} or [1, null]?
💡 Result:
No, Rust/TOML serialization cannot losslessly represent arbitrary serde_json::Value payloads, particularly those with nested nulls like {"a": null} or [1, null].
Citations:
- 1: https://docs.rs/toml/latest/toml/enum.Value.html
- 2: https://docs.rs/toml/latest/toml/value/enum.Value.html
- 3: https://docs.rs/serde_json/latest/serde_json/value/enum.Value.html
- 4: Converting between T and Json::Value, Toml::Value, etc. serde-rs/serde#1192
- 5: Deserializing toml::Value serde-rs/json#471
🏁 Script executed:
# Find the WorkspaceConfig struct definition
rg "struct WorkspaceConfig" --type rust -A 10Repository: Oxen-AI/Oxen
Length of output: 737
🏁 Script executed:
# Search for metadata field type in WorkspaceConfig
rg "metadata" crates/lib/src/repositories/workspaces.rs -B 2 -A 2Repository: Oxen-AI/Oxen
Length of output: 4202
🏁 Script executed:
# Look for any validation or test cases related to metadata serialization
rg "metadata" crates/lib/src/ --type rust -l | head -20Repository: Oxen-AI/Oxen
Length of output: 754
🏁 Script executed:
# Get full WorkspaceConfig struct definition with all derives
cat crates/lib/src/model/workspace.rs | grep -A 20 "pub struct WorkspaceConfig"Repository: Oxen-AI/Oxen
Length of output: 824
🏁 Script executed:
# Check if there's validation for metadata at the API boundary
rg "update_metadata" crates/ --type rust -B 5 -A 10 | head -100Repository: Oxen-AI/Oxen
Length of output: 7490
🏁 Script executed:
# Look for any null handling or validation in the metadata handling code
rg "null\|Null" crates/lib/src/repositories/workspaces.rsRepository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Get full controller implementation for update_metadata
rg "pub async fn update_metadata" crates/server/src/controllers/workspaces.rs -A 30Repository: Oxen-AI/Oxen
Length of output: 1304
🏁 Script executed:
# Check the full WorkspaceConfig struct and its derives
cat crates/lib/src/model/workspace.rs | grep -B 5 "pub struct WorkspaceConfig" -A 10Repository: Oxen-AI/Oxen
Length of output: 496
🏁 Script executed:
# Look for any test or code that shows TOML serialization with null values
rg "toml::to_string\|toml::from_str" crates/lib/src/ --type rust -B 5 -A 5Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Get the full write_workspace_config implementation
rg "fn write_workspace_config" crates/lib/src/repositories/workspaces.rs -A 15Repository: Oxen-AI/Oxen
Length of output: 679
🏁 Script executed:
# Create a test to see what happens when we try to serialize a serde_json::Value with nested nulls to TOML
cat > /tmp/test_toml.rs << 'EOF'
// This demonstrates the issue
fn main() {
use serde_json::{json, Value};
// Create JSON with nested null
let json_with_null = json!({
"label": null
});
// Try to serialize to TOML using serde
let result = toml::to_string(&json_with_null);
match result {
Ok(toml_str) => println!("Success: {}", toml_str),
Err(e) => println!("Error: {}", e),
}
}
EOF
# This won't actually run, but shows what we're looking for
cat /tmp/test_toml.rsRepository: Oxen-AI/Oxen
Length of output: 467
🏁 Script executed:
# Look for how serde_json::Value is declared and if there's a custom serializer
rg "impl.*Serialize.*Value" crates/ --type rust | head -20Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Check if there are any existing test cases that try to store JSON with nulls
rg "null\|Null" crates/lib/src/repositories/workspaces.rs -B 3 -A 3Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Check if there's a way to see what version of toml is used
rg "toml" crates/lib/Cargo.tomlRepository: Oxen-AI/Oxen
Length of output: 83
🏁 Script executed:
# Look for how metadata is being used in tests - especially the test case
cat crates/lib/src/repositories/workspaces.rs | grep -A 30 "test_update_workspace_metadata_persists_and_clears"Repository: Oxen-AI/Oxen
Length of output: 1249
🏁 Script executed:
# Check if there's actual error handling or validation that prevents null metadata from being stored
rg "metadata\|null" crates/server/src/controllers/workspaces.rs -B 2 -A 2 | head -50Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Check the workspace Cargo.toml to see exact toml version
cat crates/lib/Cargo.toml | grep -A 2 "^\[dependencies\]" | head -30 | grep -A 50 "toml"Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Look for UpdateWorkspaceMetadataRequest to see what it expects
rg "struct UpdateWorkspaceMetadataRequest\|UpdateWorkspaceMetadataRequest" crates/ --type rust -A 5Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Check if there's any actual test or code that tries to serialize metadata with nested nulls
rg "json!\|serde_json::json" crates/lib/src/repositories/workspaces.rs -B 2 -A 2Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Verify what TOML version is used in workspace root
cat Cargo.toml | grep tomlRepository: Oxen-AI/Oxen
Length of output: 71
🏁 Script executed:
# Search for the workspace root Cargo.toml to confirm the exact toml version specification
find . -name "Cargo.toml" -maxdepth 1 -exec cat {} \;Repository: Oxen-AI/Oxen
Length of output: 5440
🏁 Script executed:
# Check if there's any validation or filtering of metadata before it gets to write_workspace_config
rg "update_metadata\|metadata" crates/server/src/controllers/workspaces.rs -B 5 -A 5Repository: Oxen-AI/Oxen
Length of output: 38
🏁 Script executed:
# Look for any custom serialization logic for the metadata field
rg "metadata" crates/lib/src/model/workspace.rs -B 3 -A 3Repository: Oxen-AI/Oxen
Length of output: 903
🏁 Script executed:
# Check if there's a custom serializer for Value or metadata field
rg "serialize\|Serialize" crates/lib/src/model/workspace.rs -B 2 -A 2Repository: Oxen-AI/Oxen
Length of output: 38
Nested nulls in JSON metadata will fail TOML serialization.
The controller only strips top-level null values before passing to update_metadata(). However, metadata containing nested nulls like {"label": null} or [1, null] will be accepted and then fail at serialization time in write_workspace_config() when toml::to_string() cannot represent them. Either validate JSON payloads at the API boundary to reject non-TOML-representable values, or store metadata as raw JSON instead of serializing through TOML.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lib/src/repositories/workspaces.rs` around lines 531 - 567, The
workspace metadata can contain nested nulls that toml::to_string cannot
serialize; update handling so metadata is stored as raw JSON instead of passing
a serde_json::Value through TOML. Change WorkspaceConfig.metadata to an
Option<String> (JSON string), update update_metadata to convert the incoming
Option<serde_json::Value> to Option<String> via serde_json::to_string(), update
read_workspace_config to parse the TOML metadata string back to
serde_json::Value where needed (or leave as string), and update
write_workspace_config to write the JSON string into the config TOML (ensuring
toml::to_string succeeds). Adjust any callers that expect
WorkspaceConfig.metadata to use the new string representation.
| #[schema(value_type = Object, nullable = true)] | ||
| pub metadata: Option<Value>, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In utoipa/OpenAPI, how should a Rust field typed as serde_json::Value be documented when it can contain arbitrary JSON values rather than only an object?
💡 Result:
To document a Rust field typed as serde_json::Value in utoipa/OpenAPI so it represents arbitrary JSON values (not restricted to an object), use the #[schema(value_type = Value)] attribute on the field within a #[derive(ToSchema)] struct. This overrides the default schema derivation, rendering the field as a generic OpenAPI value with no type restriction (using SchemaType::Value or SchemaType::AnyValue), allowing any valid JSON (primitives, objects, arrays, etc.). Example: use serde_json::Value; use utoipa::ToSchema; #[derive(ToSchema)] struct MyStruct { /// Arbitrary JSON data #[schema(value_type = Value)] data: Value, } Note: serde_json::Value already implements ToSchema natively, but the value_type = Value attribute ensures the field is explicitly documented as accepting any JSON value rather than assuming a specific type like object. For third-party or opaque types serializing to JSON, this attribute is particularly useful as recommended in utoipa documentation and GitHub issues.
Citations:
- 1: Representing generic JSON? juhaku/utoipa#974
- 2: https://www.libelektra.org/ftp/permaplant/doc/mr/cargodoc/utoipa/derive.ToSchema.html
- 3: https://www.libelektra.org/ftp/permaplant/doc/mr/cargodoc/utoipa/openapi/schema/enum.SchemaType.html
- 4: https://docs.rs/utoipa/latest/utoipa/trait.ToSchema.html
- 5: https://docs.rs/utoipa/latest/utoipa/derive.ToSchema.html
🏁 Script executed:
cat -n crates/lib/src/view/workspaces.rs | sed -n '40,100p'Repository: Oxen-AI/Oxen
Length of output: 1920
Change #[schema(value_type = Object)] to #[schema(value_type = Value)] for metadata fields.
Both lines 47 and 94 use #[schema(value_type = Object)] on fields typed as serde_json::Value, which restricts the OpenAPI schema to objects only. Since the Rust type accepts arbitrary JSON (objects, arrays, strings, numbers, booleans, null), the schema should use value_type = Value to accurately reflect what the API accepts. This prevents generated clients and documentation from incorrectly rejecting valid payloads like arrays or primitives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/lib/src/view/workspaces.rs` around lines 47 - 48, The OpenAPI schema
attribute on the serde_json::Value fields is too restrictive—update the schema
attribute for the metadata fields (the struct fields named `metadata` typed as
`Option<Value>`) by replacing `#[schema(value_type = Object, nullable = true)]`
with `#[schema(value_type = Value, nullable = true)]` (apply the same change to
both occurrences in this file) so the generated schema accepts arbitrary JSON
values rather than only objects.
| #[pyo3(signature = (page_num=liboxen::constants::DEFAULT_PAGE_NUM, page_size=liboxen::constants::DEFAULT_PAGE_SIZE))] | ||
| fn list_workspaces( | ||
| &self, | ||
| page_num: usize, | ||
| page_size: usize, | ||
| ) -> Result<Vec<PyWorkspaceResponse>, PyOxenError> { | ||
| let page_opts = PaginateOpts { | ||
| page_num, | ||
| page_size, | ||
| }; | ||
| let paginated = pyo3_async_runtimes::tokio::get_runtime() | ||
| .block_on(async { api::client::workspaces::list(&self.repo, &page_opts).await })?; |
There was a problem hiding this comment.
Reject zero pagination args at the Python boundary.
Line 124 and Line 125 currently allow 0. Add a guard and raise PyValueError for non-positive values so invalid pagination never leaves the binding layer.
Suggested fix
fn list_workspaces(
&self,
page_num: usize,
page_size: usize,
) -> Result<Vec<PyWorkspaceResponse>, PyOxenError> {
+ if page_num == 0 || page_size == 0 {
+ return Err(PyValueError::new_err("page_num and page_size must be >= 1").into());
+ }
+
let page_opts = PaginateOpts {
page_num,
page_size,
};📝 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.
| #[pyo3(signature = (page_num=liboxen::constants::DEFAULT_PAGE_NUM, page_size=liboxen::constants::DEFAULT_PAGE_SIZE))] | |
| fn list_workspaces( | |
| &self, | |
| page_num: usize, | |
| page_size: usize, | |
| ) -> Result<Vec<PyWorkspaceResponse>, PyOxenError> { | |
| let page_opts = PaginateOpts { | |
| page_num, | |
| page_size, | |
| }; | |
| let paginated = pyo3_async_runtimes::tokio::get_runtime() | |
| .block_on(async { api::client::workspaces::list(&self.repo, &page_opts).await })?; | |
| #[pyo3(signature = (page_num=liboxen::constants::DEFAULT_PAGE_NUM, page_size=liboxen::constants::DEFAULT_PAGE_SIZE))] | |
| fn list_workspaces( | |
| &self, | |
| page_num: usize, | |
| page_size: usize, | |
| ) -> Result<Vec<PyWorkspaceResponse>, PyOxenError> { | |
| if page_num == 0 || page_size == 0 { | |
| return Err(PyValueError::new_err("page_num and page_size must be >= 1").into()); | |
| } | |
| let page_opts = PaginateOpts { | |
| page_num, | |
| page_size, | |
| }; | |
| let paginated = pyo3_async_runtimes::tokio::get_runtime() | |
| .block_on(async { api::client::workspaces::list(&self.repo, &page_opts).await })?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/oxen-py/src/py_remote_repo.rs` around lines 121 - 132, In
list_workspaces, validate the pagination args page_num and page_size at the
Python boundary (before creating PaginateOpts) and reject non-positive values by
returning a PyValueError instead of proceeding; specifically, check if page_num
== 0 || page_size == 0 and return an Err wrapping pyo3::exceptions::PyValueError
with a clear message so invalid pagination never reaches
api::client::workspaces::list. Use the function name list_workspaces and
variables page_num/page_size to locate where to add the guard.
| pub struct WorkspaceListQuery { | ||
| pub name: Option<String>, | ||
| pub page: Option<usize>, | ||
| pub page_size: Option<usize>, |
There was a problem hiding this comment.
Validate page / page_size as positive values.
Line 7 and Line 8 allow 0, which bypasses defaults and can break pagination behavior downstream. Use non-zero types (or explicit validation) so invalid values are rejected at extraction time.
Suggested fix
use serde::Deserialize;
+use std::num::NonZeroUsize;
use utoipa::IntoParams;
#[derive(Deserialize, Debug, IntoParams)]
pub struct WorkspaceListQuery {
pub name: Option<String>,
- pub page: Option<usize>,
- pub page_size: Option<usize>,
+ pub page: Option<NonZeroUsize>,
+ pub page_size: Option<NonZeroUsize>,
}// In controller usage (outside this file), convert with `.map(NonZeroUsize::get)`
// before `unwrap_or(...)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/server/src/params/workspace_list_query.rs` around lines 5 - 8, The
WorkspaceListQuery struct allows page and page_size to be zero which breaks
pagination; update validation so these fields reject zero at extraction time by
changing their types to Option<NonZeroUsize> (or add explicit validation in the
extractor) and update all usages to map with .map(NonZeroUsize::get) before
applying unwrap_or/defaults; ensure any controllers or functions referencing
WorkspaceListQuery::page and WorkspaceListQuery::page_size are adjusted to
handle Option<NonZeroUsize> and fall back to defaults only when None.
4 participants
Right now the hub stores workspace metadata in the database, which means it is easy for the hub and server to get out of sync. This adds a construct for arbitrary user provided json to be stored in the workspace object.
It also adds pagination to the workspaces call.