-
Notifications
You must be signed in to change notification settings - Fork 144
Using field's Json name instead of internal conversion from snake cas… #54
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 127
💛 - Coveralls |
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.
https://developers.google.com/protocol-buffers/docs/proto3#json |
@siderakis Exactly. We will be able to define field name as part of initial proto file using |
@siderakis Any idea when this fix will be released ? |
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 |
Looking at merging this now |
I'm looking into merging this now |
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'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. |
1 similar comment
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. |
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. |
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. |
1 similar comment
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. |
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. |
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
So, all these should be fixed. Please let me know if you need more information or you have any thought about it |
Sorry, closed by mistake, wrong button pressed. Reopening... |
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:
|
…on logic to cover the case of using `[json_name="some_name"]` in .proto file so we can still call a getter by name. Plus some performance enhancements - convert fields name during application startup time only.
Sorry for the delay, I was a little busy. In the code |
Hi @siderakis. Did you have a chance to take a look on the last commit? |
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. |
I released josn name support for fields in output types in v 0.2.0. input types aren't supported yet. |
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.