google / rejoiner

Generates a unified GraphQL schema from gRPC microservices and other Protobuf sources
https://google.github.io/rejoiner/
Apache License 2.0
3.67k stars 144 forks source link

Fix GqlInputConverter#createProtoBuf() bug. #53

Closed jaslong closed 5 years ago

jaslong commented 5 years ago

Bug was introduced in 6a9323c0c5a9c84e59fcf95f5df16c3e74c2469a. This commit aimed to convert underscore_case field names to camelCase. While the GraphQLInputObjectField's name was converted, GqlInputConverter#createProtoBuf(), which converts Map<String, Object> to a protobuf Message, did not do the same conversion. This resulted in fields like "id" working, but fields like "user_id" being ignored.

This change fixes the bug and adds an assertion that all input fields were consumed.

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 102


Changes Missing Coverage Covered Lines Changed/Added Lines %
rejoiner/src/main/java/com/google/api/graphql/rejoiner/GqlInputConverter.java 9 11 81.82%
<!-- Total: 9 11 81.82% -->
Totals Coverage Status
Change from base Build 94: 0.04%
Covered Lines: 809
Relevant Lines: 1933

💛 - Coveralls
jaslong commented 5 years ago

I signed it!

googlebot commented 5 years ago

CLAs look good, thanks!

jaslong commented 5 years ago

Pinging @siderakis -- can I get a review on this please?

siderakis commented 5 years ago

Hey, thanks for the PR, overall looks good! Only question is, why require all fields to be consumed?

jaslong commented 5 years ago

I assumed that there is a validation step before this that ensures the correct shape of types. This means that we should always consume all fields. Do you think this is a fair assumption to make? Having this assertion prevents similar bugs in the future.

On Fri, Sep 28, 2018 at 6:06 AM Nick Siderakis notifications@github.com wrote:

Hey, thanks for the PR, overall looks good! Only question is, why require all fields to be consumed?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/rejoiner/pull/53#issuecomment-425429676, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwqpEFiCk80hU5MjXefQVjvW_Opeujkks5ufh7rgaJpZM4W5jcU .

siderakis commented 5 years ago

It seems like a reasonable assumption. If input type modifications are added later we can revisit if needed.

siderakis commented 5 years ago

Thanks again!