-
Notifications
You must be signed in to change notification settings - Fork 0
fix(openapi): enforce stable unique operationIds #337
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
Closed
+326
−0
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
bb38513
test(openapi): add RED operationId contract
seonghobae f4b08dc
feat(openapi): validate stable unique operationIds
seonghobae 9e60d83
Merge branch 'main' into fix/openapi-operationid-main
opencode-agent[bot] 4454922
test(openapi): cover nested and commented operationIds
seonghobae b9e0525
fix(openapi): reject nested and commented operationId ambiguity
seonghobae 5778999
Merge branch 'main' into fix/openapi-operationid-main
opencode-agent[bot] 5d72b44
test(openapi): bind operationId contract to CI execution
seonghobae 13e4a4a
Merge branch 'main' into fix/openapi-operationid-main
opencode-agent[bot] 7062e14
Merge branch 'main' into fix/openapi-operationid-main
opencode-agent[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| #!/usr/bin/env python3 | ||
| """Inspect the buyer OpenAPI path table for stable, unique operation identifiers. | ||
|
|
||
| This checker intentionally uses only the Python standard library so the repository's | ||
| buyer-readiness gate does not need a second YAML runtime. It recognizes the narrowly | ||
| formatted top-level ``paths`` table owned by this repository and treats only standard | ||
| HTTP method keys as operations. It does not attempt to be a general-purpose YAML parser. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from dataclasses import dataclass | ||
|
|
||
|
|
||
| HTTP_METHODS = frozenset({"get", "put", "post", "delete", "options", "head", "patch", "trace"}) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ContractViolation: | ||
| """One deterministic operationId contract violation.""" | ||
|
|
||
| code: str | ||
| detail: str | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class InspectionResult: | ||
| """Operations found in the path table and any contract violations.""" | ||
|
|
||
| operations: list[tuple[str, str, str | None]] | ||
| violations: list[ContractViolation] | ||
|
|
||
|
|
||
| def _indent_width(line: str) -> int: | ||
| """Return leading-space indentation and reject tab-indented structure.""" | ||
|
|
||
| prefix = line[: len(line) - len(line.lstrip(" \t"))] | ||
| if "\t" in prefix: | ||
| raise ValueError("OpenAPI contract must use spaces for structural indentation") | ||
| return len(prefix) | ||
|
|
||
|
|
||
| def _yaml_scalar(value: str) -> str: | ||
| """Return the simple scalar form used by repository-owned operationId values. | ||
|
|
||
| YAML plain scalars may contain ``#`` as data when it is not preceded by | ||
| separation whitespace. A separated ``#`` starts a comment and is therefore | ||
| excluded from the operation identifier. Quoted scalar support remains narrow: | ||
| the complete trimmed value must be enclosed by one matching quote pair. | ||
| """ | ||
|
|
||
| value = value.strip() | ||
| if not value: | ||
| return "" | ||
|
|
||
| if value[0] in {"'", '"'}: | ||
| if len(value) < 2 or value[-1] != value[0]: | ||
| raise ValueError("unsupported quoted operationId scalar") | ||
| return value[1:-1] | ||
|
|
||
| for index, character in enumerate(value): | ||
| if character == "#" and (index == 0 or value[index - 1].isspace()): | ||
| return value[:index].rstrip() | ||
| return value | ||
|
|
||
|
|
||
| def inspect_operation_ids(contract: str) -> InspectionResult: | ||
| """Inspect standard HTTP methods under the top-level OpenAPI ``paths`` mapping. | ||
|
|
||
| The repository-owned contract keeps path keys at two spaces, method keys at four | ||
| spaces, and direct method properties at six spaces. Path-level metadata such as | ||
| ``parameters`` or ``$ref`` is ignored because it is not an HTTP operation, and | ||
| nested extension metadata cannot satisfy the direct ``operationId`` contract. | ||
| """ | ||
|
|
||
| operations: list[tuple[str, str, str | None]] = [] | ||
| violations: list[ContractViolation] = [] | ||
| first_use: dict[str, tuple[str, str]] = {} | ||
|
|
||
| in_paths = False | ||
| current_path: str | None = None | ||
| current_method: str | None = None | ||
| current_operation_id: str | None = None | ||
|
|
||
| def finish_operation() -> None: | ||
| nonlocal current_method, current_operation_id | ||
| if current_path is None or current_method is None: | ||
| return | ||
|
|
||
| method = current_method.upper() | ||
| operation_id = current_operation_id | ||
| operations.append((method, current_path, operation_id)) | ||
|
|
||
| if not operation_id: | ||
| violations.append(ContractViolation( | ||
| code="missing_operation_id", | ||
| detail=f"{method} {current_path} does not declare operationId", | ||
| )) | ||
| else: | ||
| previous = first_use.get(operation_id) | ||
| if previous is None: | ||
| first_use[operation_id] = (method, current_path) | ||
| else: | ||
| previous_method, previous_path = previous | ||
| violations.append(ContractViolation( | ||
| code="duplicate_operation_id", | ||
| detail=( | ||
| f"operationId '{operation_id}' is used by " | ||
| f"{previous_method} {previous_path} and {method} {current_path}" | ||
| ), | ||
| )) | ||
|
|
||
| current_method = None | ||
| current_operation_id = None | ||
|
|
||
| for raw_line in contract.splitlines(): | ||
| stripped = raw_line.strip() | ||
| if not stripped or stripped.startswith("#"): | ||
| continue | ||
|
|
||
| indent = _indent_width(raw_line) | ||
|
|
||
| if not in_paths: | ||
| if indent == 0 and stripped == "paths:": | ||
| in_paths = True | ||
| continue | ||
|
|
||
| if indent == 0: | ||
| finish_operation() | ||
| break | ||
|
|
||
| if indent == 2 and stripped.endswith(":"): | ||
| finish_operation() | ||
| key = stripped[:-1].strip() | ||
| if key.startswith("/"): | ||
| current_path = key | ||
| else: | ||
| current_path = None | ||
| continue | ||
|
|
||
| if current_path is None: | ||
| continue | ||
|
|
||
| if indent == 4 and stripped.endswith(":"): | ||
| finish_operation() | ||
| key = stripped[:-1].strip().lower() | ||
| if key in HTTP_METHODS: | ||
| current_method = key | ||
| continue | ||
|
|
||
| if current_method is not None and indent == 6 and stripped.startswith("operationId:"): | ||
| _, value = stripped.split(":", 1) | ||
| candidate = _yaml_scalar(value) | ||
| current_operation_id = candidate or None | ||
|
|
||
| finish_operation() | ||
| return InspectionResult(operations=operations, violations=violations) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| #!/usr/bin/env python3 | ||
| """Contract tests for stable, unique buyer OpenAPI operation identifiers.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| from scripts.openapi_operation_id_contract import ContractViolation, inspect_operation_ids | ||
|
|
||
|
|
||
| REPOSITORY_ROOT = Path(__file__).resolve().parents[1] | ||
| OPENAPI_PATH = REPOSITORY_ROOT / "docs/deployment/clearfolio-buyer-connector.openapi.yaml" | ||
| CI_WORKFLOW = REPOSITORY_ROOT / ".github/workflows/ci.yml" | ||
|
|
||
|
|
||
| class OpenApiOperationIdContractTest(unittest.TestCase): | ||
| """Keep every shipped HTTP operation addressable by one stable operationId.""" | ||
|
|
||
| def test_ci_executes_buyer_readiness_script_contracts(self) -> None: | ||
| """The operationId contract must remain reachable from exact-head CI.""" | ||
|
|
||
| workflow = CI_WORKFLOW.read_text(encoding="utf-8") | ||
|
|
||
| self.assertIn("python -m pytest -q scripts", workflow) | ||
|
|
||
| def test_current_buyer_contract_has_unique_operation_ids(self) -> None: | ||
| """The repository-owned buyer contract must contain no missing or duplicate IDs.""" | ||
|
|
||
| contract = OPENAPI_PATH.read_text(encoding="utf-8") | ||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertGreater(len(result.operations), 0) | ||
| self.assertEqual([], result.violations) | ||
|
|
||
| def test_duplicate_operation_id_is_rejected(self) -> None: | ||
| """Two HTTP operations must never share the same generated-client identity.""" | ||
|
|
||
| contract = """openapi: 3.0.3 | ||
| paths: | ||
| /api/v1/jobs: | ||
| get: | ||
| operationId: readJob | ||
| /api/v1/items: | ||
| post: | ||
| operationId: readJob | ||
| components: {} | ||
| """ | ||
|
|
||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertIn( | ||
| ContractViolation( | ||
| code="duplicate_operation_id", | ||
| detail="operationId 'readJob' is used by GET /api/v1/jobs and POST /api/v1/items", | ||
| ), | ||
| result.violations, | ||
| ) | ||
|
|
||
| def test_inline_comments_do_not_change_duplicate_operation_identity(self) -> None: | ||
| """YAML comments must not make the same operationId look distinct.""" | ||
|
|
||
| contract = """openapi: 3.0.3 | ||
| paths: | ||
| /api/v1/jobs: | ||
| get: | ||
| operationId: readJob # first operation | ||
| /api/v1/items: | ||
| post: | ||
| operationId: readJob # second operation | ||
| components: {} | ||
| """ | ||
|
|
||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertIn( | ||
| ContractViolation( | ||
| code="duplicate_operation_id", | ||
| detail="operationId 'readJob' is used by GET /api/v1/jobs and POST /api/v1/items", | ||
| ), | ||
| result.violations, | ||
| ) | ||
|
|
||
| def test_missing_operation_id_is_rejected(self) -> None: | ||
| """Every path-level HTTP method must declare an explicit operationId.""" | ||
|
|
||
| contract = """openapi: 3.0.3 | ||
| paths: | ||
| /api/v1/jobs: | ||
| parameters: [] | ||
| get: | ||
| summary: Read a job | ||
| components: {} | ||
| """ | ||
|
|
||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertIn( | ||
| ContractViolation( | ||
| code="missing_operation_id", | ||
| detail="GET /api/v1/jobs does not declare operationId", | ||
| ), | ||
| result.violations, | ||
| ) | ||
|
|
||
| def test_comment_only_operation_id_is_missing(self) -> None: | ||
| """A comment after an empty scalar must not satisfy the operationId contract.""" | ||
|
|
||
| contract = """openapi: 3.0.3 | ||
| paths: | ||
| /api/v1/jobs: | ||
| get: | ||
| operationId: # required for generated clients | ||
| components: {} | ||
| """ | ||
|
|
||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertIn( | ||
| ContractViolation( | ||
| code="missing_operation_id", | ||
| detail="GET /api/v1/jobs does not declare operationId", | ||
| ), | ||
| result.violations, | ||
| ) | ||
|
|
||
| def test_nested_extension_operation_id_does_not_satisfy_operation(self) -> None: | ||
| """Only a method's direct operationId property may satisfy the contract.""" | ||
|
|
||
| contract = """openapi: 3.0.3 | ||
| paths: | ||
| /api/v1/jobs: | ||
| get: | ||
| x-metadata: | ||
| operationId: nestedOnly | ||
| summary: Read a job | ||
| components: {} | ||
| """ | ||
|
|
||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertIn( | ||
| ContractViolation( | ||
| code="missing_operation_id", | ||
| detail="GET /api/v1/jobs does not declare operationId", | ||
| ), | ||
| result.violations, | ||
| ) | ||
|
|
||
|
seonghobae marked this conversation as resolved.
|
||
| def test_non_http_path_keys_do_not_create_operations(self) -> None: | ||
| """OpenAPI path-level metadata is not mistaken for an HTTP operation.""" | ||
|
|
||
| contract = """openapi: 3.0.3 | ||
| paths: | ||
| /api/v1/jobs/{jobId}: | ||
| parameters: [] | ||
| get: | ||
| operationId: readJob | ||
| components: {} | ||
| """ | ||
|
|
||
| result = inspect_operation_ids(contract) | ||
|
|
||
| self.assertEqual([("GET", "/api/v1/jobs/{jobId}", "readJob")], result.operations) | ||
| self.assertEqual([], result.violations) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
|
seonghobae marked this conversation as resolved.
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
P2 Inline YAML comments on path keys silently disable the operationId check for that path (fail-open)
scripts/openapi_operation_id_contract.py:157python -m pytest -q scriptsorpython -m unittest scripts.test_openapi_operation_id_contractto verify.Suggested diff