Skip to content

fix: restore public asset storage initialization - #9703

Open
NmadeleiDev wants to merge 1 commit into
makeplane:previewfrom
NmadeleiDev:fix/public-asset-storage-constructor
Open

fix: restore public asset storage initialization#9703
NmadeleiDev wants to merge 1 commit into
makeplane:previewfrom
NmadeleiDev:fix/public-asset-storage-constructor

Conversation

@NmadeleiDev

@NmadeleiDev NmadeleiDev commented Aug 27, 2026

Copy link
Copy Markdown

Description

The public API asset routes currently instantiate S3Storage with an unsupported is_server keyword. S3Storage.__init__ has always accepted only request, so authenticated generic uploads, downloads, and server-backed user uploads fail with HTTP 500 before a presigned URL is generated.

This removes the invalid keyword from all three call sites and uses the same request-aware constructor as the adjacent working asset routes. The regression tests autospec the real S3Storage signature, so future constructor/caller drift fails at the API contract boundary instead of being hidden by a permissive mock.

This is intentionally narrower than #9114: it fixes the callers without adding a second storage mode or changing MinIO endpoint selection. Presigned URLs are returned to clients, so preserving the existing request-derived public endpoint behavior is the safer compatibility choice.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

Screenshots and Media (if applicable)

Not applicable; this fixes public REST API responses.

Test Scenarios

  • Reproduced the regression with autospecced storage: generic GET and POST returned HTTP 500 with TypeError: got an unexpected keyword argument 'is_server' before the fix.
  • pytest plane/tests/contract/api/test_asset_storage.py plane/tests/contract/api/test_generic_asset.py -vv — 7 passed.
  • ruff check plane/api/views/asset.py plane/tests/contract/api/test_asset_storage.py plane/tests/contract/api/test_generic_asset.py — passed.
  • ruff format --check plane/tests/contract/api/test_asset_storage.py plane/tests/contract/api/test_generic_asset.py — passed.
  • Full public API contract directory: 59 passed; 2 existing rate-limit-state failures. Both unrelated failures pass in isolation after resetting the documented ephemeral test stack.

References

Fixes #8680
Fixes #9563
Related alternative: #9114

Summary by CodeRabbit

  • Bug Fixes

    • Improved asset upload and download handling for more reliable storage operations.
    • Presigned asset URLs and asset ownership associations continue to be generated correctly.
  • Tests

    • Added coverage for public asset upload and download workflows, including storage initialization and asset associations.

@CLAassistant

CLAassistant commented Aug 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 211d2d05-9bba-49ec-9771-d2d4307635a4

📥 Commits

Reviewing files that changed from the base of the PR and between 5016013 and fa40ae9.

📒 Files selected for processing (1)
  • apps/api/plane/tests/contract/api/test_asset_storage.py

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Public asset download and upload endpoints now construct S3Storage with the supported request argument. New contract tests cover presigned URL generation and FileAsset associations for workspace and user assets.

Changes

Public asset storage

Layer / File(s) Summary
Endpoint storage initialization
apps/api/plane/api/views/asset.py
Server asset upload, generic asset download, and generic asset upload now call S3Storage(request=request) without is_server=True.
Storage initialization contract tests
apps/api/plane/tests/contract/api/test_asset_storage.py
Contract tests verify storage construction, successful responses, presigned URLs, and correct workspace or user FileAsset associations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to fa40a

This restores existing public asset upload and download flows without changing storage modes or deployment behavior; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: dheeru0198, pablohashescobar

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: restoring correct storage initialization for public assets.
Description check ✅ Passed The description includes the required sections, explains the bug and fix, identifies the change type, documents test scenarios and results, and references the related issues.
Linked Issues check ✅ Passed The changes address both linked issues [#8680] and [#9563] by removing the unsupported is_server argument from the affected public asset storage calls and preserving request-aware behavior.
Out of Scope Changes check ✅ Passed The code and regression tests remain within scope. They implement the linked issue fixes without adding a storage mode, changing endpoint selection, or introducing unrelated behavior.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Public asset upload and download routes pass an unsupported is_server keyword to S3Storage, so authenticated requests fail with HTTP 500 before a presigned URL can be generated.

Use the existing request-only constructor consistently with adjacent asset routes and add autospecced API contract coverage for generic upload, generic download, and user server upload.
@NmadeleiDev
NmadeleiDev force-pushed the fix/public-asset-storage-constructor branch from 5016013 to fa40ae9 Compare August 27, 2026 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants