Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show http method in the connector builder test panel request tab #20109

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

clnoll
Copy link
Contributor

@clnoll clnoll commented Dec 6, 2022

What

Adds the HTTP method (GET/PUT/etc..) of the outgoing request to the test panel request tab.

Fixes #19622.

image

How

  1. Updated the HttpRequest object in openapi.yaml to include an http_method property, whose values can be GET, POST, PUT, or PATCH, per the airbyte-cdk's list of BODY_REQUEST_METHODS for sources.
  2. Regenerated the models in http_request.py by running ./gradlew :airbyte-connector-builder-server:generateOpenApiPythonServer from the airbyte repo.
  3. Updated default_api.py's _create_request_from_log_message method to include the http_method key from the request.

Note: we are currently unable to enforce the enum type specified in the openapi.yml, due to an open issue with python-fastapi's handling of enums. This means that theoretically we could see methods not supported by airbyte-cdk. It looks to me like openapi's default python generator does support enums, but switching our generator felt heavy handed for this simple change. We can revisit later if we start requiring more enums.

Recommended reading order

  1. airbyte-connector-builder-server/src/main/openapi/openapi.yaml
  2. airbyte-connector-builder-server/connector_builder/generated/models/http_request.py
  3. airbyte-connector-builder-server/connector_builder/impl/default_api.py
  4. airbyte-connector-builder-server/unit_tests/connector_builder/impl/test_default_api.py

馃毃 User Impact 馃毃

No breaking changes. When submitting a test read request, the user will now see the http method that was used along with other information about the request, in the "Request" tab.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

  • Unit tests added and passing.
  • Issue acceptance criteria met
  • Documentation updated
  • PR name follows PR naming conventions
  • Code reviews completed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

@clnoll clnoll requested a review from girarda December 6, 2022 00:21
"""

url: str
parameters: Optional[Dict[str, Any]] = None
body: Optional[Dict[str, Any]] = None
headers: Optional[Dict[str, Any]] = None
http_method: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this a required field? I can't imagine a request without an HTTP method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me @girarda. I made this change. I'm still using .get on the request (with a default "") - I assume that if there is a case where http_method is missing we don't want to throw an error in the UI. But let me know if you think otherwise.

@clnoll clnoll temporarily deployed to more-secrets December 6, 2022 13:55 Inactive
@clnoll clnoll temporarily deployed to more-secrets December 6, 2022 13:56 Inactive
@clnoll clnoll force-pushed the connector-builder-show-http-method branch from b4f5181 to 9f47ef8 Compare December 6, 2022 15:31
@clnoll clnoll temporarily deployed to more-secrets December 6, 2022 15:33 Inactive
@clnoll clnoll temporarily deployed to more-secrets December 6, 2022 15:34 Inactive
@clnoll clnoll force-pushed the connector-builder-show-http-method branch from 9f47ef8 to 55de64a Compare December 6, 2022 17:55
@clnoll clnoll temporarily deployed to more-secrets December 6, 2022 17:57 Inactive
@clnoll clnoll temporarily deployed to more-secrets December 6, 2022 17:58 Inactive
@clnoll clnoll merged commit 4429968 into master Dec 6, 2022
@clnoll clnoll deleted the connector-builder-show-http-method branch December 6, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Connector Builder UI] show HTTP request method in the test panel request tab
2 participants