diff --git a/CHANGELOG.md b/CHANGELOG.md index 79da33b..e3a14bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,19 @@ +## 0.0.45 + +* **`/invoke` no longer demands a body from a plugin whose parameters are all optional.** A pydantic + body parameter with no default is mandatory even when every field inside the model is optional, so + such a plugin required a body that no caller has a reason to populate — and before it grew those + parameters the same plugin accepted no body at all, which made adding one look + backward-compatible while silently flipping the HTTP contract to 422 for every bodyless caller. + An absent body now resolves each field to its own default, which is what the signature already + promised. Plugins with at least one required field are unchanged: a missing `file_data` still + fails validation rather than arriving as `None`. +* **An optional `file_data` no longer 500s when absent.** The wrapper converted `file_data` from its + dict form unconditionally, so a plugin declaring it optional hit + `AttributeError: 'NoneType' object has no attribute 'model_dump'` on any body that omitted it — + previously reachable via `POST {}`, and via a bodyless request once the change above landed. `None` + is now passed through untouched and only a real value is converted. + ## 0.0.44 * **Ignore SIGTERM in plugin uvicorn Servers**: plugin webservers now keep diff --git a/test/api/test_api.py b/test/api/test_api.py index f8bca3c..83e2b23 100644 --- a/test/api/test_api.py +++ b/test/api/test_api.py @@ -470,3 +470,114 @@ def test_streaming_unstructured_ingest_error_with_none_status_code(): "Async gen test UnstructuredIngestError with None status_code" in invoke_response.status_code_text ) + + +# --- optional-body contract ------------------------------------------------------------------- +# +# A pydantic body parameter with no default is mandatory even when every field inside the model is +# optional. A plugin whose parameters are ALL optional therefore used to demand a body that no +# caller has a reason to populate: before it grew those parameters the same plugin accepted no body +# at all, so adding one silently flipped its HTTP contract and every bodyless caller got a 422. + + +class _Echo(BaseModel): + settings: Optional[dict] = None + context: Optional[dict] = None + received: Optional[str] = None + + +def _all_optional( + invocation_settings: Optional[dict] = None, invocation_context: Optional[dict] = None +) -> _Echo: + return _Echo(settings=invocation_settings, context=invocation_context) + + +def _no_params() -> _Echo: + return _Echo(received="ok") + + +def _has_required(element_dicts: str, invocation_context: Optional[dict] = None) -> _Echo: + return _Echo(received=element_dicts) + + +@pytest.mark.parametrize("body", [None, {}]) +def test_all_optional_params_accept_absent_or_empty_body(body): + client = TestClient(wrap_in_fastapi(func=_all_optional, plugin_id="mock_plugin")) + + kwargs = {} if body is None else {"json": body} + resp = client.post("/invoke", **kwargs) + + assert resp.status_code == 200 + invoke_response = InvokeResponse.model_validate(resp.json()) + invoke_response.generic_validation() + # Each field resolves to its own default, which is what the signature already promised. + assert invoke_response.output == {"settings": None, "context": None, "received": None} + + +def test_all_optional_params_still_receive_a_populated_body(): + # The tolerance must not swallow a body that IS supplied, or the wire-settings plane silently + # stops working while every request keeps returning 200. + client = TestClient(wrap_in_fastapi(func=_all_optional, plugin_id="mock_plugin")) + + resp = client.post("/invoke", json={"invocation_settings": {"k": "v"}}) + + assert resp.status_code == 200 + assert InvokeResponse.model_validate(resp.json()).output == { + "settings": {"k": "v"}, + "context": None, + "received": None, + } + + +def test_required_param_still_rejects_an_absent_body(): + # The tolerance must not leak into plugins that genuinely need input: a downloader invoked + # without `file_data` has to fail loudly rather than receive None. + client = TestClient(wrap_in_fastapi(func=_has_required, plugin_id="mock_plugin")) + + assert client.post("/invoke").status_code == 422 + assert client.post("/invoke", json={}).status_code == 422 + assert client.post("/invoke", json={"element_dicts": "x"}).status_code == 200 + + +class _FileDataEcho(BaseModel): + identifier: Optional[str] = None + + +def _optional_file_data( + file_data: Optional[FileData] = None, invocation_context: Optional[dict] = None +) -> _FileDataEcho: + return _FileDataEcho(identifier=None if file_data is None else file_data.identifier) + + +@pytest.mark.parametrize("body", [None, {}]) +def test_optional_file_data_is_preserved_as_none(body): + # `file_data` is converted from its dict form for the wrapped function, but it can legitimately + # be absent. Calling `.model_dump()` on None raised before `wrap_fn` ran, turning the + # optional-body contract into a 500 rather than a normal response. + client = TestClient(wrap_in_fastapi(func=_optional_file_data, plugin_id="mock_plugin")) + + kwargs = {} if body is None else {"json": body} + resp = client.post("/invoke", **kwargs) + + assert resp.status_code == 200 + invoke_response = InvokeResponse.model_validate(resp.json()) + invoke_response.generic_validation() + assert invoke_response.output == {"identifier": None} + + +def test_optional_file_data_is_still_converted_when_supplied(): + client = TestClient(wrap_in_fastapi(func=_optional_file_data, plugin_id="mock_plugin")) + + resp = client.post("/invoke", json={"file_data": mock_file_data[0].model_dump()}) + + assert resp.status_code == 200 + assert InvokeResponse.model_validate(resp.json()).output == {"identifier": "mock file data"} + + +def test_no_param_plugin_still_accepts_a_bodyless_post(): + client = TestClient(wrap_in_fastapi(func=_no_params, plugin_id="mock_plugin")) + + resp = client.post("/invoke") + + assert resp.status_code == 200 + assert InvokeResponse.model_validate(resp.json()).output["received"] == "ok" diff --git a/unstructured_platform_plugins/__version__.py b/unstructured_platform_plugins/__version__.py index 3fbbe28..d8f2458 100644 --- a/unstructured_platform_plugins/__version__.py +++ b/unstructured_platform_plugins/__version__.py @@ -1 +1 @@ -__version__ = "0.0.44" # pragma: no cover +__version__ = "0.0.45" # pragma: no cover diff --git a/unstructured_platform_plugins/etl_uvicorn/api_generator.py b/unstructured_platform_plugins/etl_uvicorn/api_generator.py index 06e0074..4b7d8ce 100644 --- a/unstructured_platform_plugins/etl_uvicorn/api_generator.py +++ b/unstructured_platform_plugins/etl_uvicorn/api_generator.py @@ -254,22 +254,48 @@ async def _stream_response(): file_data=request_dict.get("file_data", None), ) - if input_schema_model.model_fields: + async def run_job_with_body(request: BaseModel) -> ResponseType: + log_func_and_body(func=func, body=request.json()) + # Create dictionary from pydantic model while preserving underlying types + request_dict = {f: getattr(request, f) for f in request.model_fields} + # Make sure nested classes get instantiated correctly. `file_data` can legitimately be None + # -- a plugin may declare it optional, and then an absent or partial body leaves it unset -- + # so convert only a real value. Calling `.model_dump()` on None would raise before `wrap_fn` + # runs, turning the optional-body contract into a 500. + file_data = request_dict.get("file_data") + if file_data is not None: + request_dict["file_data"] = file_data_from_dict(file_data.model_dump()) + map_inputs(func=func, raw_inputs=request_dict) + if logger.level == LOG_LEVELS.get("trace", logging.NOTSET): + logger.log(level=logger.level, msg=f"passing inputs to function: {request_dict}") + return await wrap_fn(func=func, kwargs=request_dict) + + # A pydantic body parameter with no default is mandatory even when every field inside the model + # is optional. So a plugin whose parameters are ALL optional would demand a body that no caller + # has a reason to populate -- and before it grew those parameters that same plugin accepted no + # body at all, so adding one flips the HTTP contract while looking backward-compatible. Default + # the body in that case: an absent body resolves each field to its own default, which is exactly + # what the function signature already promises. + # + # A plugin with at least one required field keeps a mandatory body, so an indexer that needs + # `file_data` still fails validation rather than silently receiving None. + body_is_optional = input_schema_model.model_fields and not any( + field.is_required() for field in input_schema_model.model_fields.values() + ) + + if body_is_optional: + + @fastapi_app.post("/invoke", response_model=InvokeResponse) + async def run_job(request: Optional[input_schema_model] = None) -> ResponseType: + return await run_job_with_body( + request if request is not None else input_schema_model() + ) + + elif input_schema_model.model_fields: @fastapi_app.post("/invoke", response_model=InvokeResponse) async def run_job(request: input_schema_model) -> ResponseType: - log_func_and_body(func=func, body=request.json()) - # Create dictionary from pydantic model while preserving underlying types - request_dict = {f: getattr(request, f) for f in request.model_fields} - # Make sure nested classes get instantiated correctly - if "file_data" in request_dict: - request_dict["file_data"] = file_data_from_dict( - request_dict["file_data"].model_dump() - ) - map_inputs(func=func, raw_inputs=request_dict) - if logger.level == LOG_LEVELS.get("trace", logging.NOTSET): - logger.log(level=logger.level, msg=f"passing inputs to function: {request_dict}") - return await wrap_fn(func=func, kwargs=request_dict) + return await run_job_with_body(request) else: