Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
FileNode schema & permission model (roadmap 3.1.1) — documentation, migrations, repo helpers and testsThis PR adds the execution plan docs/execplans/3-1-1-file-node-schema-and-permission-model.md and implements the planned, additive transition from the legacy flat files/file_acl model to a hierarchical FileNode schema with a normalised permission model (SQLite and PostgreSQL). The change set is documentation-first: it includes schema migrations, Diesel schema/models, repository helpers, test scaffolding and developer/user docs to gate and guide a staged implementation; no behavioural protocol changes beyond repository-query surface shifts are included. Key points
Scope and intent
WalkthroughSummarise migration to a hierarchical Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Client
participant Handler
participant Database
participant Cache
end
Client->>Handler: Request file list
Handler->>Database: Query visible root file_nodes for user
Database->>Database: Resolve resource_permissions join (file_nodes, permissions)
Database->>Database: Filter by permission code (download/list = 2)
Database->>Database: Include direct user and group principals
alt Legacy rows present
Database->>Database: Query legacy `files` + `file_acl`
Database->>Database: Map legacy rows to VisibleFileNode
end
Database->>Database: Merge results, dedupe preferring file_nodes
Database-->>Handler: Return unified list
Handler->>Cache: Cache permission decisions
Handler-->>Client: Respond with file name list
sequenceDiagram
rect rgba(255,245,220,0.5)
participant Admin
participant AppConfig
participant MigrationRunner
participant Watchdog
participant Database
end
Admin->>AppConfig: Load (migration_timeout_secs)
Admin->>MigrationRunner: run_migrations(timeout_secs)
MigrationRunner->>Watchdog: Spawn watchdog (sleep for duration)
par Migrate vs Watchdog
MigrationRunner->>Database: Execute migrations
and
Watchdog->>MigrationRunner: Trigger cancel if timeout reached
end
alt Completed before timeout
Database-->>MigrationRunner: Migrations done
MigrationRunner-->>Admin: Success
else Timeout fired
Watchdog->>MigrationRunner: Cancel via token
MigrationRunner->>Database: Await in‑flight work, then return timeout error
MigrationRunner-->>Admin: Return MigrationTimeout error
end
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 6 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Contributor
Reviewer's GuideIntroduces a hierarchical FileNode-based file/ACL schema shared across SQLite and PostgreSQL, wires it into Diesel models and db helpers, migrates tests and fixtures to use the new structures, and documents the final permission architecture and roadmap completion while keeping legacy tables for backfill. Sequence diagram for GetFileNameList using FileNode permissionssequenceDiagram
actor Client
participant CommandHandler
participant DbPool
participant DbConnection
participant FilesRepository
Client->>CommandHandler: send GetFileNameList
CommandHandler->>DbPool: get()
DbPool-->>CommandHandler: DbConnection
CommandHandler->>DbConnection: list_visible_root_file_nodes_for_user(user_id)
DbConnection->>FilesRepository: join file_nodes, resource_permissions, permissions, user_groups
FilesRepository-->>DbConnection: Vec<VisibleFileNode>
DbConnection-->>CommandHandler: visible roots
CommandHandler-->>Client: FileNameList response
ER diagram for new FileNode and shared permission schemaerDiagram
User {
int id
varchar username
varchar email
boolean active
}
Group {
int id
varchar name
}
UserGroup {
int user_id
int group_id
}
Permission {
int id
int code
varchar name
varchar description
}
UserPermission {
int user_id
int permission_id
}
FileNode {
int id
enum kind
varchar name
int parent_id
int alias_target_id
varchar object_key
bigint size
text comment
boolean is_dropbox
int creator_id
timestamptz created_at
timestamptz updated_at
}
ResourcePermission {
varchar resource_type
int resource_id
varchar principal_type
int principal_id
int permission_id
}
%% Membership
User ||--o{ UserGroup : member
Group ||--o{ UserGroup : membership
%% Global privileges
Permission ||--o{ UserPermission : grants
User ||--o{ UserPermission : holds
%% File hierarchy
FileNode ||--o| FileNode : parent
FileNode ||--o| FileNode : alias_target
User ||--o{ FileNode : creates
%% Resource ACLs
Permission ||--o{ ResourcePermission : grants
FileNode ||--o{ ResourcePermission : protected_resource
User ||--o{ ResourcePermission : principal_user
Group ||--o{ ResourcePermission : principal_group
Class diagram for FileNode models and DB helpersclassDiagram
direction LR
class FileNodeKind {
<<enum>>
+File
+Folder
+Alias
+as_str() str
}
class Permission {
+i32 id
+i32 code
+String name
+String description
}
class NewPermission {
+i32 code
+&str name
+&str description
}
class Group {
+i32 id
+String name
}
class NewGroup {
+&str name
}
class NewUserGroup {
+i32 user_id
+i32 group_id
}
class NewUserPermission {
+i32 user_id
+i32 permission_id
}
class FileNode {
+i32 id
+String kind
+String name
+Option~i32~ parent_id
+Option~i32~ alias_target_id
+Option~String~ object_key
+Option~i64~ size
+Option~String~ comment
+bool is_dropbox
+i32 creator_id
+NaiveDateTime created_at
+NaiveDateTime updated_at
}
class NewFileNode {
+&str kind
+&str name
+Option~i32~ parent_id
+Option~i32~ alias_target_id
+Option~&str~ object_key
+Option~i64~ size
+Option~&str~ comment
+bool is_dropbox
+i32 creator_id
}
class VisibleFileNode {
+i32 id
+String name
+String kind
}
class NewResourcePermission {
+&str resource_type
+i32 resource_id
+&str principal_type
+i32 principal_id
+i32 permission_id
}
class FileNodeLookupError {
<<enum>>
+InvalidPath
+Diesel
+Serde
}
class DbConnection {
}
class FilesRepository {
<<module>>
+seed_permission(conn, permission) Result~i32~
+create_group(conn, group) Result~i32~
+add_user_to_group(conn, membership) Result~bool~
+create_file_node(conn, node) Result~i32~
+grant_resource_permission(conn, permission) Result~bool~
+get_file_node(conn, node_id) Result~Option~FileNode~~
+resolve_file_node_path(conn, path) Result~Option~FileNode~~
+list_child_file_nodes(conn, parent_id) Result~Vec~FileNode~~
+resolve_alias_target(conn, node_id) Result~Option~FileNode~~
+list_visible_root_file_nodes_for_user(conn, user_id) Result~Vec~VisibleFileNode~~
}
FileNodeKind --> FileNode : kind_string
Permission <.. NewPermission : builds
Group <.. NewGroup : builds
FileNode <.. NewFileNode : builds
Permission <.. NewResourcePermission : permission_id
DbConnection --> FilesRepository : used_by
FilesRepository --> FileNode : queries
FilesRepository --> VisibleFileNode : projects
FilesRepository --> Permission : seeds
FilesRepository --> Group : seeds
Architecture flow for FileNode outbound adapterflowchart LR
subgraph InboundPort
WF["Wireframe transport
(Hotline transactions)"]
CMD["Command handlers
(GetFileNameList)"]
end
subgraph ApplicationCore
SVC["FileListingService
(port interface)"]
end
subgraph OutboundAdapter
DBMOD["mxd::db::files
(Diesel helpers)"]
SCHEMA["Diesel schema.rs
(FileNode, permissions)"]
end
subgraph Persistence
DB1[(SQLite DB)]
DB2[(PostgreSQL DB)]
end
WF --> CMD
CMD --> SVC
SVC --> DBMOD
DBMOD --> SCHEMA
DBMOD --> DB1
DBMOD --> DB2
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file /// Returns any error produced by the database.
#[must_use = "handle the result"]
pub async fn create_file(
pub async fn seed_permission(❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Code Duplicationsrc/db/files.rs: What lead to degradation?The module contains 4 functions with similar structure: create_file_node,create_file_node,create_group,seed_permission Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on file /// Returns any error produced by the database.
#[must_use = "handle the result"]
pub async fn create_file(
pub async fn seed_permission(❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db7aac141f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
implement-file-node-schema-permissions-23omhv
branch
from
8db4f39 to
2a9bb60
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Explain the Makefile fallback lookup for Cargo, Whitaker, and markdownlint so contributors understand why the build gates do not require every tool location to be preloaded into PATH.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15075
Error checks — all resolved ✅
Warning checks — resolved or properly deferred ✅
Inconclusive check — resolved ✅
No further work is required. |
Route the shared Postgres DB test harness through `PostgresTestDb` when `POSTGRES_TEST_URL` is set. This lets CI coverage jobs use their provisioned database instead of downloading embedded PostgreSQL binaries. Keep the embedded harness as the fallback for local runs without an external test database.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Expand the first CTE reference and update the `InvalidPath` description so the developer guide matches the file-node path handling contract.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Add backend-agnostic file-node scenarios for missing paths, permission filtering, nested child visibility, and kind-specific constraints. Instrument file visibility, path resolution, permission grants, and migration watchdog cancellation. Split the legacy visibility query into a small helper module so the repository module stays within the line budget.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/db/tests/file_node_tests/additional.rs Comment on lines +365 to +389 async fn assert_alias_without_target_rejected(
conn: &mut DbConnection,
creator_id: i32,
) -> Result<(), AnyError> {
let alias_without_target = create_file_node(
conn,
&NewFileNode {
kind: FileNodeKind::Alias.as_str(),
name: "orphan-alias",
parent_id: None,
alias_target_id: None,
object_key: None,
size: None,
comment: None,
is_dropbox: false,
creator_id,
},
)
.await;
anyhow::ensure!(
alias_without_target.is_err(),
"alias without alias_target_id should be rejected"
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Move the new backend-agnostic file-node scenario bodies into the shared test module and expose the requested SQLite wrappers from the parent DB test module. Split the older shared scenarios into a small core helper module and move SQLite-only wrappers into the SQLite helper module so all touched Rust files stay within the line budget while preserving Postgres coverage.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document the legacy visibility helper contract and bound watchdog cancellation so timed-out migrations cannot stall indefinitely while waiting for non-cooperative work to finish.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace the redundant timeout result match with a direct discard binding while preserving the bounded grace wait after migration cancellation.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/migrations.rs (1)
209-227: 🛠️ Refactor suggestion | 🟠 MajorDocument the new timeout parameter and failure mode.
Update the Rustdoc for
run_migrationsandapply_migrations. The docs still describe only generic Diesel failures, but these APIs now accepttimeout_secsand can return a wrapped watchdog timeout after cancellation. As per coding guidelines: "Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc" and "Public functions must be fully documented with all sections required by the relevant documentation standard".Also applies to: 247-268, 273-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/migrations.rs` around lines 209 - 227, The Rustdoc for run_migrations (and apply_migrations) is out of date: update the /// comments to document the new timeout_secs parameter and the new failure mode when the watchdog cancels the operation; explicitly describe the timeout_secs argument (Option<u64> in seconds), how the function behaves when a timeout occurs (returns a wrapped timeout/cancellation error in the QueryResult), and note that Diesel errors may still be returned; ensure the doc includes a Parameters section for timeout_secs and an Errors section describing both Diesel QueryResult errors and the watchdog timeout/cancellation error for run_migrations and apply_migrations.
🤖 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/db/migrations.rs`:
- Around line 302-303: Add a module-level doc comment for the test module by
placing a //! comment immediately above the `mod tests {` declaration (the
`tests` module) that briefly describes the module's purpose and scope (what
kinds of tests live here and why), matching the project's module-doc convention.
- Around line 306-319: Collapse the three tests that call migration_timeout into
a single parameterized rstest: replace
migration_timeout_uses_default_when_config_value_is_missing,
migration_timeout_uses_default_when_config_value_is_zero, and
migration_timeout_accepts_positive_config_override with one #[rstest] that
supplies the input/config values (None, Some(0), Some(7)) and the expected
Duration results (DEFAULT_MIGRATION_TIMEOUT, DEFAULT_MIGRATION_TIMEOUT,
Duration::from_secs(7)), then assert_eq!(migration_timeout(input), expected);
add the rstest attribute (use rstest::rstest) and a clear test name to keep
coverage while removing duplication and relying on the migration_timeout
function, DEFAULT_MIGRATION_TIMEOUT symbol and Duration::from_secs for
expectations.
---
Outside diff comments:
In `@src/db/migrations.rs`:
- Around line 209-227: The Rustdoc for run_migrations (and apply_migrations) is
out of date: update the /// comments to document the new timeout_secs parameter
and the new failure mode when the watchdog cancels the operation; explicitly
describe the timeout_secs argument (Option<u64> in seconds), how the function
behaves when a timeout occurs (returns a wrapped timeout/cancellation error in
the QueryResult), and note that Diesel errors may still be returned; ensure the
doc includes a Parameters section for timeout_secs and an Errors section
describing both Diesel QueryResult errors and the watchdog timeout/cancellation
error for run_migrations and apply_migrations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8a19144-4bc5-49e1-a6ed-7a6efcf23833
📒 Files selected for processing (1)
src/db/migrations.rs
Update migration Rustdoc to describe the timeout parameter and watchdog failure mode, and collapse the migration timeout unit checks into one parameterized rstest case.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9560
No further work is required. |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
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
What changed in this PR
Why this is important
Plan & milestones (Stages A–H)
Validation gates & success criteria
Testing plan (high level)
Risks & mitigations
Documentation touched
How to review
Next steps
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/7d2b3e56-da4e-44e8-9730-e922b927f924
Summary by Sourcery
Introduce a hierarchical FileNode schema and shared permission model, wire the existing file-list path to the new adapter, and document completion of roadmap item 3.1.1 while keeping legacy tables for later backfill.
New Features:
Enhancements:
Build:
Documentation:
Tests: