[low-code] convert request.body to a dict when converting to AirbyteLogMessage #20557

girarda · 2022-12-16T00:58:51Z

What

The SimpleRetriever sends the request body as a string while the server expects it to be a map
Resolves connector builder server does not handle request body #20505

How

If the request body is set as body_json, the string is a json object encoded as a string
If the request body is set as body_data, the string is of the form "k1=v1&k2=v"
Either, the SimpleRetriever should convert the string to a dict so it can be interpreted by the connector builder

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

girarda · 2022-12-16T00:59:00Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

@@ -408,20 +408,34 @@ def state(self, value: StreamState):
    def parse_records_and_emit_request_and_responses(self, request, response, stream_slice, stream_state) -> Iterable[StreamData]:
        # Only emit requests and responses when running in debug mode
        if self.logger.isEnabledFor(logging.DEBUG):
-            yield self._create_trace_message_from_request(request)
-            yield self._create_trace_message_from_response(response)
+            yield prepared_request_to_airbyte_message(request)


girarda · 2022-12-16T00:59:39Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

+        "url": request.url,
+        "http_method": request.method,
+        "headers": dict(request.headers),
+        "body": _body_binary_string_to_dict(request.body),


instead of assigning the body directly, delegate to_body_binary_string_to_dict to convert it to an Optional[Mapping]

girarda · 2022-12-16T00:59:52Z

airbyte-connector-builder-server/unit_tests/connector_builder/impl/test_default_api.py

@@ -155,6 +155,7 @@ def test_read_stream():
    request = {
        "url": "https://demonslayers.com/api/v1/hashiras?era=taisho",
        "headers": {"Content-Type": "application/json"},
+        "http_method": "GET",


fixes the tests

we'll be able to merge this PR when this is merged in so CI fails on broken tests #20217

Thanks for pointing that out - was looking into it for a PR I'm working on right now 😅

girarda · 2022-12-16T01:00:13Z

airbyte-connector-builder-server/unit_tests/connector_builder/impl/test_default_api.py

@@ -409,6 +422,11 @@ def test_read_stream_returns_error_if_stream_does_not_exist():
            HttpRequest(url="https://nichirin.com/v1/swords", headers={"field": "name"}, body={"key": "value"}, http_method="PUT"),
            id="test_create_request_with_no_parameters",
        ),
