fix(rest): make REST connector E2E + unit tests hermetic, harden OpenAPI parser#27835
fix(rest): make REST connector E2E + unit tests hermetic, harden OpenAPI parser#27835mohittilala wants to merge 5 commits intomainfrom
Conversation
github-actions
Bot
added
Ingestion
safe to test
There was a problem hiding this comment.
Pull request overview
This PR aims to make REST connector E2E + unit tests hermetic (no dependency on external petstore3.swagger.io) and to harden OpenAPI parsing by rejecting non-object parse results early.
Changes:
- Updated Playwright REST service form automation to use a bundled OpenAPI schema fixture file path instead of a live URL.
- Updated REST unit tests to patch
get_connectionsoRestSource.create(...)doesn’t perform network calls. - Hardened OpenAPI parsing helpers to raise
OpenAPIParseErrorwhen parsed content is not a mapping/object.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/playwright/support/entity/ingestion/ApiIngestionClass.ts | Switch REST connector E2E form fill from external URL to local OpenAPI fixture path. |
| ingestion/tests/unit/topology/api/test_rest.py | Patch get_connection in unit tests to avoid network during RestSource.create(...). |
| ingestion/src/metadata/ingestion/source/api/rest/parser.py | Add _ensure_mapping guard and apply it across HTTP/file/S3 OpenAPI parsing paths. |
| ingestion/examples/openapi/sample.json | Add a bundled OpenAPI fixture used by the E2E flow. |
Comments suppressed due to low confidence (1)
ingestion/tests/unit/topology/api/test_rest.py:441
- The new
get_connectionpatch inRESTTest.__init__only affects construction ofself.rest_source. This test module still has otherRestSource.create(...)calls (e.g., intest_endpoint_filter_pattern) that are only patchingApiServiceSource.test_connectionand will still execute the realget_connection(and hithttps://petstore3.swagger.io/...) duringRestSource.create, so the suite is not fully hermetic yet. Consider patchingmetadata.ingestion.source.api.api_service.get_connectionfor those tests as well (or overriding the config to use the local fixture).
@patch(
"metadata.ingestion.source.api.api_service.get_connection",
return_value=MOCK_JSON_RESPONSE,
)
@patch("metadata.ingestion.source.api.api_service.ApiServiceSource.test_connection")
def __init__(self, methodName, test_connection, get_connection) -> None: # noqa: N803
super().__init__(methodName)
test_connection.return_value = False
self.config = OpenMetadataWorkflowConfig.model_validate(mock_rest_config)
self.rest_source = RestSource.create(
mock_rest_config["source"],
self.config.workflowConfig.openMetadataServerConfig,
)
Jest test CoverageUI tests summary
|
🟡 Playwright Results — all passed (12 flaky)✅ 3986 passed · ❌ 0 failed · 🟡 12 flaky · ⏭️ 86 skipped
🟡 12 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| logger.debug("Successfully parsed OpenAPI schema as JSON") | ||
| return parsed # noqa: TRY300 | ||
| except json.JSONDecodeError: | ||
| except (json.JSONDecodeError, OpenAPIParseError): |
There was a problem hiding this comment.
In parse_openapi_schema's fallback path, _ensure_mapping(json.loads(...), "JSON") raising OpenAPIParseError (e.g., body is a valid JSON array/scalar) is currently caught and treated the same as JSONDecodeError, which then logs "Content is not valid JSON" and tries YAML. Since the JSON parse already succeeded, retrying YAML can’t fix the shape and also discards the more specific error message. Consider letting OpenAPIParseError bubble up here (or at least logging a more accurate message like "JSON is not an object") and skipping the YAML fallback in this case.
| except (json.JSONDecodeError, OpenAPIParseError): | |
| except json.JSONDecodeError: |
Code Review ✅ Approved 1 resolved / 1 findingsHardens the OpenAPI parser by introducing robust error handling and makes REST connector tests fully hermetic by replacing network dependencies with local fixtures. Resolved an issue where OpenAPIParseError could escape fallback blocks. ✅ 1 resolved✅ Edge Case: OpenAPIParseError escapes fallback try/except blocks
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
|
Describe your changes:
Make the REST connector tests stop depending on petstore3.swagger.io and reject malformed OpenAPI responses up front.
/home/airflow/ingestion/examples/openapi/sample.json(a fixture bundled into the ingestion container) instead of a live URL.get_connectionsoRestSource.createno longer hits the network.parse_openapi_schema,parse_openapi_schema_from_file, andparse_openapi_schema_from_s3raiseOpenAPIParseErrorwhen parsing returns a value that isn't a dict (yaml.safe_loadreturns a bare string for HTML error pages, which previously leaked through to callers).Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Bug fix
Summary by Gitar
parse_openapi_schema_from_fileandparse_openapi_schema_from_s3to include the file path or S3 URI inOpenAPIParseErrormessages._ensure_mappingto verify validation of input types likelist,str, andNone.test_openapi_parser.pycovering file path reporting for invalid JSON/YAML schemas.This will update automatically on new commits.