Skip to content

fix(storage): let replace mode create missing files#1788

Open
Gujiassh wants to merge 1 commit intovolcengine:mainfrom
Gujiassh:fix/content-write-replace-create
Open

fix(storage): let replace mode create missing files#1788
Gujiassh wants to merge 1 commit intovolcengine:mainfrom
Gujiassh:fix/content-write-replace-create

Conversation

@Gujiassh
Copy link
Copy Markdown
Contributor

Summary

This fixes issue #1709 by making mode=replace fall back to the existing create path when the target file does not exist yet.

The change is intentionally small:

  • allow write() to stat with allow_not_found for mode=replace
  • if the target is missing, delegate to _create_and_write(...) instead of surfacing NotFoundError
  • add a focused regression test that verifies replace-on-missing takes the create path and preserves the original call arguments

Verification

Local checks completed:

  • python3 -m py_compile openviking/storage/content_write.py tests/server/test_content_write_service.py

Focused pytest could not complete in this checkout because the repo test harness import is currently blocked by missing local Python deps (pytest_asyncio / pydantic) before the selected test module can run.

Related

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1709 - Partially compliant

Compliant requirements:

  • Fix mode=replace in PUT /api/v1/content/write to create missing files instead of returning 404
  • Implement "upsert" semantics for replace mode
  • Ensure replace mode handles non-existent files correctly

Non-compliant requirements:

Requires further human verification:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🏅 Score: 95
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

@Gujiassh Gujiassh force-pushed the fix/content-write-replace-create branch from 7e3b5e0 to f4e4f9c Compare April 30, 2026 03:24
@qin-ctx
Copy link
Copy Markdown
Collaborator

qin-ctx commented Apr 30, 2026

Thanks for the PR.

I do not think we should change mode="replace" to implicitly create missing files as-is. The current public docs define replace / append as modes that require the target file to already exist, while create is the mode that creates a new file and returns 409 Conflict when the target already exists.

This PR changes that API contract silently: a request with mode="replace" can now take the create path, return a result with mode="create", and apply create-only validation such as the extension allowlist. That is more than a bug fix; it changes the write-mode semantics.

My preference is to keep replace strict and return NotFoundError when the target does not exist. If we want server-side upsert behavior, I suggest adding an explicit mode="upsert" instead, with docs, SDK docstrings, and tests covering both branches:

  • missing target: create the file
  • existing file: replace the file
  • existing directory: reject

So I would not merge this PR in its current form. The safer direction is either to close this as expected replace behavior, or revise the PR to introduce an explicit upsert mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

[Bug]: replace 模式文件不存在时抛出 NotFoundError 而非创建文件

2 participants