Skip to content

Grpc transcoding supports preserve proto name #685

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

Merged
merged 6 commits into from
Sep 3, 2019

Conversation

qiwzhang
Copy link
Contributor

@qiwzhang qiwzhang commented Sep 3, 2019

Added a flag in start_esp --transcoding_preserve_proto_field_names. If it is true, grpc transcoding will preserve proto names.

@qiwzhang qiwzhang force-pushed the preserve_proto_name branch from 1194c4b to ecac98d Compare September 3, 2019 21:31
@qiwzhang qiwzhang requested review from JLXIA and nareddyt September 3, 2019 21:32
@nareddyt
Copy link
Contributor

nareddyt commented Sep 3, 2019

Fixes #682

ok(ApiManager::verify_http_json_response($response1,
{'shelfId'=>'100', }), "The shelf_id 1 was echoed successfully.");
ok(ApiManager::verify_http_json_response($response2,
{'shelfId'=>'100', }), "The shelfId 2 was echoed successfully.");
Copy link
Contributor

Choose a reason for hiding this comment

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

-> 2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


ok(ApiManager::verify_http_json_response($response1,
{'shelfId'=>'100', }), "The shelf_id 1 was echoed successfully.");
ok(ApiManager::verify_http_json_response($response2,
Copy link
Contributor

Choose a reason for hiding this comment

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

-> 1, to keep it consistent with the description?

Same for the following one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -177,6 +177,9 @@ ngx_esp_request_ctx_s::ngx_esp_request_ctx_s(ngx_http_request_t *r,
if (lc->esp->get_always_print_primitive_fields()) {
json_print_options.always_print_primitive_fields = true;
}
if (lc->esp->get_preserve_proto_field_names()) {
json_print_options.preserve_proto_field_names = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the transcoding lib does support this feature, what needs to be done here is just pass in the configuration?

Should we do this for CloudESF as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

nareddyt
nareddyt previously approved these changes Sep 3, 2019
@qiwzhang qiwzhang merged commit aef5aa5 into cloudendpoints:master Sep 3, 2019
@qiwzhang qiwzhang deleted the preserve_proto_name branch September 3, 2019 23:59
@email2liyang
Copy link

which release contains this fix? I just tried latest version 1.43.0 it does not support it yet

@JLXIA
Copy link
Contributor

JLXIA commented Dec 24, 2019

From our release note, it has been included since 1.39.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants