fix: handle server-side S3 storage initialization for generic assets

[vvaswani]

Description

Fixes a 500 error in the generic asset API by updating S3Storage to accept the existing is_server=True argument used by GenericAssetEndpoint methods.

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

Test Scenarios

References

Fixes #8680

Summary by CodeRabbit

• Improvements
  • Refined storage endpoint configuration handling to better support different deployment contexts when using MinIO-based storage solutions, improving reliability and compatibility across various server environments.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Updates S3Storage.__init__() to accept an is_server parameter and conditionally configures the endpoint URL based on MinIO settings and whether the request originates from a server context.

Changes

Cohort / File(s)	Summary
S3Storage Initialization apps/api/plane/settings/storage.py	Added is_server parameter to __init__ method signature (default: False). Introduced conditional endpoint URL selection logic: when MinIO is enabled, if a request exists and is_server=False, uses host-based URL with SSL protocol detection; otherwise falls back to configured endpoint.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A parameter was lost in translation's night,
is_server whispered, but wasn't in sight,
Now endpoints dance with conditionals bright,
The S3 storage shines with all of its might! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: handling server-side S3 storage initialization for generic assets, which directly addresses the issue of S3Storage not accepting the is_server parameter.
Description check	✅ Passed	The description covers the essential elements: it explains the bug fix, indicates it's non-breaking, and references the linked issue #8680. However, screenshots/media and test scenarios sections are missing/incomplete.
Linked Issues check	✅ Passed	The code changes directly address issue #8680 by adding the is_server parameter to S3Storage.init, enabling GenericAssetEndpoint methods to pass is_server=True without TypeError.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the is_server parameter issue in S3Storage initialization; no out-of-scope modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

You can disable the changed files summary in the walkthrough.

Disable the reviews.changed_files_summary setting to disable the changed files summary in the walkthrough.

[coderabbitai]

🧹 Nitpick comments (2)

apps/api/plane/settings/storage.py (2)

46-55: Add a fail-fast guard for MinIO endpoint configuration.

When USE_MINIO=1, missing AWS_S3_ENDPOINT_URL/MINIO_ENDPOINT_URL can silently create a client against unintended defaults. Prefer explicit validation and an early error.

Proposed patch

         if os.environ.get("USE_MINIO") == "1":
             # Determine protocol based on environment variable
             if os.environ.get("MINIO_ENDPOINT_SSL") == "1":
                 endpoint_protocol = "https"
             else:
                 endpoint_protocol = request.scheme if request else "http"

             endpoint_url = self.aws_s3_endpoint_url
             if request and not is_server:
                 endpoint_url = f"{endpoint_protocol}://{request.get_host()}"
+            if not endpoint_url:
+                raise ValueError("AWS_S3_ENDPOINT_URL or MINIO_ENDPOINT_URL must be set when USE_MINIO=1")
             # Create an S3 client for MinIO
             self.s3_client = boto3.client(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/plane/settings/storage.py` around lines 46 - 55, When MinIO is
enabled, add a fail-fast check before creating self.s3_client to ensure the
MinIO endpoint is explicitly configured (i.e., aws_s3_endpoint_url /
MINIO_ENDPOINT_URL present) and raise a clear error (ValueError/RuntimeError) if
it's missing; update the logic around endpoint_url (which currently computes
endpoint_protocol and may override with request.get_host()) to validate that
when USE_MINIO (or the instance flag that enables MinIO) is true, endpoint_url
is not empty/None and abort construction of the boto3.client with a descriptive
error instead of letting boto3 use defaults. Ensure you modify the block that
sets endpoint_url and the self.s3_client initialization so the guard runs before
boto3.client is called.

---

25-55: Please add regression tests for endpoint selection branches.

This logic now controls whether presigned URLs use internal endpoint vs request host. Add targeted tests for (request present, is_server=True), (request present, is_server=False), and (request absent) under USE_MINIO=1 to lock in behavior and prevent future 500 regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/api/plane/settings/storage.py` around lines 25 - 55, Add regression
tests targeting the storage settings initializer behavior in the class with
__init__ (the constructor shown in apps/api/plane/settings/storage.py) to cover
endpoint selection under USE_MINIO=1: create three tests for (1) request present
and is_server=True, (2) request present and is_server=False, and (3) request
absent; each test should set environment vars (USE_MINIO=1 and relevant
MINIO/AWS envs), provide a stub/mocked request with scheme and get_host where
needed, instantiate the class, and assert the resolved endpoint_url used to
construct boto3.client (or the resulting s3_client.endpoint_url / stored
aws_s3_endpoint_url) matches the expected value (internal MINIO endpoint vs
request host vs default), cleaning up env changes between tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/api/plane/settings/storage.py`:
- Around line 46-55: When MinIO is enabled, add a fail-fast check before
creating self.s3_client to ensure the MinIO endpoint is explicitly configured
(i.e., aws_s3_endpoint_url / MINIO_ENDPOINT_URL present) and raise a clear error
(ValueError/RuntimeError) if it's missing; update the logic around endpoint_url
(which currently computes endpoint_protocol and may override with
request.get_host()) to validate that when USE_MINIO (or the instance flag that
enables MinIO) is true, endpoint_url is not empty/None and abort construction of
the boto3.client with a descriptive error instead of letting boto3 use defaults.
Ensure you modify the block that sets endpoint_url and the self.s3_client
initialization so the guard runs before boto3.client is called.
- Around line 25-55: Add regression tests targeting the storage settings
initializer behavior in the class with __init__ (the constructor shown in
apps/api/plane/settings/storage.py) to cover endpoint selection under
USE_MINIO=1: create three tests for (1) request present and is_server=True, (2)
request present and is_server=False, and (3) request absent; each test should
set environment vars (USE_MINIO=1 and relevant MINIO/AWS envs), provide a
stub/mocked request with scheme and get_host where needed, instantiate the
class, and assert the resolved endpoint_url used to construct boto3.client (or
the resulting s3_client.endpoint_url / stored aws_s3_endpoint_url) matches the
expected value (internal MINIO endpoint vs request host vs default), cleaning up
env changes between tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cb6143c8-639e-46c7-8d00-3bbc5a4dccc4

📥 Commits

Reviewing files that changed from the base of the PR and between 1faf06c and 44b643c.

📒 Files selected for processing (1)

• apps/api/plane/settings/storage.py