Add DirectoryNameLock model and enhance name conflict handling#71
Open
Creeper19472 wants to merge 5 commits into
Open
Add DirectoryNameLock model and enhance name conflict handling#71Creeper19472 wants to merge 5 commits into
Creeper19472 wants to merge 5 commits into
Conversation
…g for directories and documents
…with nested transaction
…date locking mechanism for directory and document operations
Contributor
Reviewer's GuideIntroduces a DirectoryNameLock model and name-locking workflow so document and directory operations share a single normalized name namespace per parent, centralizes name conflict detection into reusable helpers returning structured NameConflict responses, and updates handlers and tests to enforce non-empty names and correct conflict handling for create, rename, move, and restore operations. Sequence diagram for name-locked document creationsequenceDiagram
actor Client
participant RequestCreateDocumentHandler as CreateDocumentHandler
participant Session
participant NameConflicts as name_conflicts
participant DirectoryNameLock
Client->>CreateDocumentHandler: handle
CreateDocumentHandler->>CreateDocumentHandler: require_object_name(title)
alt invalid title
CreateDocumentHandler-->>Client: conclude_request(400, ..., DOCUMENT_TITLE_REQUIRED)
else valid title
CreateDocumentHandler->>Session: begin
CreateDocumentHandler->>name_conflicts: reserve_name_for_write(session, folder_id, title)
alt lock or name conflict
name_conflicts-->>CreateDocumentHandler: conflict(NameConflict)
CreateDocumentHandler-->>Client: conclude_request(conflict.code, conflict.public_data(), conflict.message)
else no conflict
name_conflicts-->>CreateDocumentHandler: (name_lock, None)
CreateDocumentHandler->>Session: create Document
CreateDocumentHandler->>name_conflicts: release_name_write_lock(session, name_lock)
Session-->>CreateDocumentHandler: commit
CreateDocumentHandler-->>Client: conclude_request(200, ..., SUCCESS)
end
end
Sequence diagram for directory rename with shared name namespacesequenceDiagram
actor Client
participant RequestRenameDirectoryHandler as RenameDirectoryHandler
participant Session
participant NameConflicts as name_conflicts
Client->>RenameDirectoryHandler: handle
RenameDirectoryHandler->>RenameDirectoryHandler: require_object_name(new_name)
alt invalid name
RenameDirectoryHandler-->>Client: conclude_request(400, ..., DIRECTORY_NAME_REQUIRED)
else valid name
RenameDirectoryHandler->>Session: load Folder
RenameDirectoryHandler->>name_conflicts: reserve_name_for_write(session, parent_id, new_name)
alt conflict with Folder or Document
name_conflicts-->>RenameDirectoryHandler: conflict(NameConflict)
RenameDirectoryHandler-->>Client: conclude_request(conflict.code, conflict.public_data(), conflict.message)
else no conflict
name_conflicts-->>RenameDirectoryHandler: (name_lock, None)
RenameDirectoryHandler->>Session: update Folder.name
RenameDirectoryHandler->>name_conflicts: release_name_write_lock(session, name_lock)
Session-->>RenameDirectoryHandler: commit
RenameDirectoryHandler-->>Client: conclude_request(200, ..., DIRECTORY_RENAMED)
end
end
Entity relationship diagram for DirectoryNameLock and shared name namespaceerDiagram
FOLDER {
string id
string parent_id
string name
}
DOCUMENT {
string id
string folder_id
string title
}
DIRECTORYNAMELOCK {
string parent_id
string name
}
FOLDER ||--o{ DOCUMENT : contains
FOLDER ||--o{ FOLDER : contains
DIRECTORYNAMELOCK ||--o| FOLDER : parent_id_ref
DOCUMENT }o--|| DIRECTORYNAMELOCK : shares_name_namespace
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The tests in
TestDocumentDirectoryNameConflictsdirectly opensrc/app.dbwithsqlite3, hardcoding the path and bypassing the existing SQLAlchemy/session setup; consider routing these inserts/deletes through the same DB config/fixtures used by the rest of the test suite to avoid coupling to a specific file path and schema layout. - The new name validation logic introduces several ad‑hoc
"Directory name is required"andDOCUMENT_TITLE_REQUIREDresponses; it may be worth centralizing this validation (e.g., a small helper aroundnormalize_object_name) so the behavior and error messages for empty/whitespace-only names stay consistent across create/rename/move/restore handlers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests in `TestDocumentDirectoryNameConflicts` directly open `src/app.db` with `sqlite3`, hardcoding the path and bypassing the existing SQLAlchemy/session setup; consider routing these inserts/deletes through the same DB config/fixtures used by the rest of the test suite to avoid coupling to a specific file path and schema layout.
- The new name validation logic introduces several ad‑hoc `"Directory name is required"` and `DOCUMENT_TITLE_REQUIRED` responses; it may be worth centralizing this validation (e.g., a small helper around `normalize_object_name`) so the behavior and error messages for empty/whitespace-only names stay consistent across create/rename/move/restore handlers.
## Individual Comments
### Comment 1
<location path="src/include/domains/documents/commands/name_conflicts.py" line_range="91-57" />
<code_context>
- .filter_by(folder_id=folder_id, title=title)
- .first()
- )
existing_folder = (
session.query(Folder)
.with_for_update()
.filter_by(parent_id=folder_id, name=title)
.first()
)
+ existing_docs = (
</code_context>
<issue_to_address>
**issue (bug_risk):** find_name_conflict incorrectly treats non-active / self entities as conflicts, which breaks restore flows and can delete the object being restored
Reusing `reserve_name_for_write` / `find_name_conflict` in the restore handlers changes semantics:
- `existing_folder` no longer filters by `status`, so the folder being restored is treated as a conflicting sibling and can incorrectly yield `DIRECTORY_NAME_DUPLICATE`.
- `existing_docs` also does not filter by status; the non-active doc being restored is included in `inactive_docs`, and the cleanup loop can call `delete_all_revisions` and `session.delete(existing_doc)` on it.
To match the previous behavior and avoid self-conflicts/deleting the target of the restore, `find_name_conflict` should either:
- Filter to `status == EntityStatus.OK` (and potentially only active entities), or
- Take the ID of the entity being restored and explicitly exclude it from conflict queries.
Without this, restore operations can behave incorrectly compared to the prior status-filtered checks.
</issue_to_address>
### Comment 2
<location path="src/include/domains/documents/handlers/documents.py" line_range="304" />
<code_context>
def handle(self, handler: ConnectionHandler):
folder_id = handler.data.get("folder_id") or ROOT_DIRECTORY_ID
- title = (handler.data.get("title") or "").strip()
+ title = normalize_object_name(handler.data.get("title") or "")
access_rules = handler.data.get("access_rules") or {}
inherit_parent = handler.data.get("inherit_parent", True)
</code_context>
<issue_to_address>
**suggestion:** Create document still allows empty titles while other handlers now reject empty names/titles
Here `title` is normalized but not validated, so documents with empty titles can still be created while other operations now reject empty names/titles. For consistency and to avoid edge cases with blank titles (especially given centralized name locking/conflict detection), please add an `if not title` check here and return a 400 with `DOCUMENT_TITLE_REQUIRED` (or equivalent).
Suggested implementation:
```python
folder_id = handler.data.get("folder_id") or ROOT_DIRECTORY_ID
title = normalize_object_name(handler.data.get("title") or "")
if not title:
handler.write_error(
status_code=400,
message=smsg.DOCUMENT_TITLE_REQUIRED,
)
return
access_rules = handler.data.get("access_rules") or {}
inherit_parent = handler.data.get("inherit_parent", True)
```
1. Ensure `Messages` (`smsg`) contains `DOCUMENT_TITLE_REQUIRED`; if the constant name differs, replace `smsg.DOCUMENT_TITLE_REQUIRED` with the correct one used elsewhere for document title validation.
2. If your `ConnectionHandler` uses a different API for emitting HTTP errors (e.g. `handler.error(...)`, `handler.write_response(...)`, or raising a specific exception), adjust the `handler.write_error(...)` call to match the existing error-handling pattern used by sibling handlers that validate names/titles.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…nction and updating validation in directory and document operations
…t database session management
Collaborator
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new name locking flow only calls
release_name_write_lockon the happy path; in several handlers (e.g., document/directory create, rename, move, restore) any early return or exception after acquiring a lock will leave a persistent row indirectory_name_locks, so it would be safer to wrap the critical sections in atry/finallyand always release the lock. - In
reserve_name_for_write, whenfind_name_conflictreturns a conflict, the previously insertedDirectoryNameLockisn’t released, which means the lock row will linger even though the caller receives a conflict; consider deleting the lock before returning the conflict to keep the lock table from accumulating unnecessary rows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new name locking flow only calls `release_name_write_lock` on the happy path; in several handlers (e.g., document/directory create, rename, move, restore) any early return or exception after acquiring a lock will leave a persistent row in `directory_name_locks`, so it would be safer to wrap the critical sections in a `try/finally` and always release the lock.
- In `reserve_name_for_write`, when `find_name_conflict` returns a conflict, the previously inserted `DirectoryNameLock` isn’t released, which means the lock row will linger even though the caller receives a conflict; consider deleting the lock before returning the conflict to keep the lock table from accumulating unnecessary rows.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 by Sourcery
Introduce a locking-based mechanism to prevent directory/document name conflicts and align handlers/tests with the new conflict semantics.
Bug Fixes:
Enhancements:
Tests:
Chores: