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

Fix grpc transcoding bugs for unknown fields #797

Merged

Conversation

qiwzhang
Copy link
Contributor

Signed-off-by: Wayne Zhang qiwzhang@google.com

Fixes #795.

It is based on #796

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Copy link
Contributor Author

@bnordli

Please review this change

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@bnordli
Copy link

bnordli commented May 16, 2020

LGTM!

@qiwzhang qiwzhang requested review from JLXIA, nareddyt and TAOXUY May 16, 2020 00:21
@qiwzhang
Copy link
Contributor Author

nareddyt
nareddyt previously approved these changes May 17, 2020
Copy link
Contributor

@nareddyt nareddyt left a comment

Choose a reason for hiding this comment

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

LGTM (but see if we can add a t test)

qiwzhang added 2 commits May 16, 2020 20:40
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@@ -481,6 +486,27 @@ types {
}
syntax: SYNTAX_PROTO3
}
types {
name: "endpoints.examples.bookstore.UnknownBook"
fields {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, I manually deleted "unknown" field and "UknownMsg" from the service config.

my $response1 = ApiManager::http_get($NginxPort,'/unknownbook?key=this-is-an-api-key');

# With bug in https://github.com/cloudendpoints/esp/issues/795, the first field
# after the uknown is discarded. Once the bug is fixed, use follow correct result.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the fix, the first field "known_int" is discarded. With the fix, all fields are there

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
nareddyt
nareddyt previously approved these changes May 18, 2020
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang merged commit b840c62 into cloudendpoints:master May 18, 2020
@qiwzhang qiwzhang deleted the Exabel-zero-copy-stream-skip-and-count branch May 18, 2020 19:45
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.

gRPC transcoder may truncate response data for unknown proto fields
4 participants