diff --git a/.github/workflows/pr-tests.yml b/.github/workflows/pr-tests.yml index ecd692d..030715b 100644 --- a/.github/workflows/pr-tests.yml +++ b/.github/workflows/pr-tests.yml @@ -4,9 +4,19 @@ name: PR Tests on: pull_request: types: [opened, synchronize, reopened] + # Manual trigger so tests can be run on demand for any ref — including a PR + # whose head commit carries a [skip ci] marker, which GitHub otherwise uses + # to short-circuit pull_request/push runs before the workflow starts. + # workflow_dispatch is not subject to that skip-ci short-circuit. + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to comment coverage on (optional; omit to just run tests)' + required: false + type: string concurrency: - group: pr-tests-${{ github.event.pull_request.number }} + group: pr-tests-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true permissions: @@ -49,6 +59,11 @@ jobs: name: Coverage Report needs: test runs-on: ubuntu-latest + # Only post a PR comment when there is a PR to comment on: either the + # pull_request event, or a manual dispatch that supplied a pr_number. + if: >- + github.event_name == 'pull_request' || + (github.event_name == 'workflow_dispatch' && inputs.pr_number != '') steps: - uses: actions/download-artifact@v4 with: @@ -113,7 +128,7 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - PR_NUMBER=${{ github.event.pull_request.number }} + PR_NUMBER="${{ github.event.pull_request.number || inputs.pr_number }}" MARKER="" # Try to find and update an existing comment first (avoids race where diff --git a/etsy_python/v3/enums/Listing.py b/etsy_python/v3/enums/Listing.py index b8aebf3..71e35b4 100644 --- a/etsy_python/v3/enums/Listing.py +++ b/etsy_python/v3/enums/Listing.py @@ -85,6 +85,10 @@ class SortOrder(Enum): class Includes(Enum): + # SHIPPING and INVENTORY were removed from the includes enum on getListing + # and getListingsByListingIds, but remain valid on getListingsByShop, which + # shares this enum. They are kept for backward compatibility; removing them + # would break callers. May be revisited in the next major version. SHIPPING = "Shipping" IMAGES = "Images" SHOP = "Shop" diff --git a/scripts/audit_sdk.py b/scripts/audit_sdk.py index f54aaba..33fc354 100644 --- a/scripts/audit_sdk.py +++ b/scripts/audit_sdk.py @@ -121,7 +121,16 @@ def get_request_body_schema(op: dict, spec: dict) -> Optional[dict]: def get_spec_enums(spec: dict) -> Dict[str, List[str]]: - """Extract all enum definitions from schemas.""" + """Extract all enum definitions from the spec. + + Covers two locations: + 1. Component schema properties (``components/schemas/.``). + 2. Operation parameters (``.``), including array + parameters whose values live under ``schema.items.enum`` rather than + ``schema.enum``. Parameter-level enums (e.g. the ``includes`` filter on + the listing endpoints) are not defined as component schemas, so + omitting them left those SDK enums entirely unaudited. + """ enums = {} for name, schema in spec.get("components", {}).get("schemas", {}).items(): for prop_name, prop in schema.get("properties", {}).items(): @@ -129,6 +138,27 @@ def get_spec_enums(spec: dict) -> Dict[str, List[str]]: if "enum" in prop: key = f"{name}.{prop_name}" enums[key] = prop["enum"] + + for path, methods in spec.get("paths", {}).items(): + for method, details in methods.items(): + if method not in ("get", "post", "put", "delete", "patch"): + continue + op_id = details.get("operationId", f"{method}:{path}") + for param in details.get("parameters", []): + param = resolve_ref(param, spec) + param_name = param.get("name") + if not param_name: + continue + param_schema = param.get("schema", {}) + # Scalar enum params (schema.enum) and array enum params + # (schema.items.enum) both need to be surfaced. + enum_values = param_schema.get("enum") + if enum_values is None: + items = param_schema.get("items", {}) + if isinstance(items, dict): + enum_values = items.get("enum") + if enum_values is not None: + enums[f"{op_id}.{param_name}"] = enum_values return enums @@ -193,9 +223,17 @@ def scan_resource_methods(resources_dir: Path) -> Dict[str, Dict[str, dict]]: return resources -def scan_enum_values(enums_dir: Path) -> Dict[str, List[str]]: - """Scan enum .py files and extract enum class names and their values.""" - sdk_enums = {} +def scan_enum_values(enums_dir: Path) -> Dict[str, List[List[Any]]]: + """Scan enum .py files and extract enum class names and their values. + + Two distinct enum classes may legitimately share a name across files (e.g. + ``Includes`` in both ``Listing.py`` and ``ListingInventory.py``). Keying a + plain dict by class name would let whichever file sorts last silently + clobber the other, corrupting the comparison. So each name maps to a *list* + of value-lists (one per class definition); the matcher picks the best + overlap per spec enum. + """ + sdk_enums: Dict[str, List[List[Any]]] = {} for py_file in sorted(enums_dir.glob("*.py")): if py_file.name.startswith("__"): continue @@ -215,7 +253,7 @@ def scan_enum_values(enums_dir: Path) -> Dict[str, List[str]]: if isinstance(item.value, ast.Constant): values.append(item.value.value) if values: - sdk_enums[node.name] = values + sdk_enums.setdefault(node.name, []).append(values) return sdk_enums @@ -481,7 +519,7 @@ def _norm_values(values: Any) -> Set[str]: def compute_enum_findings( - spec: dict, sdk_enums: Dict[str, List[str]] + spec: dict, sdk_enums: Dict[str, List[List[Any]]] ) -> List[dict]: """Compare OAS enum definitions against SDK enum classes. @@ -489,33 +527,53 @@ def compute_enum_findings( value sets differ. ``direction`` is ``"missing"`` (present in the spec, absent from the SDK) or ``"extra"`` (present in the SDK, absent from the spec). ``values`` is a set of lower-cased strings. + + ``sdk_enums`` maps each class name to the list of value-lists for every + class of that name (names can collide across files). When the expected name + matches, or when falling back to fuzzy overlap, the candidate value-list + with the greatest overlap against the spec enum is used, so a same-named but + unrelated enum can't hijack the comparison. """ + + def best_candidate(spec_value_set: Set[str], candidates: List[List[Any]]): + """Return the candidate value-list sharing the most values with the spec.""" + best_vals, best_overlap = None, -1 + for vals in candidates: + overlap = len(_norm_values(vals) & spec_value_set) + if overlap > best_overlap: + best_overlap, best_vals = overlap, vals + return best_vals, max(best_overlap, 0) + findings: List[dict] = [] spec_enums = get_spec_enums(spec) for spec_key, spec_values in sorted(spec_enums.items()): prop_name = spec_key.split(".")[-1] if "." in spec_key else spec_key expected_enum_name = snake_to_pascal(prop_name) + spec_value_set = _norm_values(spec_values) best_match = None + matched_values = None if expected_enum_name in sdk_enums: best_match = expected_enum_name + matched_values, _ = best_candidate( + spec_value_set, sdk_enums[expected_enum_name] + ) else: best_overlap = 0 - for sdk_name, sdk_values in sdk_enums.items(): - sdk_value_set = {str(v).lower() for v in sdk_values} - spec_value_set = {str(v).lower() for v in spec_values} - overlap = len(sdk_value_set & spec_value_set) + for sdk_name, candidates in sdk_enums.items(): + vals, overlap = best_candidate(spec_value_set, candidates) + sdk_value_set = _norm_values(vals) if vals else set() smaller_set = min(len(sdk_value_set), len(spec_value_set)) min_required = max(2, smaller_set // 2) if overlap >= min_required and overlap > best_overlap: best_overlap = overlap best_match = sdk_name + matched_values = vals if not best_match: continue - sdk_value_set = {str(v).lower() for v in sdk_enums[best_match]} - spec_value_set = {str(v).lower() for v in spec_values} + sdk_value_set = _norm_values(matched_values) if matched_values else set() missing = spec_value_set - sdk_value_set extra = sdk_value_set - spec_value_set key = f"{spec_key} -> {best_match}" diff --git a/scripts/diff_spec.py b/scripts/diff_spec.py index 3b09cd9..4fd149b 100644 --- a/scripts/diff_spec.py +++ b/scripts/diff_spec.py @@ -75,8 +75,22 @@ def diff_parameters( changes.append( f"type: {old_schema.get('type')} -> {new_schema.get('type')}" ) - if old_schema.get("enum") != new_schema.get("enum"): - changes.append("enum values changed") + # Scalar enum params carry values under schema.enum; array params (e.g. + # the `includes` filter) carry them under schema.items.enum. Check both. + old_enum = old_schema.get("enum", old_schema.get("items", {}).get("enum")) + new_enum = new_schema.get("enum", new_schema.get("items", {}).get("enum")) + if old_enum != new_enum: + added = sorted(set(new_enum or []) - set(old_enum or [])) + removed = sorted(set(old_enum or []) - set(new_enum or [])) + detail = [] + if added: + detail.append(f"added {added}") + if removed: + detail.append(f"removed {removed}") + changes.append( + "enum values changed" + + (f" ({'; '.join(detail)})" if detail else "") + ) if changes: result["changed"].append(f"{name} ({', '.join(changes)})") diff --git a/specs/audit-ignore.json b/specs/audit-ignore.json index 3fc60fd..9080617 100644 --- a/specs/audit-ignore.json +++ b/specs/audit-ignore.json @@ -48,6 +48,38 @@ "values": ["removed"], "reason": "State.REMOVED is kept for backward compatibility and is not in the OAS response schema. Documented inline in enums/Listing.py; may be removed in the next major version.", "added": "2026-06-02" + }, + { + "type": "enum_staleness", + "key": "getListing.includes -> Includes", + "direction": "extra", + "values": ["shipping", "inventory"], + "reason": "Etsy removed 'Shipping' and 'Inventory' from the includes enum on getListing and getListingsByListingIds (still valid on getListingsByShop). The SDK Includes enum keeps both for backward compatibility and because they remain valid on getListingsByShop, which shares the enum; removing them would be a breaking change. Documented inline in enums/Listing.py.", + "added": "2026-07-08" + }, + { + "type": "enum_staleness", + "key": "getListingsByListingIds.includes -> Includes", + "direction": "extra", + "values": ["shipping", "inventory"], + "reason": "Etsy removed 'Shipping' and 'Inventory' from the includes enum on getListing and getListingsByListingIds (still valid on getListingsByShop). The SDK Includes enum keeps both for backward compatibility and because they remain valid on getListingsByShop, which shares the enum; removing them would be a breaking change. Documented inline in enums/Listing.py.", + "added": "2026-07-08" + }, + { + "type": "enum_staleness", + "key": "getListingsByShop.state -> State", + "direction": "extra", + "values": ["removed"], + "reason": "Same State.REMOVED back-compat value as ShopListing.state; surfaced additionally as the state query parameter on getListingsByShop now that parameter-level enums are audited. Documented inline in enums/Listing.py.", + "added": "2026-07-08" + }, + { + "type": "enum_staleness", + "key": "updateHolidayPreferences.holiday_id -> CA_HOLIDAYS", + "direction": "missing", + "values": "*", + "reason": "Same by-design holiday_id handling as ShopHolidayPreference.holiday_id; surfaced additionally as the holiday_id parameter on updateHolidayPreferences now that parameter-level enums are audited. The SDK names US_HOLIDAYS (1-11) and CA_HOLIDAYS (12-23) and documents passing integer IDs directly for other regions. See enums/HolidayPreferences.py.", + "added": "2026-07-08" } ] } diff --git a/specs/baseline.json b/specs/baseline.json index a20325f..a3de734 100644 --- a/specs/baseline.json +++ b/specs/baseline.json @@ -953,20 +953,18 @@ { "name": "includes", "in": "query", - "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shipping', 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Inventory' and 'Personalization'.", + "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Personalization' and 'BuyerPrice'.", "required": false, "schema": { "type": "array", - "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shipping', 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Inventory' and 'Personalization'.", + "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Personalization' and 'BuyerPrice'.", "items": { "type": "string", "enum": [ - "Shipping", "Images", "Shop", "User", "Translations", - "Inventory", "Videos", "Personalization", "BuyerPrice" @@ -2923,20 +2921,18 @@ { "name": "includes", "in": "query", - "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shipping', 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Inventory' and 'Personalization'.", + "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Personalization' and 'BuyerPrice'.", "required": false, "schema": { "type": "array", - "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shipping', 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Inventory' and 'Personalization'.", + "description": "An enumerated string that attaches a valid association. Acceptable inputs are 'Shop', 'Images', 'User', 'Translations', 'Videos', 'Personalization' and 'BuyerPrice'.", "items": { "type": "string", "enum": [ - "Shipping", "Images", "Shop", "User", "Translations", - "Inventory", "Videos", "Personalization", "BuyerPrice" diff --git a/tests/test_audit_ignore.py b/tests/test_audit_ignore.py index 8b09aeb..8bea92e 100644 --- a/tests/test_audit_ignore.py +++ b/tests/test_audit_ignore.py @@ -234,7 +234,7 @@ def test_missing_value_detected(self): "schemas": {"Foo": {"properties": {"color": {"enum": ["red", "green", "blue"]}}}} } } - findings = audit_sdk.compute_enum_findings(spec, {"Color": ["red", "green"]}) + findings = audit_sdk.compute_enum_findings(spec, {"Color": [["red", "green"]]}) assert len(findings) == 1 f = findings[0] assert f["direction"] == "missing" @@ -248,7 +248,7 @@ def test_extra_value_detected(self): } } findings = audit_sdk.compute_enum_findings( - spec, {"Color": ["red", "green", "purple"]} + spec, {"Color": [["red", "green", "purple"]]} ) assert len(findings) == 1 assert findings[0]["direction"] == "extra" @@ -260,7 +260,108 @@ def test_in_sync_yields_no_findings(self): "schemas": {"Foo": {"properties": {"color": {"enum": ["red", "green"]}}}} } } - assert audit_sdk.compute_enum_findings(spec, {"Color": ["red", "green"]}) == [] + assert audit_sdk.compute_enum_findings(spec, {"Color": [["red", "green"]]}) == [] + + def test_duplicate_sdk_enum_name_picks_best_overlap(self): + # Two SDK classes share the name "Includes" (Listing vs ListingInventory). + # The comparison must pick the candidate that actually overlaps the spec + # enum, not whichever was inserted last. + spec = { + "components": { + "schemas": {"Foo": {"properties": {"includes": {"enum": ["a", "b", "c"]}}}} + } + } + sdk_enums = {"Includes": [["listing"], ["a", "b", "c", "d"]]} + findings = audit_sdk.compute_enum_findings(spec, sdk_enums) + # Should match the ["a","b","c","d"] candidate: only "d" is extra. + assert len(findings) == 1 + assert findings[0]["direction"] == "extra" + assert findings[0]["values"] == {"d"} + + +# --------------------------------------------------------------------------- # +# get_spec_enums — parameter-level enum extraction +# --------------------------------------------------------------------------- # +class TestGetSpecEnums: + def test_component_schema_enums_extracted(self): + spec = { + "components": { + "schemas": {"Foo": {"properties": {"color": {"enum": ["red", "green"]}}}} + } + } + assert audit_sdk.get_spec_enums(spec) == {"Foo.color": ["red", "green"]} + + def test_scalar_parameter_enum_extracted(self): + spec = { + "paths": { + "/x": { + "get": { + "operationId": "getX", + "parameters": [ + {"name": "state", "schema": {"enum": ["active", "draft"]}} + ], + } + } + } + } + assert audit_sdk.get_spec_enums(spec) == {"getX.state": ["active", "draft"]} + + def test_array_parameter_items_enum_extracted(self): + # The `includes` filter is an array param: values live under + # schema.items.enum, which was previously ignored entirely. + spec = { + "paths": { + "/x": { + "get": { + "operationId": "getX", + "parameters": [ + { + "name": "includes", + "schema": { + "type": "array", + "items": {"enum": ["Shop", "User"]}, + }, + } + ], + } + } + } + } + assert audit_sdk.get_spec_enums(spec) == {"getX.includes": ["Shop", "User"]} + + def test_parameter_without_enum_ignored(self): + spec = { + "paths": { + "/x": { + "get": { + "operationId": "getX", + "parameters": [ + {"name": "limit", "schema": {"type": "integer"}} + ], + } + } + } + } + assert audit_sdk.get_spec_enums(spec) == {} + + +# --------------------------------------------------------------------------- # +# scan_enum_values — duplicate class names across files +# --------------------------------------------------------------------------- # +class TestScanEnumValues: + def test_same_name_across_files_kept_separately(self, tmp_path): + (tmp_path / "A.py").write_text( + "from enum import Enum\n\nclass Includes(Enum):\n SHOP = 'Shop'\n", + encoding="utf-8", + ) + (tmp_path / "B.py").write_text( + "from enum import Enum\n\nclass Includes(Enum):\n LISTING = 'Listing'\n", + encoding="utf-8", + ) + result = audit_sdk.scan_enum_values(tmp_path) + assert "Includes" in result + # Both definitions retained, neither clobbered. + assert sorted(result["Includes"], key=lambda v: v[0]) == [["Listing"], ["Shop"]] # --------------------------------------------------------------------------- #