Using field's Json name instead of internal conversion from snake cas…

[dsborets]

I'm proposing to use internal FieldDescriptor JSON name (Descriptors.fieldNameToJsonName(..) under the hood) instead of using custom snake to camel case converters, so it will be one source of truth for the field naming. As for now we already have multiple repeated places in the code to do a transformation and one time we missed that already.

P.S. Not sure that I found all places in the code where name getting should be modified as well.

[coveralls]

Pull Request Test Coverage Report for Build 127

• 9 of 16 (56.25%) changed or added relevant lines in 2 files are covered.
• 25 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.4%) to 42.22%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rejoiner/src/main/java/com/google/api/graphql/rejoiner/ProtoToGql.java	7	14	50.0%
<!--	Total:	9	16	56.25%	-->

Files with Coverage Reduction	New Missed Lines	%
rejoiner/src/main/java/com/google/api/graphql/rejoiner/ProtoToGql.java	1	51.13%
rejoiner/src/main/java/com/google/api/graphql/rejoiner/SchemaModule.java	24	81.66%
<!--	Total:	25		-->

Totals
Change from base Build 104:	0.4%
Covered Lines:	814
Relevant Lines:	1928

---

💛 - Coveralls

[siderakis]

I like this idea.

Some users use camel case in their photo files and want to keep the case. by default I think this change would change camel case field name to lowercase. It looks like they could annotate their field with json_name proto field option.

Generates JSON objects. Message field names are mapped to lowerCamelCase and become JSON object keys. If the json_name field option is specified, the specified value will be used as the key instead.

https://developers.google.com/protocol-buffers/docs/proto3#json

https://github.com/protocolbuffers/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/Descriptors.java#L1238-L1242

https://github.com/protocolbuffers/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/Descriptors.java#L1210-L1225

[dsborets]

@siderakis Exactly. We will be able to define field name as part of initial proto file using [json_name="..."]

[arc-008]

@siderakis Any idea when this fix will be released ?

[dsborets]

Hi @siderakis. Could you please make a release with this fix ASAP. We really need it in our projects and I would like to avoid switching to our forked version

[siderakis]

Looking at merging this now

[siderakis]

I'm looking into merging this now

[siderakis]

I'm looking into merging this now. This seems to be incomplete, sorry the tests didn't catch this automatically. It looks like the ProtoDataFetcher needs to be updated.

[siderakis]

I'm looking into merging this now. This seems to be incomplete, sorry the tests didn't catch this automatically. It looks like the ProtoDataFetcher needs to be updated. I will continue looking into it and add more tests.

[siderakis]

I'm looking into merging this now. This seems to be incomplete, sorry the tests didn't catch this automatically. It looks like the ProtoDataFetcher needs to be updated. I will continue looking into it and add more tests.

[siderakis]

Hey, I had some time to run the changes with the example projects and it looks like this PR is incomplete. The ProtoDataFetcher needs to be updated. Sorry the tests didn't catch this. I'll continue looking into it.

[siderakis]

I'm looking into merging this now. This seems to be incomplete, sorry the tests didn't catch this automatically. It looks like the ProtoDataFetcher needs to be updated. I will continue looking into it and add more tests.

There are actually 3 names to consider, the field name in the proto file, the name in the generated java code (needed in order to call the method), and the name that's used in the GraphQL shema. This change should only change the name in the GraphQL schema.

[siderakis]

I'm looking into merging this now. This seems to be incomplete, sorry the tests didn't catch this automatically. It looks like the ProtoDataFetcher needs to be updated. I will continue looking into it and add more tests.

There are actually 3 names to consider, the field name in the proto file, the name in the generated java code (needed in order to call the method), and the name that's used in the GraphQL shema. This change should only change the name in the GraphQL schema.

[siderakis]

There are actually 3 names to consider, the field name in the proto file, the name in the generated java code (needed in order to call the method), and the name that's used in the GraphQL shema. This change should only change the name in the GraphQL schema.

This PR is changing the name used in ProtoDataFetcher , sorry the tests didn't catch this automatically. It looks like the ProtoDataFetcher needs to be updated. I will continue looking into it and add more tests.

[dsborets]

Could you please point me to the exact code that you mentioned? And especially test-cases that should be passed. When I start using the changes from this PR, I didn't see any issues. We have to change field naming gRPC->GraphQL to convert something like order_id to orderId (and also do not rename orderId (gRPC) ) and back from GraphQL query to gRPC. I see couple of issues in existing releases:

1. I still have to use snake_case field name in my GraphQL request event if it returns camel-case back as a response: getOrder(order_id:...) { order { orderId } }

2. If I have orderInfo field in my gRPC it transforms to orderinfo (all lower-case) and I can use it in the request, BUT it doesn't return any result back for orderId (because of unable to find appropriate gRPC field orderinfo)

So, all these should be fixed.

Please let me know if you need more information or you have any thought about it

[dsborets]

Sorry, closed by mistake, wrong button pressed. Reopening...

[siderakis]

The case that I saw breaking was when the json_name annotation was used. For example I changed Shelf from the example shelf.proto to include:

repeated string book_ids = 3 [json_name = "amazingBookIds"];

[dsborets]

Sorry for the delay, I was a little busy. Ok... This should cover all cases now, I hope.

In the code ProtoToGgl.apply ideally we shouldn't do ... LOWER_CAMEL_TO_UPPER.convert( UNDERSCORE_TO_CAMEL.convert(fieldDescriptor.getName()) ) ... but instead exactly what com.google.protobuf.Descriptors.fieldNameToJsonName does. Unfortunately that function is private. We can copy it into this project (they also relay on ToJsonName() function in C++, according to the comments).

[dsborets]

Hi @siderakis. Did you have a chance to take a look on the last commit?

[dsborets]

Hi @siderakis. Do you have a plan to merge this PR and release a new version soon? It will be great, because we have to use our own forked version of the project for now.

[siderakis]

I released josn name support for fields in output types in v 0.2.0. input types aren't supported yet.