diff --git a/CLAUDE.md b/CLAUDE.md index 3fe751a..31f8649 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,7 @@ All code lives under `etsy_python/v3/`: Every resource is a `@dataclass` with `session: EtsyClient`. Methods return `Union[Response, RequestException]`. - **GET/DELETE**: Parameters passed as `**kwargs`, built into query string via `generate_get_uri()` +- **Required ID-list params** (`listing_ids`, `payment_ids`, `shop_section_ids`, ...): join with `join_ids(name, ids)` from `common/Utils.py`. It raises `ValueError` for an empty list and `TypeError` for a string, locally and before any request, so these methods can raise those in addition to `RequestException` - **POST/PUT/PATCH**: Payload is a `Request` model, serialized via `.get_dict()` to JSON - **File uploads**: Payload is a `FileRequest`, sent as multipart form data - **Enums**: Always use `.value` to extract the API string (e.g., `State.ACTIVE.value` -> `"active"`) @@ -130,6 +131,7 @@ Every resource is a `@dataclass` with `session: EtsyClient`. Methods return `Uni ### Utility Functions (`common/Utils.py`) - `generate_get_uri(uri, **kwargs)` -- builds query string from kwargs dict +- `join_ids(name, ids)` -- validates and comma-joins a required ID list (raises `ValueError` if empty, `TypeError` if a string) - `todict(obj)` -- recursive serializer handling Enums, nested objects, nullable fields - `generate_bytes_from_file(file)` -- reads file bytes for uploads @@ -210,5 +212,6 @@ value-bearing types); to stop accepting it, delete the entry. A missing file mea "no suppressions". Supported `type` values: `extra_method`, `enum_staleness`, `param_drift`, -`body_drift`, `code_issue`. Prefer an explicit `values` list over `"*"` — a wildcard +`body_drift`, `code_issue`. `param_drift` directions are `missing`, `extra`, and `optional` +(required in the spec but defaulted to `None` in the SDK). Prefer an explicit `values` list over `"*"` — a wildcard hides everything on that key, including drift nobody has reviewed. diff --git a/etsy_python/v3/common/Utils.py b/etsy_python/v3/common/Utils.py index 50eeb79..6bd71e0 100644 --- a/etsy_python/v3/common/Utils.py +++ b/etsy_python/v3/common/Utils.py @@ -1,6 +1,6 @@ import warnings from enum import Enum -from typing import Any, Dict, List, Optional +from typing import Any, Dict, Iterable, List, Optional def warn_removed_legacy_param(operation_id: str) -> None: @@ -25,6 +25,21 @@ def warn_removed_legacy_param(operation_id: str) -> None: ) +def join_ids(name: str, ids: Iterable[Any]) -> str: + """Join a required list of IDs into Etsy's comma-separated query value. + + Raises ``TypeError`` for a string (iterating it would split the ID into + characters) and ``ValueError`` when no IDs are given, so a request Etsy + would reject is caught before it is sent. + """ + if isinstance(ids, (str, bytes)): + raise TypeError(f"{name} must be a list of IDs, not a string") + joined = ",".join(map(str, ids)) if ids is not None else "" + if not joined: + raise ValueError(f"{name} is required and must contain at least one ID") + return joined + + def generate_get_uri(uri: str, params: Optional[Dict[str, Any]] = None) -> str: if not params: return uri diff --git a/etsy_python/v3/resources/Listing.py b/etsy_python/v3/resources/Listing.py index 6a03aaa..5cc9c36 100644 --- a/etsy_python/v3/resources/Listing.py +++ b/etsy_python/v3/resources/Listing.py @@ -2,7 +2,7 @@ from dataclasses import dataclass from typing import Optional, List, Dict, Any, Union -from etsy_python.v3.common.Utils import warn_removed_legacy_param +from etsy_python.v3.common.Utils import join_ids, warn_removed_legacy_param from etsy_python.v3.enums.Listing import Includes, State, SortOn, SortOrder from etsy_python.v3.exceptions.RequestException import RequestException from etsy_python.v3.models.Listing import ( @@ -150,7 +150,7 @@ def get_listings_by_listing_ids( ) -> Union[Response, RequestException]: endpoint = "/listings/batch" query_params: Dict[str, Any] = { - "listing_ids": ",".join(list(map(str, listing_ids))), + "listing_ids": join_ids("listing_ids", listing_ids), "includes": ",".join(list(map(lambda inc: inc.value, includes))) if includes is not None else None, @@ -179,7 +179,7 @@ def get_listings_inventory_by_listing_ids( ) -> Union[Response, RequestException]: endpoint = "/listings/batch/inventory" query_params: Dict[str, Any] = { - "listing_ids": ",".join(list(map(str, listing_ids))), + "listing_ids": join_ids("listing_ids", listing_ids), } return self.session.make_request(endpoint, query_params=query_params) @@ -188,7 +188,7 @@ def get_listings_shipping_by_listing_ids( ) -> Union[Response, RequestException]: endpoint = "/listings/batch/shipping" query_params: Dict[str, Any] = { - "listing_ids": ",".join(list(map(str, listing_ids))), + "listing_ids": join_ids("listing_ids", listing_ids), } return self.session.make_request(endpoint, query_params=query_params) @@ -260,18 +260,21 @@ def get_listings_by_shop_return_policy( def get_listings_by_shop_section_id( self, shop_id: int, - shop_section_ids: Optional[List[int]] = None, + shop_section_ids: List[int], limit: int = 25, offset: int = 0, sort_on: SortOn = SortOn.CREATED, sort_order: SortOrder = SortOrder.DESC, legacy: Optional[bool] = None, ) -> Union[Response, RequestException]: + """Get listings in the given shop sections. + + Raises ``ValueError`` if ``shop_section_ids`` is empty and + ``TypeError`` if it is a string, before any request is sent. + """ endpoint = f"/shops/{shop_id}/shop-sections/listings" query_params: Dict[str, Any] = { - "shop_section_ids": ",".join(list(map(str, shop_section_ids))) - if shop_section_ids is not None - else None, + "shop_section_ids": join_ids("shop_section_ids", shop_section_ids), "limit": limit, "offset": offset, "sort_on": sort_on.value, diff --git a/etsy_python/v3/resources/Payment.py b/etsy_python/v3/resources/Payment.py index 85feda1..6675fa1 100644 --- a/etsy_python/v3/resources/Payment.py +++ b/etsy_python/v3/resources/Payment.py @@ -2,6 +2,7 @@ from dataclasses import dataclass from typing import Union, Dict, Any, List +from etsy_python.v3.common.Utils import join_ids from etsy_python.v3.exceptions.RequestException import RequestException from etsy_python.v3.resources.Session import EtsyClient from etsy_python.v3.resources.Response import Response @@ -16,7 +17,7 @@ def get_payment_account_ledger_entry_payments( ) -> Union[Response, RequestException]: endpoint = f"/shops/{shop_id}/payment-account/ledger-entries/payments" query_params: Dict[str, Any] = { - "ledger_entry_ids": ",".join(list(map(str, ledger_entry_ids))) + "ledger_entry_ids": join_ids("ledger_entry_ids", ledger_entry_ids) } return self.session.make_request(endpoint, query_params=query_params) @@ -42,5 +43,5 @@ def get_payments( self, shop_id: int, payment_ids: List[int] ) -> Union[Response, RequestException]: endpoint = f"/shops/{shop_id}/payments" - query_params: Dict[str, Any] = {"payment_ids": ",".join(list(map(str, payment_ids)))} + query_params: Dict[str, Any] = {"payment_ids": join_ids("payment_ids", payment_ids)} return self.session.make_request(endpoint, query_params=query_params) diff --git a/scripts/audit_sdk.py b/scripts/audit_sdk.py index 39ab442..1888b35 100644 --- a/scripts/audit_sdk.py +++ b/scripts/audit_sdk.py @@ -192,6 +192,16 @@ def scan_resource_methods(resources_dir: Path) -> Dict[str, Dict[str, dict]]: if arg.annotation and isinstance(arg.annotation, ast.Name): param_annotations[arg.arg] = arg.annotation.id + # Params whose default is the literal None (defaults + # align with the tail of the positional args). + args = item.args.args + defaults = item.args.defaults + none_defaults = { + arg.arg + for arg, default in zip(args[len(args) - len(defaults):], defaults) + if isinstance(default, ast.Constant) and default.value is None + } + # Detect NotImplementedError stubs is_not_implemented = False if len(item.body) == 1: @@ -211,6 +221,7 @@ def scan_resource_methods(resources_dir: Path) -> Dict[str, Dict[str, dict]]: file_methods[item.name] = { "params": params, "param_annotations": param_annotations, + "none_defaults": none_defaults, "class": node.name, "file": py_file.name, "line": item.lineno, @@ -629,7 +640,9 @@ def compute_param_findings( Yields one finding per (operation, direction) whose parameter sets differ. ``direction`` is ``"missing"`` (in the spec, absent from the SDK signature) - or ``"extra"`` (in the SDK signature, absent from the spec). ``values`` is a + or ``"extra"`` (in the SDK signature, absent from the spec) or + ``"optional"`` (required in the spec, but the SDK defaults it to ``None``, + so omitting it silently sends a request Etsy rejects). ``values`` is a set of parameter names, so an ignore listing specific names suppresses only those — a newly drifted parameter on an already-suppressed operation still surfaces. Model-object and path params are excluded from the comparison. @@ -653,8 +666,15 @@ def compute_param_findings( non_model_sdk_params = sdk_params - model_param_names spec_only = spec_params - non_model_sdk_params sdk_only = non_model_sdk_params - spec_params - PATH_PARAM_NAMES - - for direction, values in (("missing", spec_only), ("extra", sdk_only)): + required_but_optional = { + p["name"] for p in op["parameters"] if p.get("required") + } & set(sdk.get("none_defaults", ())) + + for direction, values in ( + ("missing", spec_only), + ("extra", sdk_only), + ("optional", required_but_optional), + ): if values: findings.append( { @@ -1017,6 +1037,9 @@ def generate_report( if "extra" in directions: names = ", ".join(sorted(directions["extra"]["values"])) lines.append(f"- In SDK but not spec: {names}") + if "optional" in directions: + names = ", ".join(sorted(directions["optional"]["values"])) + lines.append(f"- Required in spec but defaults to None in SDK: {names}") lines.append("") else: lines.append("No query/path parameter drift detected.\n") diff --git a/tests/test_audit_ignore.py b/tests/test_audit_ignore.py index 15dea4b..5152e2e 100644 --- a/tests/test_audit_ignore.py +++ b/tests/test_audit_ignore.py @@ -333,13 +333,20 @@ def test_ambiguous_suffix_matches_are_not_guessed(self): # compute_param_findings — query/path parameter drift # --------------------------------------------------------------------------- # class TestComputeParamFindings: - def _implemented(self, spec_params, sdk_params, annotations=None): + def _implemented( + self, spec_params, sdk_params, annotations=None, required=(), none_defaults=() + ): return { "getX": { - "spec": {"parameters": [{"name": n} for n in spec_params]}, + "spec": { + "parameters": [ + {"name": n, "required": n in required} for n in spec_params + ] + }, "sdk": { "params": list(sdk_params), "param_annotations": annotations or {}, + "none_defaults": set(none_defaults), "file": "X.py", "line": 10, }, @@ -396,6 +403,35 @@ def test_path_params_excluded(self): ) assert findings == [] + def test_required_param_defaulting_to_none_detected(self): + findings = audit_sdk.compute_param_findings( + self._implemented( + ["ids", "limit"], + ["ids", "limit"], + required={"ids"}, + none_defaults={"ids", "limit"}, + ), + {}, + ) + assert len(findings) == 1 + assert findings[0]["direction"] == "optional" + assert findings[0]["values"] == {"ids"} + + def test_required_param_without_default_not_flagged(self): + findings = audit_sdk.compute_param_findings( + self._implemented(["ids"], ["ids"], required={"ids"}), {} + ) + assert findings == [] + + def test_scan_records_none_defaults(self, tmp_path): + (tmp_path / "X.py").write_text( + "class R:\n" + " def get_x(self, a: int, b=None, c: int = 0, d=None):\n" + " pass\n" + ) + info = audit_sdk.scan_resource_methods(tmp_path)["X"]["get_x"] + assert info["none_defaults"] == {"b", "d"} + # --------------------------------------------------------------------------- # # partition_findings — param_drift value verification diff --git a/tests/test_listing_resource.py b/tests/test_listing_resource.py index a711cab..35f6082 100644 --- a/tests/test_listing_resource.py +++ b/tests/test_listing_resource.py @@ -583,16 +583,50 @@ def test_with_section_ids(self, mock_session): qp = mock_session.make_request.call_args[1]["query_params"] assert qp["shop_section_ids"] == "111,222" - def test_without_section_ids(self, mock_session): - mock_session.make_request.return_value = Response( - 200, make_shop_listing_collection() - ) + def test_section_ids_required(self, mock_session): + """Etsy marks shop_section_ids as required, so omitting it is a TypeError.""" resource = ListingResource(session=mock_session) - resource.get_listings_by_shop_section_id(MOCK_SHOP_ID) + with pytest.raises(TypeError): + resource.get_listings_by_shop_section_id(MOCK_SHOP_ID) - qp = mock_session.make_request.call_args[1]["query_params"] - assert qp["shop_section_ids"] is None + mock_session.make_request.assert_not_called() + + @pytest.mark.parametrize("section_ids", [None, []]) + def test_section_ids_none_or_empty_raises(self, mock_session, section_ids): + """None or [] would send an invalid request, so reject it before calling Etsy.""" + resource = ListingResource(session=mock_session) + + with pytest.raises(ValueError, match="shop_section_ids"): + resource.get_listings_by_shop_section_id(MOCK_SHOP_ID, section_ids) + + mock_session.make_request.assert_not_called() + + def test_string_section_ids_raises(self, mock_session): + """A string would be split into characters and sent as bogus IDs.""" + resource = ListingResource(session=mock_session) + + with pytest.raises(TypeError, match="shop_section_ids"): + resource.get_listings_by_shop_section_id(MOCK_SHOP_ID, "111,222") + + mock_session.make_request.assert_not_called() + + +@pytest.mark.parametrize( + "method", + [ + "get_listings_by_listing_ids", + "get_listings_inventory_by_listing_ids", + "get_listings_shipping_by_listing_ids", + ], +) +def test_batch_listing_methods_reject_empty_ids(mock_session, method): + resource = ListingResource(session=mock_session) + + with pytest.raises(ValueError, match="listing_ids"): + getattr(resource, method)([]) + + mock_session.make_request.assert_not_called() class TestErrorPropagation: diff --git a/tests/test_payment_resource.py b/tests/test_payment_resource.py index 4e8ce20..ef2c47b 100644 --- a/tests/test_payment_resource.py +++ b/tests/test_payment_resource.py @@ -1,3 +1,5 @@ +import pytest + from etsy_python.v3.resources.Payment import PaymentResource from etsy_python.v3.resources.Response import Response @@ -32,6 +34,14 @@ def test_payment_ids_joined(self, mock_session): qp = mock_session.make_request.call_args[1]["query_params"] assert qp["payment_ids"] == f"{MOCK_PAYMENT_ID},20202" + def test_empty_payment_ids_raises(self, mock_session): + resource = PaymentResource(session=mock_session) + + with pytest.raises(ValueError, match="payment_ids"): + resource.get_payments(MOCK_SHOP_ID, []) + + mock_session.make_request.assert_not_called() + class TestGetShopPaymentAccountLedgerEntryPayments: def test_ledger_entry_ids_joined(self, mock_session): @@ -50,3 +60,11 @@ def test_ledger_entry_ids_joined(self, mock_session): == f"/shops/{MOCK_SHOP_ID}/payment-account/ledger-entries/payments" ) assert call_args[1]["query_params"]["ledger_entry_ids"] == f"{MOCK_LEDGER_ENTRY_ID},30303" + + def test_empty_ledger_entry_ids_raises(self, mock_session): + resource = PaymentResource(session=mock_session) + + with pytest.raises(ValueError, match="ledger_entry_ids"): + resource.get_payment_account_ledger_entry_payments(MOCK_SHOP_ID, []) + + mock_session.make_request.assert_not_called() diff --git a/tests/test_utils.py b/tests/test_utils.py index 4ab4bf7..a78a2b1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,12 +1,32 @@ from enum import Enum +import pytest + from etsy_python.v3.common.Utils import ( generate_bytes_from_file, generate_get_uri, + join_ids, todict, ) +class TestJoinIds: + @pytest.mark.parametrize("ids", [[1, 2, 3], (1, 2, 3), (i for i in (1, 2, 3))]) + def test_joins_any_iterable(self, ids): + assert join_ids("ids", ids) == "1,2,3" + + @pytest.mark.parametrize("ids", [None, [], (), iter([])]) + def test_empty_raises_value_error(self, ids): + with pytest.raises(ValueError, match="ids is required"): + join_ids("ids", ids) + + @pytest.mark.parametrize("ids", ["111,222", b"111"]) + def test_string_raises_type_error(self, ids): + # Iterating a string would send "1,1,1,,,2,2,2". + with pytest.raises(TypeError, match="not a string"): + join_ids("ids", ids) + + class TestGenerateGetUri: def test_no_params(self): assert generate_get_uri("/shops/123") == "/shops/123"