Fix:S3->Oracle#134
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough이 PR은 AWS S3 특화 구성에서 OCI Secrets Manager를 통한 자격증명 지원이 있는 범용 객체 저장소 구성으로 리팩토링합니다. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant Config as StorageConfig
participant Secret as OCISecretManager
participant S3 as S3-Compatible<br/>Object Storage
App->>Config: Initialize StorageConfig()
Config->>Secret: Retrieve object_storage_*<br/>secrets (if vault-backed)
Secret-->>Config: bucket_name, endpoint_url,<br/>credentials, region
Config-->>App: Configured storage parameters
App->>App: Initialize S3Client()
App->>S3: Create client with<br/>endpoint_url, region,<br/>addressing_style
S3-->>App: S3 client ready
App->>S3: Generate presigned POST URL
S3-->>App: URL + form fields
App->>S3: Upload object
S3-->>App: 204/200 response
App->>S3: Generate presigned GET URL
S3-->>App: Download URL
App->>S3: Download object
S3-->>App: Object data
App->>S3: Delete object
S3-->>App: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
test/check_vault_bucket_name.py (1)
16-20: 사용되지 않는 헬퍼 함수
_required_env함수가 정의되어 있지만 이 스크립트 내에서 호출되는 곳이 없습니다. 사용할 계획이 없다면 제거하는 것이 좋습니다.🔧 제거 제안
-def _required_env(name: str) -> str: - value = os.getenv(name, "").strip() - if not value: - raise ValueError(f"Missing required env: {name}") - return value - -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/check_vault_bucket_name.py` around lines 16 - 20, The helper function _required_env is defined but never used in this script; either delete the _required_env function to remove dead code or replace existing getenv usage with calls to _required_env (e.g., where env variables are read) so the helper is actually utilized; locate the _required_env definition and remove it or refactor the env retrieval sites to call _required_env accordingly.wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py (2)
46-68: 헬퍼 함수 내 불필요한 context 재생성
_assert_post_contract와_assert_get_contract내부에서_build_storage_context()를 다시 호출하고 있습니다(Line 47, 72). 테스트 함수에서 이미 생성한s3_client를 파라미터로 전달하면 중복 생성을 피할 수 있습니다.🔧 파라미터로 전달하는 방식 제안
-def _assert_post_contract(storage_config: StorageConfig, object_key: str) -> None: - _, s3_client = _build_storage_context() +def _assert_post_contract( + storage_config: StorageConfig, s3_client: BaseClient, object_key: str +) -> None: post_url, fields = generate_presigned_post_url(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py` around lines 46 - 68, The helper functions _assert_post_contract and _assert_get_contract recreate the storage context by calling _build_storage_context() unnecessarily; change their signatures to accept an s3_client parameter (instead of calling _build_storage_context inside), remove the internal _build_storage_context() call, and use the passed s3_client when calling generate_presigned_post_url / generate_presigned_get_url; update all test callers to pass the existing s3_client returned by the test setup so no duplicate clients are created.
25-27: 테스트 파일 간 코드 중복
_reset_singletons(),_target_env(),_build_storage_context()패턴이test_oracle_secret_integration.py와 중복됩니다. 공통 테스트 유틸리티 모듈로 추출하면 유지보수성이 향상됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py` around lines 25 - 27, The three helper routines _reset_singletons, _target_env, and _build_storage_context are duplicated across tests; extract them into a shared test utility module (e.g., tests/util/test_helpers or similar), export those functions, and replace the duplicated definitions in both test_oracle_presigned_contract.py and test_oracle_secret_integration.py with imports from that module; ensure you reference and update usages of SingletonMeta._instances manipulation in _reset_singletons, the environment-handling logic in _target_env, and the storage construction in _build_storage_context so existing tests import the helpers and run unchanged.wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py (1)
56-81:settings.env변경 후 복원 누락테스트에서
settings.env를 변경하지만 원래 값으로 복원하지 않습니다. 이후 실행되는 테스트에 영향을 줄 수 있습니다. pytest fixture나 try-finally 패턴을 사용하여 정리해 주세요.🔧 fixture를 사용한 정리 패턴 제안
`@pytest.fixture` def restore_settings_env(): original_env = settings.env yield settings.env = original_env _reset_singletons()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py` around lines 56 - 81, test_storage_configuration_is_available mutates settings.env and doesn't restore it, which can leak state into other tests; update the test to restore settings.env and call _reset_singletons() after changing it (either by using a pytest fixture like restore_settings_env that saves and yields the original settings.env or by wrapping the modification in a try/finally block inside test_storage_configuration_is_available), ensuring the original value is reassigned to settings.env in the cleanup path so subsequent tests see the original environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/check_vault_bucket_name.py`:
- Line 42: The default key name used when reading the env var into key_name is
inconsistent with StorageConfig; change the default from "portfolio_bucket_name"
to "object_storage_bucket_name" so tests align with StorageConfig
expectations—update the assignment that sets key_name (the os.getenv call) to
use "object_storage_bucket_name" as the fallback and ensure any related
assertions referencing StorageConfig.object_storage_bucket_name remain
consistent.
In `@wacruit/src/apps/portfolio/file/aws/config.py`:
- Around line 20-32: The __init__ in the AWS config currently assumes
OCISecretManager.get_secret always returns the requested keys and will raise
KeyError if a secret is missing; update the __init__ to handle missing secrets
by wrapping each call to OCISecretManager.get_secret (for bucket_name, region,
endpoint_url, access_key_id, secret_access_key) with a try/except KeyError (or
using a .get-like fallback) and fall back to sensible defaults such as
environment variables or None; keep the is_available() check but ensure
missing-key handling prevents startup crashes when a particular secret is
absent.
In `@wacruit/src/apps/portfolio/file/aws/s3/client.py`:
- Line 7: S3Client가 모듈 레벨의 전역 storage_config를 고정으로 캡처해 환경 전환 시 이전 엔드포인트/자격증명을 계속
사용하게 되는 문제이므로, S3Client 생성 시 또는 호출 시 최신 설정을 주입하도록 변경하세요; 구체적으로 현재의
storage_config 전역 참조를 제거하고 S3Client.__init__에 storage_config 파라미터(또는 설정을 반환하는
콜백)를 추가하거나, S3Client.get_client()/connect() 같은 메서드 내부에서 런타임에 storage_config를 다시
조회하도록 수정하여 동적 환경 전환을 지원하게 만드세요.
In `@wacruit/src/tests/portfolio/file/test_portfolio_file_service.py`:
- Around line 25-29: The expected_url in the test currently builds the custom
endpoint without a trailing slash causing mismatch with boto3's
generate_presigned_post; update the expression that constructs expected_url (the
conditional using storage_config.endpoint_url and storage_config.bucket_name) to
append a trailing '/' after the bucket for the custom endpoint branch so it
matches the AWS default branch and the URL returned by generate_presigned_post.
- Around line 152-156: The tests fail because StorageConfig (env_prefix =
"OBJECT_STORAGE_") expects OBJECT_STORAGE_BUCKET_NAME but .env.test defines
S3_PORTFOLIO_BUCKET_NAME, so storage_config.bucket_name becomes empty; fix by
either updating .env.test to set
OBJECT_STORAGE_BUCKET_NAME=wacruit-portfolio-test or by setting
storage_config.bucket_name="wacruit-portfolio-test" in the test setup (e.g., in
conftest.py where the bucket is created) so the Key in
test_portfolio_file_service.py uses the actual bucket name.
In `@wacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.py`:
- Around line 50-55: The test helper _build_storage_context creates a new
StorageConfig() and separately instantiates S3Client().client which reads the
module-global storage_config in
wacruit/src/apps/portfolio/file/aws/s3/client.py, and it also mutates
settings.env without restoring it; fix this by creating a single StorageConfig
instance and ensuring the S3 client uses that same instance (e.g., set the
client module's storage_config to the created StorageConfig or construct the
S3Client with that config) and after _reset_singletons() and client creation
restore settings.env to its original value so tests do not leak global env;
update references in _build_storage_context, StorageConfig, and S3Client usage
accordingly.
In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py`:
- Around line 26-28: The singleton reset function _reset_singletons currently
mutates SingletonMeta._instances without synchronization; modify it to acquire
SingletonMeta._lock while removing entries for OCISecretManager and S3Client to
avoid race conditions. Specifically, inside _reset_singletons obtain
SingletonMeta._lock (e.g., with a context or explicit acquire/release) and then
pop those keys from SingletonMeta._instances, preserving thread safety while
resetting singletons.
In `@wacruit/src/tests/project/test_project_service.py`:
- Around line 112-127: The test currently false-positives because it never
creates the S3 object before calling delete_project_image; ensure the object
exists first, e.g. use the generated presigned URL to PUT the file or call the
S3 client put_object (project_service._s3_client.client.put_object) to upload
"PROJECT/{project.id}/image.png" after
generate_presigned_url_for_post_image/register_project_image_info_in_db, then
call project_service.delete_project_image(upload_response.project_image_id) and
finally assert head_object raises ClientError; reference
generate_presigned_url_for_post_image, register_project_image_info_in_db,
delete_project_image and project_service._s3_client.client.head_object to locate
where to add the pre-delete upload.
---
Nitpick comments:
In `@test/check_vault_bucket_name.py`:
- Around line 16-20: The helper function _required_env is defined but never used
in this script; either delete the _required_env function to remove dead code or
replace existing getenv usage with calls to _required_env (e.g., where env
variables are read) so the helper is actually utilized; locate the _required_env
definition and remove it or refactor the env retrieval sites to call
_required_env accordingly.
In `@wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py`:
- Around line 46-68: The helper functions _assert_post_contract and
_assert_get_contract recreate the storage context by calling
_build_storage_context() unnecessarily; change their signatures to accept an
s3_client parameter (instead of calling _build_storage_context inside), remove
the internal _build_storage_context() call, and use the passed s3_client when
calling generate_presigned_post_url / generate_presigned_get_url; update all
test callers to pass the existing s3_client returned by the test setup so no
duplicate clients are created.
- Around line 25-27: The three helper routines _reset_singletons, _target_env,
and _build_storage_context are duplicated across tests; extract them into a
shared test utility module (e.g., tests/util/test_helpers or similar), export
those functions, and replace the duplicated definitions in both
test_oracle_presigned_contract.py and test_oracle_secret_integration.py with
imports from that module; ensure you reference and update usages of
SingletonMeta._instances manipulation in _reset_singletons, the
environment-handling logic in _target_env, and the storage construction in
_build_storage_context so existing tests import the helpers and run unchanged.
In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py`:
- Around line 56-81: test_storage_configuration_is_available mutates
settings.env and doesn't restore it, which can leak state into other tests;
update the test to restore settings.env and call _reset_singletons() after
changing it (either by using a pytest fixture like restore_settings_env that
saves and yields the original settings.env or by wrapping the modification in a
try/finally block inside test_storage_configuration_is_available), ensuring the
original value is reassigned to settings.env in the cleanup path so subsequent
tests see the original environment.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c40ec7ac-4a10-4f9f-adf0-dcab6d150920
📒 Files selected for processing (13)
.gitignoretest/check_vault_bucket_name.pytest/oci_s3_connection_test.pywacruit/src/apps/portfolio/file/aws/config.pywacruit/src/apps/portfolio/file/aws/s3/client.pywacruit/src/apps/portfolio/file/services.pywacruit/src/apps/portfolio/file/services_v2.pywacruit/src/apps/project/services.pywacruit/src/tests/portfolio/file/test_portfolio_file_service.pywacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.pywacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.pywacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.pywacruit/src/tests/project/test_project_service.py
| "Missing OCI secret id. Set OCI_SECRET_OCID in .env.test " | ||
| "or run with ENV=dev/prod so settings.oci_secret_ocid is available." | ||
| ) | ||
| key_name = os.getenv("OCI_BUCKET_KEY_NAME", "portfolio_bucket_name") |
There was a problem hiding this comment.
기본 키 이름이 StorageConfig와 불일치할 수 있음
기본값 portfolio_bucket_name이 StorageConfig에서 사용하는 object_storage_bucket_name과 다릅니다. 이 스크립트가 새로운 스토리지 설정을 검증하기 위한 것이라면 기본값을 object_storage_bucket_name으로 변경하는 것이 일관성 있습니다.
🔧 수정 제안
- key_name = os.getenv("OCI_BUCKET_KEY_NAME", "portfolio_bucket_name")
+ key_name = os.getenv("OCI_BUCKET_KEY_NAME", "object_storage_bucket_name")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| key_name = os.getenv("OCI_BUCKET_KEY_NAME", "portfolio_bucket_name") | |
| key_name = os.getenv("OCI_BUCKET_KEY_NAME", "object_storage_bucket_name") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/check_vault_bucket_name.py` at line 42, The default key name used when
reading the env var into key_name is inconsistent with StorageConfig; change the
default from "portfolio_bucket_name" to "object_storage_bucket_name" so tests
align with StorageConfig expectations—update the assignment that sets key_name
(the os.getenv call) to use "object_storage_bucket_name" as the fallback and
ensure any related assertions referencing
StorageConfig.object_storage_bucket_name remain consistent.
Comment on lines
20
to
+32
| def __init__(self): | ||
| super().__init__() | ||
| aws_secrets = OCISecretManager() | ||
| if aws_secrets.is_available(): | ||
| self.bucket_name = aws_secrets.get_secret("portfolio_bucket_name") | ||
|
|
||
| @property | ||
| def bucket_region(self) -> str: | ||
| return "ap-northeast-2" | ||
|
|
||
|
|
||
| s3_config = S3PortfolioConfig() | ||
| secret_manager = OCISecretManager() | ||
| if secret_manager.is_available(): | ||
| self.bucket_name = secret_manager.get_secret("object_storage_bucket_name") | ||
| self.region = secret_manager.get_secret("object_storage_region") | ||
| self.endpoint_url = secret_manager.get_secret("object_storage_endpoint_url") | ||
| self.access_key_id = secret_manager.get_secret( | ||
| "object_storage_access_key_id" | ||
| ) | ||
| self.secret_access_key = secret_manager.get_secret( | ||
| "object_storage_secret_access_key" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: OCISecretManager.get_secret 메서드의 에러 처리 확인
ast-grep --pattern $'def get_secret(self, $key) -> str:
$$$
}'Repository: wafflestudio/wacruit-server
Length of output: 53
🏁 Script executed:
find . -name "secrets.py" -type f | head -20Repository: wafflestudio/wacruit-server
Length of output: 95
🏁 Script executed:
rg "class OCISecretManager" -A 20Repository: wafflestudio/wacruit-server
Length of output: 1436
🏁 Script executed:
cat wacruit/src/apps/portfolio/file/aws/config.py | head -40Repository: wafflestudio/wacruit-server
Length of output: 1246
🏁 Script executed:
rg "def get_secret" wacruit/src/secrets.py -A 15Repository: wafflestudio/wacruit-server
Length of output: 602
🏁 Script executed:
cat wacruit/src/secrets.py | tail -50Repository: wafflestudio/wacruit-server
Length of output: 1742
Vault의 키 누락 시 KeyError 발생 위험
OCISecretManager.get_secret() 메서드는 직접 딕셔너리 접근 (secret_data[key])을 통해 데이터를 반환하며, 요청한 키가 없으면 KeyError를 발생시킵니다. Vault가 사용 가능하다고 해도 (is_available() 반환 True) 특정 키가 누락되어 있으면 애플리케이션 시작 시 실패합니다.
각 get_secret() 호출을 try-except로 감싸거나 환경 변수 기본값으로 fallback하도록 개선하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacruit/src/apps/portfolio/file/aws/config.py` around lines 20 - 32, The
__init__ in the AWS config currently assumes OCISecretManager.get_secret always
returns the requested keys and will raise KeyError if a secret is missing;
update the __init__ to handle missing secrets by wrapping each call to
OCISecretManager.get_secret (for bucket_name, region, endpoint_url,
access_key_id, secret_access_key) with a try/except KeyError (or using a
.get-like fallback) and fall back to sensible defaults such as environment
variables or None; keep the is_available() check but ensure missing-key handling
prevents startup crashes when a particular secret is absent.
| from botocore.client import BaseClient | ||
| from botocore.config import Config | ||
|
|
||
| from wacruit.src.apps.portfolio.file.aws.config import storage_config |
There was a problem hiding this comment.
초기화 시점 설정 고정으로 잘못된 스토리지 대상에 연결될 위험이 있습니다.
Line 7, Line 14~24 구조는 S3Client가 전역 storage_config를 단일 인스턴스로 캡처합니다. 환경(settings.env/시크릿 소스) 전환 후에도 이전 endpoint/credential을 계속 사용할 수 있어, 라이브 테스트 및 멀티 환경 실행에서 오동작할 수 있습니다.
🔧 제안 수정안 (설정 주입 가능 + 고정 결합 완화)
-from wacruit.src.apps.portfolio.file.aws.config import storage_config
+from wacruit.src.apps.portfolio.file.aws.config import StorageConfig, storage_config
class S3Client(metaclass=SingletonMeta):
_client: BaseClient
- def __init__(self):
+ def __init__(self, config: StorageConfig | None = None):
+ cfg = config or storage_config
self._client = cast(
BaseClient,
boto3.client(
"s3",
- region_name=storage_config.region,
- endpoint_url=storage_config.endpoint_url,
- aws_access_key_id=storage_config.access_key_id,
- aws_secret_access_key=storage_config.secret_access_key,
- config=Config(s3={"addressing_style": storage_config.addressing_style}),
+ region_name=cfg.region,
+ endpoint_url=cfg.endpoint_url,
+ aws_access_key_id=cfg.access_key_id,
+ aws_secret_access_key=cfg.secret_access_key,
+ config=Config(s3={"addressing_style": cfg.addressing_style}),
),
)Also applies to: 14-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacruit/src/apps/portfolio/file/aws/s3/client.py` at line 7, S3Client가 모듈 레벨의
전역 storage_config를 고정으로 캡처해 환경 전환 시 이전 엔드포인트/자격증명을 계속 사용하게 되는 문제이므로, S3Client 생성
시 또는 호출 시 최신 설정을 주입하도록 변경하세요; 구체적으로 현재의 storage_config 전역 참조를 제거하고
S3Client.__init__에 storage_config 파라미터(또는 설정을 반환하는 콜백)를 추가하거나,
S3Client.get_client()/connect() 같은 메서드 내부에서 런타임에 storage_config를 다시 조회하도록 수정하여
동적 환경 전환을 지원하게 만드세요.
Comment on lines
+25
to
29
| expected_url = ( | ||
| f"{storage_config.endpoint_url}/{storage_config.bucket_name}" | ||
| if storage_config.endpoint_url | ||
| else f"https://{storage_config.bucket_name}.s3.amazonaws.com/" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: generate_presigned_post 반환 URL 형식 확인
rg -n "generate_presigned_post" --type py -A 10Repository: wafflestudio/wacruit-server
Length of output: 12927
🏁 Script executed:
cat -n wacruit/src/tests/portfolio/file/test_portfolio_file_service.py | head -50Repository: wafflestudio/wacruit-server
Length of output: 2276
🏁 Script executed:
rg -n "expected_url" wacruit/src/tests/portfolio/file/test_portfolio_file_service.py -A 5 -B 5Repository: wafflestudio/wacruit-server
Length of output: 893
🏁 Script executed:
cat -n wacruit/src/apps/portfolio/file/aws/s3/utils.py | sed -n '88,105p'Repository: wafflestudio/wacruit-server
Length of output: 560
🏁 Script executed:
cat -n wacruit/src/apps/portfolio/file/services_v2.py | sed -n '111,140p'Repository: wafflestudio/wacruit-server
Length of output: 1285
🏁 Script executed:
rg -n "PresignedUrlResponse" wacruit/src/apps/portfolio/file/ -A 10Repository: wafflestudio/wacruit-server
Length of output: 11910
🏁 Script executed:
fd -t f "config.py" wacruit/src/apps/portfolio/file/aws/Repository: wafflestudio/wacruit-server
Length of output: 116
🏁 Script executed:
cat -n wacruit/src/apps/portfolio/file/aws/config.pyRepository: wafflestudio/wacruit-server
Length of output: 1491
🏁 Script executed:
# boto3 generate_presigned_post 반환 형식 확인
rg -n "def generate_presigned_post" wacruit/src/apps/portfolio/file/aws/s3/utils.py -A 30 | head -40Repository: wafflestudio/wacruit-server
Length of output: 1210
🏁 Script executed:
# 테스트에서 실제로 비교하는 부분 확인
cat -n wacruit/src/tests/portfolio/file/test_portfolio_file_service.py | sed -n '20,42p'Repository: wafflestudio/wacruit-server
Length of output: 1128
커스텀 엔드포인트 URL에 trailing slash 추가 필요
26번 줄의 커스텀 엔드포인트 경로에 trailing slash가 누락되어 있습니다. boto3의 generate_presigned_post가 반환하는 URL은 항상 마지막에 /를 포함하므로, 28번 줄의 AWS 기본 경로와 일치하도록 수정하세요.
수정 위치
expected_url = (
f"{storage_config.endpoint_url}/{storage_config.bucket_name}/" # trailing slash 추가
if storage_config.endpoint_url
else f"https://{storage_config.bucket_name}.s3.amazonaws.com/"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacruit/src/tests/portfolio/file/test_portfolio_file_service.py` around lines
25 - 29, The expected_url in the test currently builds the custom endpoint
without a trailing slash causing mismatch with boto3's generate_presigned_post;
update the expression that constructs expected_url (the conditional using
storage_config.endpoint_url and storage_config.bucket_name) to append a trailing
'/' after the bucket for the custom endpoint branch so it matches the AWS
default branch and the URL returned by generate_presigned_post.
Comment on lines
+50
to
+55
| def _build_storage_context() -> tuple[StorageConfig, BaseClient]: | ||
| settings.env = cast(Literal["dev", "prod", "local", "test"], _target_env()) | ||
| _reset_singletons() | ||
| storage_config = StorageConfig() | ||
| s3_client = S3Client().client | ||
|
|
There was a problem hiding this comment.
스토리지 설정 컨텍스트와 S3 클라이언트 설정 소스가 불일치할 수 있습니다.
Line 53에서 만든 StorageConfig()와 Line 54의 S3Client().client는 동일 설정 객체를 보장하지 않습니다(클라이언트는 wacruit/src/apps/portfolio/file/aws/s3/client.py의 전역 storage_config를 참조). 또한 Line 51의 settings.env 변경을 복구하지 않아 다른 테스트에 전역 상태 오염을 일으킬 수 있습니다.
🔧 제안 수정안 (동일 config로 직접 client 생성 + env 복구)
+import boto3
+from botocore.config import Config
@@
def _build_storage_context() -> tuple[StorageConfig, BaseClient]:
- settings.env = cast(Literal["dev", "prod", "local", "test"], _target_env())
- _reset_singletons()
- storage_config = StorageConfig()
- s3_client = S3Client().client
+ previous_env = settings.env
+ try:
+ settings.env = cast(Literal["dev", "prod", "local", "test"], _target_env())
+ _reset_singletons()
+ storage_config = StorageConfig()
+ s3_client = cast(
+ BaseClient,
+ boto3.client(
+ "s3",
+ region_name=storage_config.region,
+ endpoint_url=storage_config.endpoint_url,
+ aws_access_key_id=storage_config.access_key_id,
+ aws_secret_access_key=storage_config.secret_access_key,
+ config=Config(s3={"addressing_style": storage_config.addressing_style}),
+ ),
+ )
@@
- return storage_config, s3_client
+ return storage_config, s3_client
+ finally:
+ settings.env = previous_env
+ _reset_singletons()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.py` around
lines 50 - 55, The test helper _build_storage_context creates a new
StorageConfig() and separately instantiates S3Client().client which reads the
module-global storage_config in
wacruit/src/apps/portfolio/file/aws/s3/client.py, and it also mutates
settings.env without restoring it; fix this by creating a single StorageConfig
instance and ensuring the S3 client uses that same instance (e.g., set the
client module's storage_config to the created StorageConfig or construct the
S3Client with that config) and after _reset_singletons() and client creation
restore settings.env to its original value so tests do not leak global env;
update references in _build_storage_context, StorageConfig, and S3Client usage
accordingly.
Comment on lines
+26
to
+28
| def _reset_singletons() -> None: | ||
| for singleton_cls in (OCISecretManager, S3Client): | ||
| SingletonMeta._instances.pop(singleton_cls, None) |
There was a problem hiding this comment.
싱글톤 리셋 시 스레드 안전성 우려
SingletonMeta._instances.pop()은 SingletonMeta._lock을 획득하지 않고 직접 인스턴스를 제거합니다. 병렬 테스트 실행 시 경쟁 조건이 발생할 수 있습니다.
🔧 lock을 사용한 안전한 리셋 제안
def _reset_singletons() -> None:
- for singleton_cls in (OCISecretManager, S3Client):
- SingletonMeta._instances.pop(singleton_cls, None)
+ with SingletonMeta._lock:
+ for singleton_cls in (OCISecretManager, S3Client):
+ SingletonMeta._instances.pop(singleton_cls, None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _reset_singletons() -> None: | |
| for singleton_cls in (OCISecretManager, S3Client): | |
| SingletonMeta._instances.pop(singleton_cls, None) | |
| def _reset_singletons() -> None: | |
| with SingletonMeta._lock: | |
| for singleton_cls in (OCISecretManager, S3Client): | |
| SingletonMeta._instances.pop(singleton_cls, None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py` around
lines 26 - 28, The singleton reset function _reset_singletons currently mutates
SingletonMeta._instances without synchronization; modify it to acquire
SingletonMeta._lock while removing entries for OCISecretManager and S3Client to
avoid race conditions. Specifically, inside _reset_singletons obtain
SingletonMeta._lock (e.g., with a context or explicit acquire/release) and then
pop those keys from SingletonMeta._instances, preserving thread safety while
resetting singletons.
Comment on lines
+112
to
+127
| upload_response = project_service.generate_presigned_url_for_post_image( | ||
| project_id=project.id, | ||
| file_name="image.png", | ||
| ) | ||
| project_service.register_project_image_info_in_db( | ||
| file_id=upload_response.project_image_id | ||
| ) | ||
|
|
||
| project_service.delete_project_image(upload_response.project_image_id) | ||
|
|
||
| with pytest.raises(ClientError): | ||
| project_service._s3_client.client.head_object( | ||
| Bucket=storage_config.bucket_name, | ||
| Key=f"PROJECT/{project.id}/image.png", | ||
| ) | ||
|
|
There was a problem hiding this comment.
삭제 테스트가 거짓 양성(false positive)을 허용합니다.
Line 112~127에서는 실제 S3 객체를 먼저 생성하지 않아도 head_object는 실패합니다. 현재 검증은 “삭제 성공”이 아니라 “원래 없던 객체 조회 실패”도 통과시킬 수 있습니다.
🔧 제안 수정안 (삭제 전 존재 보장)
def test_delete_project_image(
project: Project,
project_service: ProjectService,
):
@@
project_service.register_project_image_info_in_db(
file_id=upload_response.project_image_id
)
+ object_key = f"PROJECT/{project.id}/image.png"
+ project_service._s3_client.client.put_object(
+ Bucket=storage_config.bucket_name,
+ Key=object_key,
+ Body=b"smoke-test",
+ )
+ project_service._s3_client.client.head_object(
+ Bucket=storage_config.bucket_name,
+ Key=object_key,
+ )
project_service.delete_project_image(upload_response.project_image_id)
with pytest.raises(ClientError):
project_service._s3_client.client.head_object(
Bucket=storage_config.bucket_name,
- Key=f"PROJECT/{project.id}/image.png",
+ Key=object_key,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| upload_response = project_service.generate_presigned_url_for_post_image( | |
| project_id=project.id, | |
| file_name="image.png", | |
| ) | |
| project_service.register_project_image_info_in_db( | |
| file_id=upload_response.project_image_id | |
| ) | |
| project_service.delete_project_image(upload_response.project_image_id) | |
| with pytest.raises(ClientError): | |
| project_service._s3_client.client.head_object( | |
| Bucket=storage_config.bucket_name, | |
| Key=f"PROJECT/{project.id}/image.png", | |
| ) | |
| upload_response = project_service.generate_presigned_url_for_post_image( | |
| project_id=project.id, | |
| file_name="image.png", | |
| ) | |
| project_service.register_project_image_info_in_db( | |
| file_id=upload_response.project_image_id | |
| ) | |
| object_key = f"PROJECT/{project.id}/image.png" | |
| project_service._s3_client.client.put_object( | |
| Bucket=storage_config.bucket_name, | |
| Key=object_key, | |
| Body=b"smoke-test", | |
| ) | |
| project_service._s3_client.client.head_object( | |
| Bucket=storage_config.bucket_name, | |
| Key=object_key, | |
| ) | |
| project_service.delete_project_image(upload_response.project_image_id) | |
| with pytest.raises(ClientError): | |
| project_service._s3_client.client.head_object( | |
| Bucket=storage_config.bucket_name, | |
| Key=object_key, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacruit/src/tests/project/test_project_service.py` around lines 112 - 127,
The test currently false-positives because it never creates the S3 object before
calling delete_project_image; ensure the object exists first, e.g. use the
generated presigned URL to PUT the file or call the S3 client put_object
(project_service._s3_client.client.put_object) to upload
"PROJECT/{project.id}/image.png" after
generate_presigned_url_for_post_image/register_project_image_info_in_db, then
call project_service.delete_project_image(upload_response.project_image_id) and
finally assert head_object raises ClientError; reference
generate_presigned_url_for_post_image, register_project_image_info_in_db,
delete_project_image and project_service._s3_client.client.head_object to locate
where to add the pre-delete upload.
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.
No description provided.