-
Notifications
You must be signed in to change notification settings - Fork 184
Fix incorrect return annotation on StructuredData.get() #895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| from enum import Enum | ||
| from typing import Annotated, Any, Generic, Self, TypeVar | ||
| from typing import Annotated, Any, Generic, Self, TypeVar, cast | ||
|
|
||
| import requests | ||
| from pydantic import BaseModel, Field, RootModel, model_serializer, model_validator | ||
|
|
@@ -91,13 +91,22 @@ def serialize_model(self): | |
| result["data"] = self.data | ||
| return result | ||
|
|
||
| def get(self) -> TBaseModel: | ||
| """Get the extracted data, raising ScrapeFailedError if extraction failed.""" | ||
| def get(self) -> TBaseModel | dict[str, Any]: | ||
| """Get the extracted data, raising ScrapeFailedError if extraction failed. | ||
|
|
||
| Returns the validated model when the data was extracted against a user-provided | ||
| schema. When no schema was provided the payload is stored as a `DictBaseModel` | ||
| (i.e. `RootModel`) wrapper and this returns the unwrapped plain JSON object, which | ||
| is *not* a `BaseModel`. Callers must handle both shapes, e.g. by guarding with | ||
| `isinstance(data, BaseModel)` before calling `model_dump()`. | ||
| """ | ||
| if not self.success or self.data is None: | ||
| raise ScrapeFailedError(self.error or "Unknown extraction error") | ||
| if isinstance(self.data, RootModel): | ||
| return self.data.root # type: ignore[attr-defined] | ||
| return self.data | ||
| # local alias: keeps the narrowed type checkable instead of `RootModel[Unknown]` | ||
| data: TBaseModel | DictBaseModel = self.data | ||
| if isinstance(data, RootModel): | ||
| return cast(dict[str, Any], data.root) | ||
| return data | ||
|
Comment on lines
+94
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Represent accepted list payloads in the public result contract.
Also synchronize the corresponding 📍 Affects 3 files
🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class DataSpace(BaseModel): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import pytest | ||
| from notte_core.data.space import DataSpace, StructuredData | ||
| from notte_core.errors.processing import ScrapeFailedError | ||
| from pydantic import BaseModel, RootModel | ||
|
|
||
|
|
||
| class Verification(BaseModel): | ||
| status: str | ||
| code: str | None = None | ||
|
|
||
|
|
||
| def test_get_returns_plain_dict_for_dict_payload(): | ||
| """A dict payload is wrapped in a RootModel, so `get()` returns the unwrapped dict.""" | ||
| structured = StructuredData(success=True, data={"status": "found", "code": "463092"}) | ||
|
|
||
| assert isinstance(structured.data, RootModel) | ||
| data = structured.get() | ||
| assert isinstance(data, dict) | ||
| assert not isinstance(data, BaseModel) | ||
| assert data == {"status": "found", "code": "463092"} | ||
|
|
||
|
|
||
| def test_get_returns_model_for_model_payload(): | ||
| """A model payload is returned as-is, so `get()` returns a `BaseModel`.""" | ||
| structured = StructuredData[Verification](success=True, data=Verification(status="found", code="463092")) | ||
|
|
||
| data = structured.get() | ||
| assert isinstance(data, Verification) | ||
| assert data.model_dump() == {"status": "found", "code": "463092"} | ||
|
|
||
|
|
||
| def test_get_returns_plain_dict_for_deserialized_response(): | ||
| """Structured data deserialized from a JSON response also yields a plain dict.""" | ||
| space = DataSpace.model_validate( | ||
| { | ||
| "markdown": "# page", | ||
| "structured": {"success": True, "error": None, "data": {"status": "found", "code": "463092"}}, | ||
| } | ||
| ) | ||
|
|
||
| assert space.structured is not None | ||
| data = space.structured.get() | ||
| assert isinstance(data, dict) | ||
| assert data == {"status": "found", "code": "463092"} | ||
|
|
||
|
|
||
| def test_get_raises_on_failed_extraction(): | ||
| structured = StructuredData[Verification](success=False, error="boom", data=None) | ||
|
|
||
| with pytest.raises(ScrapeFailedError): | ||
| _ = structured.get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When structured extraction produces a top-level JSON list, this cast advertises the unchanged list root as
dict[str, Any], allowing callers to type-check dictionary operations such as.items()or**unpacking that then fail at runtime.Knowledge Base Used:
Prompt To Fix With AI