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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support JSON requests over HTTP #28

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Support JSON requests over HTTP #28

merged 2 commits into from
Jun 24, 2021

Conversation

taral
Copy link
Contributor

@taral taral commented Jun 24, 2021

This change is Reviewable

@taral taral requested a review from burk June 24, 2021 14:07
Copy link
Contributor

@burk burk left a comment

Choose a reason for hiding this comment

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

Reviewed 43 of 43 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @taral)


exabel_data_sdk/client/api/error_handler.py, line 46 at r1 (raw file):

    if status_code == HTTPStatus.SERVICE_UNAVAILABLE:
        return ErrorType.UNAVAILABLE
    if status_code == HTTPStatus.GATEWAY_TIMEOUT:

(there are some more here: https://github.com/cloudendpoints/esp/blob/master/src/api_manager/utils/status.cc#L320 but I don't know if they are required)


exabel_data_sdk/client/api/api_client/http/base_http_client.py, line 31 at r1 (raw file):

    @overload
    def _request(self, method: str, url: str, response_proto: None, body: Message = None) -> None:
        ...

Oh, I haven't seen this before. Cool!


exabel_data_sdk/client/api/api_client/http/relationship_http_client.py, line 48 at r1 (raw file):

    def get_relationship(self, request: GetRelationshipRequest) -> Relationship:
        # Since GetRelationship has the same url as ListRelationships, these requests cannot be
        # distinguished by the server, and we get a ListRelationshipsResponse.

(Sounds like a bug in the REST mapping)


exabel_data_sdk/client/api/data_classes/signal.py, line 25 at r1 (raw file):

        self,
        name: str,
        entity_type: str,

(would it be better to deprecate it but leave it with a default value of None to not break existing code? I guess it's not so important as we're our biggest user ourselves..)

Copy link
Contributor

@burk burk left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @taral)


exabel_data_sdk/client/api/api_client/http/base_http_client.py, line 31 at r1 (raw file):

Previously, burk (Bjørn Rustad) wrote…

Oh, I haven't seen this before. Cool!

In the example on the webpage, the actual implementation of the method has no type annotation at all though?

Copy link
Contributor Author

@taral taral left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @burk)


exabel_data_sdk/client/api/error_handler.py, line 46 at r1 (raw file):

Previously, burk (Bjørn Rustad) wrote…

(there are some more here: https://github.com/cloudendpoints/esp/blob/master/src/api_manager/utils/status.cc#L320 but I don't know if they are required)

I selected the error codes that I see we use in the Data API server, so I think this is sufficient.


exabel_data_sdk/client/api/api_client/http/base_http_client.py, line 31 at r1 (raw file):

Previously, burk (Bjørn Rustad) wrote…

In the example on the webpage, the actual implementation of the method has no type annotation at all though?

Mypy complains if I remove them.


exabel_data_sdk/client/api/api_client/http/relationship_http_client.py, line 48 at r1 (raw file):

Previously, burk (Bjørn Rustad) wrote…

(Sounds like a bug in the REST mapping)

Yes, the proto method definition is accompanied by a TODO.


exabel_data_sdk/client/api/data_classes/signal.py, line 25 at r1 (raw file):

Previously, burk (Bjørn Rustad) wrote…

(would it be better to deprecate it but leave it with a default value of None to not break existing code? I guess it's not so important as we're our biggest user ourselves..)

It’s a bit awkward to deprecate the second argument in a list of four mandatory arguments. Since the fix is just to remove an argument, I don’t think it is a great burden on clients to make the change right away.

Copy link
Contributor

@burk burk left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


exabel_data_sdk/client/api/data_classes/signal.py, line 25 at r1 (raw file):

Previously, taral wrote…

It’s a bit awkward to deprecate the second argument in a list of four mandatory arguments. Since the fix is just to remove an argument, I don’t think it is a great burden on clients to make the change right away.

Ah, true.

@taral taral merged commit 3b6a83a into Exabel:main Jun 24, 2021
@taral taral deleted the json-support branch June 24, 2021 15:06
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.

2 participants