protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

[PHP] Serialize message to json add include default values and preserve fields name

Open s2x opened this issue 3 years ago • 9 comments

s2x avatar Dec 15 '22 10:12 s2x

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Dec 15 '22 10:12 google-cla[bot]

Also please rebase, the testing infrastructure has changed a lot in the last few weeks. Sorry for the delay on this PR.

haberman avatar Mar 06 '23 22:03 haberman

Piotrze @s2x any updates on this?

knobik avatar Dec 05 '23 12:12 knobik

Piotrze @s2x any updates on this?

Hello @knobik , I finally managed to find some time to finish it. Here is a list of fixes that have been added since the last commit:

Warning for boolean parameter in serializeToJsonString Added a warning to users informing them about incorrect use of the boolean parameter in the serializeToJsonString function.

Unify preserve_proto_field_names and json_name Unified behavior of preserve_proto_field_names and json_name options between php-ext and php, eliminating ambiguities. If json_name was provided in the proto file, it will always be used.

Adding the JsonSerializeOptions class to php-ext A new class, JsonSerializeOptions, has been introduced in php-ext, allowing more advanced configuration of JSON serialization options. Previously it was only in the composer package.

Test extension Unit tests have been expanded with new scenarios, covering the changes introduced, in order to more fully check their correctness.

s2x avatar Jan 23 '24 07:01 s2x

This PR makes several changes to the semantics of JSON serialization. We have to be careful, because we have to keep these semantics the same across languages.

None of our other languages currently serialize default values when serializing to JSON. So I don't think we can make that change in PHP.

This PR also is changing the behavior of the upb JSON serializer, so that JSON name is always used if it is explicitly provided, even if a user asked for proto field names. We cannot change this behavior, as it would cause divergence from what other languages do.

For example, here is the C++ logic for preserve_proto_field_names. Note that it always uses the proto field name if the option is set; it doesn't look to see if a json_name was set: https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/src/google/protobuf/json/internal/unparser.cc#L409-L410

haberman avatar Feb 01 '24 16:02 haberman

Hey, thanks for the quick reply. I read your comment, but I'm not sure if I understood it correctly.

This PR makes several changes to the semantics of JSON serialization. We have to be careful, because we have to keep these semantics the same across languages.

None of our other languages currently serialize default values when serializing to JSON. So I don't think we can make that change in PHP.

Unfortunately, I don't really understand what the problem is. To make the default values serialized, you must provide the parameter JsonSerializeOptions::EMIT_DEFAULTS in serializeToJsonString function. Without this parameter, only non-empty fields are serialized. It appears that this behavior is already implemented in other languages

  • java: https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/java/util/src/test/java/com/google/protobuf/util/JsonFormatTest.java#L1486
  • ruby: ???https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java#L703
  • python: https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/python/google/protobuf/internal/json_format_test.py#L301

This PR also is changing the behavior of the upb JSON serializer, so that JSON name is always used if it is explicitly provided, even if a user asked for proto field names. We cannot change this behavior, as it would cause divergence from what other languages do.

For example, here is the C++ logic for preserve_proto_field_names. Note that it always uses the proto field name if the option is set; it doesn't look to see if a json_name was set:

https://github.com/protocolbuffers/protobuf/blob/671b61b523dbec8bf8e41a61d5a8f689c3e2bb9f/src/google/protobuf/json/internal/unparser.cc#L409-L410

Okay, I didn't know. I will change the code to be compatible with C++ and keep preserve_proto_field_names priority over json_names

s2x avatar Feb 01 '24 17:02 s2x

It appears that this behavior is already implemented in other languages

You are right, I misremembered. If this is implementing the behavior already supported by the upb_JsonEncode_EmitDefaults flag in upb, that should be fine.

haberman avatar Feb 01 '24 22:02 haberman

Hi @haberman, I hope you had a great weekend. I've been busy with a few tasks lately, but today I finally made the necessary changes.

I've incorporated your suggestions in the latest commit, ensuring that PHP code is now compatible with C++ code. Now the priority is to keep preserve_proto_field_names instead of json_names as you mentioned earlier.

If you could take a moment from your schedule, I would really appreciate it if you could review my latest commit.

s2x avatar Feb 05 '24 18:02 s2x

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

github-actions[bot] avatar May 08 '24 10:05 github-actions[bot]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

github-actions[bot] avatar May 22 '24 10:05 github-actions[bot]