+        pytest.param(
+            'request:{"url": "https://nichirin.com/v1/swords", "http_method": "POST", "headers": {"field": "name"}, "body":null}',


add a test where "body": null for peace of mind

girarda · 2022-12-16T01:18:46Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

+        return None
+
+
+def response_to_airbyte_message(response: requests.Response) -> AirbyteMessage:


moved to a function for consistency and ease of testing

Those methods aren't used anywhere else except from the tests though. Should we indicate them as private using the _ prefix?

Saying that makes me think it is weird that parse_records_and_emit_request_and_responses is public as well since it's only used internally by the class. My main concern is that we have someone external using it which would bring a dependency to manage when we want to change this. This happened for #20019 and prevented us to do a breaking change we wanted to do

good call. prepended both with _

girarda · 2022-12-16T03:00:51Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

-        return AirbyteMessage(type=MessageType.LOG, log=AirbyteLogMessage(level=Level.INFO, message=log_message))
+
+def prepared_request_to_airbyte_message(request: requests.PreparedRequest) -> AirbyteMessage:
+    # FIXME: this should return some sort of trace message


need to keep them FIXMEs for now 😞

maxi297 · 2022-12-16T14:06:03Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

+    return AirbyteMessage(type=MessageType.LOG, log=AirbyteLogMessage(level=Level.INFO, message=log_message))
+
+
+def _body_binary_string_to_dict(body_str) -> Optional[Mapping[str, str]]:


Should we have typing for this parameter?

maxi297 · 2022-12-16T14:49:24Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/retrievers/simple_retriever.py

+        return None
+
+
+def response_to_airbyte_message(response: requests.Response) -> AirbyteMessage:


Those methods aren't used anywhere else except from the tests though. Should we indicate them as private using the _ prefix?

Saying that makes me think it is weird that parse_records_and_emit_request_and_responses is public as well since it's only used internally by the class. My main concern is that we have someone external using it which would bring a dependency to manage when we want to change this. This happened for #20019 and prevented us to do a breaking change we wanted to do

clnoll

LGTM (pending resolution of Maxime's comments) @girarda!

girarda · 2022-12-21T23:02:39Z

airbyte-connector-builder-server/connector_builder/impl/default_api.py

-            raise HTTPException(status_code=400, detail=f"Could not perform read with with error: {error.args[0]}")
+            raise HTTPException(
+                status_code=400,
+                detail=f"Could not perform read with with error: {error.args[0]}\n{''.join(traceback.TracebackException.from_exception(error).format())}",


@flash1293 FYI you can rebase from this branch if you need the stacktraces. will merge in after the holidays

This reverts commit 0912538.

This reverts commit b6f62d6.

girarda added 2 commits December 15, 2022 16:52

convert request body

2398adf

fix tests

93aabe7

octavia-squidington-iv added the CDK Connector Development Kit label Dec 16, 2022

girarda commented Dec 16, 2022

View reviewed changes

girarda temporarily deployed to more-secrets December 16, 2022 01:01 — with GitHub Actions Inactive

girarda added 2 commits December 15, 2022 17:03

test body data

6d8ccd7

more tests

0adf45c

girarda temporarily deployed to more-secrets December 16, 2022 01:14 — with GitHub Actions Inactive

girarda temporarily deployed to more-secrets December 16, 2022 01:15 — with GitHub Actions Inactive

girarda commented Dec 16, 2022

View reviewed changes

more tests

c5547b8

girarda temporarily deployed to more-secrets December 16, 2022 01:35 — with GitHub Actions Inactive

girarda temporarily deployed to more-secrets December 16, 2022 01:36 — with GitHub Actions Inactive

girarda changed the title ~~Alex/request body~~ [low-code] convert request.body to a dict when converting to AirbyteLogMessage Dec 16, 2022

girarda requested review from brianjlai and clnoll December 16, 2022 02:59

girarda marked this pull request as ready for review December 16, 2022 03:00

girarda requested a review from a team as a code owner December 16, 2022 03:00

girarda commented Dec 16, 2022

View reviewed changes

maxi297 reviewed Dec 16, 2022

View reviewed changes

clnoll approved these changes Dec 16, 2022

View reviewed changes

_

f40a56c

girarda temporarily deployed to more-secrets December 20, 2022 01:36 — with GitHub Actions Inactive

return stacktrace

84bd5dc

girarda commented Dec 21, 2022

View reviewed changes

girarda temporarily deployed to more-secrets December 21, 2022 23:02 — with GitHub Actions Inactive

girarda temporarily deployed to more-secrets December 21, 2022 23:03 — with GitHub Actions Inactive

pretty print

0912538

girarda added 3 commits December 21, 2022 23:05

Revert "pretty print"

b6f62d6

This reverts commit 0912538.

Revert "Revert "pretty print""

a6d0a93

This reverts commit b6f62d6.

replace \n

fa4ea8b

girarda temporarily deployed to more-secrets December 22, 2022 07:10 — with GitHub Actions Inactive

girarda temporarily deployed to more-secrets December 22, 2022 07:11 — with GitHub Actions Inactive

Merge branch 'master' into alex/request_body

dcb8390

girarda temporarily deployed to more-secrets January 1, 2023 22:05 — with GitHub Actions Inactive

girarda temporarily deployed to more-secrets January 1, 2023 22:06 — with GitHub Actions Inactive

missing type hint

3452bba

girarda temporarily deployed to more-secrets January 2, 2023 22:37 — with GitHub Actions Inactive

girarda temporarily deployed to more-secrets January 2, 2023 22:38 — with GitHub Actions Inactive

girarda merged commit 2f2e530 into master Jan 3, 2023

girarda deleted the alex/request_body branch January 3, 2023 09:45

flash1293 mentioned this pull request Jan 4, 2023

Connector builder server: Return stack trace on uncaught exception for read command #20714

Closed

This was referenced Jan 6, 2023

Bump Airbyte version from 0.40.26 to 0.40.27 #21092

Closed

Bump Airbyte version from 0.40.26 to 0.40.27 #21121

Closed

github-actions bot mentioned this pull request Jan 6, 2023

Bump helm chart version reference to 0.43.15 #21125

Closed

This was referenced Jan 6, 2023

Bump Airbyte version from 0.40.26 to 0.40.27 #21130

Closed

Bump Airbyte version from 0.40.26 to 0.40.27 #21135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[low-code] convert request.body to a dict when converting to AirbyteLogMessage #20557

[low-code] convert request.body to a dict when converting to AirbyteLogMessage #20557

girarda commented Dec 16, 2022 •

edited

Loading

girarda Dec 16, 2022

girarda Dec 16, 2022

girarda Dec 16, 2022 •

edited

Loading

girarda Dec 16, 2022

clnoll Dec 16, 2022

girarda Dec 16, 2022

girarda Dec 16, 2022

maxi297 Dec 16, 2022

girarda Dec 20, 2022

girarda Dec 16, 2022

maxi297 Dec 16, 2022

maxi297 Dec 16, 2022

clnoll left a comment

girarda Dec 21, 2022

		return None


		def response_to_airbyte_message(response: requests.Response) -> AirbyteMessage:

		return AirbyteMessage(type=MessageType.LOG, log=AirbyteLogMessage(level=Level.INFO, message=log_message))


		def _body_binary_string_to_dict(body_str) -> Optional[Mapping[str, str]]:

[low-code] convert request.body to a dict when converting to AirbyteLogMessage #20557

[low-code] convert request.body to a dict when converting to AirbyteLogMessage #20557

Conversation

girarda commented Dec 16, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda commented Dec 16, 2022 •

edited

Loading

girarda Dec 16, 2022 •

edited

Loading