mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-09-21 02:16:27 +00:00
a59cf26f56
* fix(files): return None instead of bare raise in get_uploader get_uploader is documented to return None for an unsupported provider, and every caller branches on `if uploader is None`. Two fallthrough paths ran a bare `raise` with no active exception, so an unknown provider and a Bedrock provider without a configured S3 bucket raised "RuntimeError: No active exception to reraise" instead of returning None. Return None in both paths and widen the return types to `... | None`. The Bedrock "not configured" guard now treats a falsy bucket_name (None or "") as unconfigured, not only an absent one. The except ImportError re-raises are unaffected. Fixes #7282 * fix(files): raise ValueError from get_uploader for unknown/unconfigured providers Per review, raise a ValueError with a concrete reason instead of returning None. Returning None let the resolver silently fall back to inline and hid the misconfiguration from the user, so the docstring no longer promises None and the return types drop `| None`. The Bedrock guard also treats a falsy bucket_name (None or "") as unconfigured. The ImportError re-raises are unchanged. cleanup skips providers it cannot build an uploader for, so it routes get_uploader through a local helper that treats the ValueError as "unavailable" and continues the pass. * refactor(files): surface get_uploader errors through the resolver Follow-up to review. get_uploader now raises ValueError, so _get_uploader no longer promises FileUploader | None: it returns the uploader and lets the error propagate through resolve() to the caller instead of swallowing it and falling back to inline. Drop the now-dead `if uploader is None` checks at the two upload call sites. Also make the unknown-provider ValueError list the supported providers, and add a happy-path test that a configured provider returns its uploader. * fix(files): surface uploader lookup errors in async batch resolution aresolve_files gathers with return_exceptions=True, which was silently dropping files when _get_uploader raised (a missing provider SDK, or an unknown or unconfigured provider). A batch shares one provider, so such a lookup failure applies to every file: re-raise ValueError and ImportError to surface it, matching the sync resolve_files path. Genuine per-file upload errors are still logged and skipped. * fix(files): only re-raise uploader config errors in async batch resolution The earlier fix re-raised any ValueError or ImportError from asyncio.gather(return_exceptions=True), so one unrelated per-file error (for example a stream that raises ValueError when read) aborted the whole batch instead of the intended log-and-skip. _get_uploader now translates the lookup failure into a dedicated UploaderConfigurationError, and aresolve_files re-raises only that, since it applies to every file for the provider. Ordinary per-file failures stay best-effort. Adds regression tests for the wrap, a provider-setup error surfacing from the batch, and an unrelated per-file error skipped while the rest resolve. * style(files): apply ruff import sort and formatting to resolver tests --------- Co-authored-by: Vidit Ostwal <110953813+Vidit-Ostwal@users.noreply.github.com>
40 lines
1.8 KiB
Python
40 lines
1.8 KiB
Python
"""Tests for get_uploader."""
|
|
|
|
from crewai_files.uploaders import get_uploader
|
|
from crewai_files.uploaders.openai import OpenAIFileUploader
|
|
import pytest
|
|
|
|
|
|
def test_get_uploader_returns_uploader_for_configured_provider():
|
|
# Happy path: a configured provider returns its uploader instance rather
|
|
# than raising or returning None
|
|
uploader = get_uploader("openai", api_key="test-key")
|
|
assert isinstance(uploader, OpenAIFileUploader)
|
|
|
|
|
|
def test_get_uploader_raises_for_unknown_provider():
|
|
# Regression for #7282: an unsupported provider must raise a clear
|
|
# ValueError, not the opaque "RuntimeError: No active exception to reraise"
|
|
# a bare `raise` produced, and not a silent None that hides the
|
|
# misconfiguration behind an inline fallback
|
|
with pytest.raises(ValueError, match="No file uploader available"):
|
|
get_uploader("does-not-exist")
|
|
|
|
|
|
def test_get_uploader_raises_for_unconfigured_bedrock(monkeypatch):
|
|
# Bedrock without a configured S3 bucket must raise a ValueError that names
|
|
# the missing configuration, not RuntimeError and not a silent None
|
|
monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False)
|
|
with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"):
|
|
get_uploader("bedrock")
|
|
|
|
|
|
def test_get_uploader_raises_for_bedrock_with_falsy_bucket_name(monkeypatch):
|
|
# An explicit falsy bucket_name (None or "") is unconfigured just like an
|
|
# absent one, so the guard keys on the value, not key presence
|
|
monkeypatch.delenv("CREWAI_BEDROCK_S3_BUCKET", raising=False)
|
|
with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"):
|
|
get_uploader("bedrock", bucket_name=None)
|
|
with pytest.raises(ValueError, match="CREWAI_BEDROCK_S3_BUCKET"):
|
|
get_uploader("bedrock", bucket_name="")
|