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

Ruby: Add support for proto3 json_name in compiler and field definitions #8356

Merged
merged 3 commits into from Apr 20, 2021

Conversation

lfittl
Copy link
Contributor

@lfittl lfittl commented Mar 1, 2021

This adds support for the json_name field definition, per the Proto3 spec.

Note the JSON output mechanism is already reading the json_name from ubp's struct (see here), but this was not being added to the generated Ruby DSL, or read in from the DSL into the ubp field definitions.

@google-cla
Copy link

google-cla bot commented Mar 1, 2021

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).

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

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


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Mar 1, 2021
@lfittl
Copy link
Contributor Author

lfittl commented Mar 1, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Mar 1, 2021
@lfittl lfittl force-pushed the support-json-name-for-ruby branch from 3f6da46 to 2c354c6 Compare March 1, 2021 08:25
@lfittl
Copy link
Contributor Author

lfittl commented Mar 1, 2021

@haberman I assume you would be the correct person to review this further?

(Thanks for your work on the Ruby protobuf support!)

@lfittl
Copy link
Contributor Author

lfittl commented Mar 30, 2021

@haberman Ping on this, tagging you since you seem to be the primary maintainer for the Ruby bindings - not sure if I missed anything here. Appreciate the help - thank you :)

@haberman
Copy link
Member

haberman commented Apr 3, 2021

Hi @lfittl , I'm sorry for my slow reply on this.

I'm somewhat reluctant to go farther down this path of extending the DSL, because it is an un-winnable game of whack-a-mole. The DSL fundamentally can't represent all options, particular custom options that are used in their own definition:

syntax = "proto3";

import "google/protobuf/descriptor.proto";

extend google.protobuf.FieldOptions {
  optional int32 foo_option = 1000 [(foo_option) = 5];
}

That said, the fact that json_name is a member of FieldDescriptorProto (and not FieldOptions) does make the PR more compatible with a future world where we do handle options correctly. So maybe we can move forward with this now.

FieldDescriptor* self = ruby_to_FieldDescriptor(_self);
const upb_fielddef *f = self->fielddef;
const char *json_name = upb_fielddef_jsonname(f);
if (json_name != NULL)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this check should be necessary, upb_fielddef_jsonname() should never return NULL.

rb_hash_lookup(options, ID2SYM(rb_intern("json_name")));

/* Call #to_s since json_name is always a string in the descriptor. */
json_name = rb_funcall(json_name, rb_intern("to_s"), 0);
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow, if it is a string already why do we need to call to_s on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you are correct - this was copied from the default value handling above, but json_name is indeed always a string, so this conversion is unnecessary.

@lfittl
Copy link
Contributor Author

lfittl commented Apr 4, 2021

Hi @lfittl , I'm sorry for my slow reply on this.

No worries, I know the same from my own open source projects, thanks for taking a look!

I'm somewhat reluctant to go farther down this path of extending the DSL, because it is an un-winnable game of whack-a-mole. The DSL fundamentally can't represent all options, particular custom options that are used in their own definition:

syntax = "proto3";

import "google/protobuf/descriptor.proto";

extend google.protobuf.FieldOptions {
  optional int32 foo_option = 1000 [(foo_option) = 5];
}

That said, the fact that json_name is a member of FieldDescriptorProto (and not FieldOptions) does make the PR more compatible with a future world where we do handle options correctly. So maybe we can move forward with this now.

Yeah, I see how it's important to handle this generically at some point - but I think it still makes sense to do this now, since it allows use of json_name which seems to me one of the core features of Proto3 (as opposed to other custom field tags).


I've addressed the review comments you shared, and also added a test that covers this, to ensure it doesn't break by accident in the future.

@haberman haberman merged commit fecf7e9 into protocolbuffers:master Apr 20, 2021
acozzette added a commit that referenced this pull request Apr 20, 2021
* Ruby: Add support for proto3 json_name in compiler and field definitions

* Address review feedback

* Add test for json_name functionality

Co-authored-by: Lukas Fittl <lukas@fittl.com>
lfittl added a commit to pganalyze/pg_query that referenced this pull request Jul 4, 2021
This was previously relying on an auto-generated mapping layer, but since
the upstream patch was merged into protobuf 3.17.0 we can rely on the
upstream support instead.

See protocolbuffers/protobuf#8356
lfittl added a commit to pganalyze/pg_query that referenced this pull request Jul 4, 2021
This was previously relying on an auto-generated mapping layer, but since
the upstream patch was merged into protobuf 3.17.0 we can rely on the
upstream support instead.

See protocolbuffers/protobuf#8356
bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023
…-for-ruby

Ruby: Add support for proto3 json_name in compiler and field definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants