Auto capitalize enums name in Ruby

[tisonkun]

This closes #1965.

[tisonkun]

@anandolee could you help with triggering the CI workflows? If there're some failure, I can investigate during wait for reviews.

[tisonkun]

It seems all tasks passed. @haberman could you give this patch a review?

[tisonkun]

@JasonLunn thanks for your reviews. Comments addressed.

[JasonLunn]

Thanks, @tisonkun. Is the new test passing for you locally? It is failing in CI. The JRuby changes fail with NameError: bad constant name 860 with stack trace:

add_serialized_file at [com/google/protobuf/jruby/RubyDescriptorPool.java:125]
internal_add_file at /[workspace/ruby/lib/google/protobuf/descriptor_dsl.rb:65]
add_file at /[workspace/ruby/lib/google/protobuf/descriptor_dsl.rb:43]

This exception appears in all the tests targets, including gc_test and the conformance_test, not just the unit tests.

In the CRuby interpreters, the failure is limited to the unit tests. The new assertion (assert proto_module::TestEnum::V0 == 4 fails with NameError: uninitialized constant BasicTest::TestEnum::V0 and separately the new call to push (l.push :V0) fails with RangeError: Unknown symbol value for enum field ''..

[tisonkun]

@JasonLunn how can I run tests locally? I don't find some instructions. Also the test output is missing :(

Let me try out rake test.

[tisonkun]

OK. I found that I only changed for enummodule calls, but not for the compiler. I will spend some time to find the related code and made the change. But it would help a lot if you can tell when which logic writes:

    add_enum "a.b.proto2.TestEnum" do
      value :Default, 0
      value :A, 1
      value :B, 2
      value :C, 3
      value :v0, 4
    end

[tisonkun]

For JRuby, it may need more effort. And I may consider we finish the CRuby part first and following up for the JRuby part - I work on this part-time and my use case is CRuby only.

[tisonkun]

Addressed. The ruby unit test can be still failed because updates to the compiler seems not be used by the unit test in the same patch. I'm unsure if we should make the compiler changes in first?

Hmm...I hope the CI can build the protoc and use the new one just built :)

[tisonkun]

@JasonLunn it seems rake test doesn't run JRuby tests. Can you show me how to do that? rake test should now passed but I don't make change on JRuby related logics.

[tisonkun]

For JRuby part, I switch the ruby version to jruby-9.3.7.0 with rbenv and run rake test. It seems all tests passed locally now.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.093 s
[INFO] Finished at: 2022-09-10T08:45:13+08:00
[INFO] ------------------------------------------------------------------------
/Users/chenzili/.rbenv/versions/jruby-9.3.7.0/lib/ruby/gems/shared/gems/power_assert-1.1.3/lib/power_assert.rb:13: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/Users/chenzili/.rbenv/versions/jruby-9.3.7.0/lib/ruby/gems/shared/gems/power_assert-1.1.3/lib/power_assert.rb:13: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
Loaded suite /Users/chenzili/.rbenv/versions/jruby-9.3.7.0/lib/ruby/gems/shared/gems/rake-12.3.3/lib/rake/rake_test_loader
Started
..........................................................................................................................................................
......................................................................No kokoro ruby version found, skipping check
...................
Finished in 2.7511509999999997 seconds.
----------------------------------------------------------------------------------------------------------------------------------------------------------
243 tests, 318350 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
----------------------------------------------------------------------------------------------------------------------------------------------------------
88.33 tests/s, 115715.20 assertions/s

[deannagarcia]

There are still some errors in the conformance test:

ERROR, test=Required.Proto3.JsonInput.EnumFieldWithAliasLowerCase.ProtobufOutput: Failed to parse input or produce output. request=json_payload: "{\"optionalAliasedEnum\": \"moo\"}" requested_output_format: PROTOBUF message_type: "protobuf_test_messages.proto3.TestAllTypesProto3" test_category: JSON_TEST, response=parse_error: "Error occurred during parsing: Error parsing JSON @1:29: Unknown enumerator: \'moo\'"

The test comes from this file which I hope you will be able to run locally.

[tisonkun]

@deannagarcia reproduced and pushed a fix. It's because now Ruby will auto-capitalize enum names so that tests cannot find moo which becomes Moo. Previously, ruby compiler simply compiles moo as moo but at runtime, it will cause NameError.

[tisonkun]

@deannagarcia @JasonLunn could you help with triggering the CI again? I run bazel test //ruby:conformance_test locally and all tests passed.

[JasonLunn]

I believe that the changes to conformance/binary_json_conformance_suite.cc are breaking non-ruby languages.

[tisonkun]

Let me think of it...

[tisonkun]

OK. I think I get the cause. Let me try to find a way adjust ruby tests only.

[tisonkun]

It's because we don't perform the same capitalize logic for decode json.

[tisonkun]

Fixed locally. Please rerun the tests. I think I should fix it for JRuby also, but I still don't find the related code.

[tisonkun]

@JasonLunn may I ask how to run JRuby conformance test?

bazel test //jruby:conformance_test gives:

ERROR: no such package 'jruby': BUILD file not found in any of the following directories. Add a BUILD file to a directory to mark it as a package.
 - /Users/chenzili/Brittani/protobuf/jruby
INFO: Elapsed time: 0.092s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)

[tisonkun]

Anyway, since JRuby delegates the building stage to

parser.merge(data.asJavaString(), ret.builder);

The enum name in the data can hardly be handled. I tend not to implement it for JRuby.

Here we have two ways:

1. Ignore the cases for JRuby. Because enum with lower letter doesn't work for Ruby, the real field is always with an upper name.
2. Update all occurrence of bAz and moo for aliasing tests to BAz and Moo.

[tisonkun]

Pushed as f369bcb4e1d7d3d209f804c4a7e991f22ed48fd3 and 4bb02e09d373a21dde2e7522d6c29d16a4810603 for JRuby. No regressions locally.

[tisonkun]

Well. The last two commits don't help on decode_json (auto capitalize) for JRuby as described above.

[tisonkun]

@JasonLunn @deannagarcia I think ignore these cases for JRuby is OK.

1. If you define an Enum starting with lower case, it's already invalid for Ruby. We auto capitalize for using the enum without changing the proto file while keep pb binary compatible.
2. Now, if we encode to json, v0 will be V0. And it works well for Ruby self usage but fails if interop between different languages.
3. A thoroughgoing solution can be: (1) add a mark of the "original name" for codec in text/json format. (2) Use something different from Ruby's constants to represent Enum so that lower letter is allowed.

Anyway, currently, these cases pass wrongly. Even decode_json accepts enum name in lower case, it means nothing for a real Ruby use case.

[tisonkun]

@JasonLunn @deannagarcia @mkruskal-google any further comments here?

[JasonLunn]

@tisonkun - can you help me understand two things:

1. Why is it that only JRuby needs expectation changes to the conformance tests, but CRuby doesn't?
2. Can you clarify what specific tests you mean when you wrote "these cases pass wrongly"?

[tisonkun]

@JasonLunn thanks for your follow-ups:

Why is it that only JRuby needs expectation changes to the conformance tests, but CRuby doesn't?

Please read https://github.com/protocolbuffers/protobuf/pull/10454#issuecomment-1246898576. The CRuby path is full under control and can be changed, while the JRuby path depends on Java Protobuf implementation.

Can you clarify what specific tests you mean when you wrote "these cases pass wrongly"?

Even decode_json accepts enum name in lower case, it means nothing for a real Ruby use case. I'm debugging deeper in a more expressive output. Or I'd say the tests pass but it doesn't reflect a real-world use case.

[tisonkun]

Emmm..I may make a mistake here. That we can already use resolve to work with lower letter started enums.

Let me comment on the original issue.

https://github.com/protocolbuffers/protobuf/issues/1965#issuecomment-1259723485 Waiting for reviewers' suggestion.

[tisonkun]

Wait a minute. I think the original purpose is to define the constant following Ruby's rule. Let me try to narrow the change scope.

[tisonkun]

Updated. I think this patch is tidy now. All tests passed locally:

rbenv global 3.1.2
bazel test //ruby:conformance_test
rbenv global jruby-9.3.7.0
bazel test //ruby:conformance_test_jruby --define=ruby_platform=java --action_env=PATH --action_env=GEM_PATH --action_env=GEM_HOME --verbose_failures
cd ./ruby/
bundle install
bundle exec rake test
rbenv global 3.1.2
bundle exec rake test

cc @JasonLunn @deannagarcia please take a review :)

[tisonkun]

@JasonLunn @deannagarcia @mkruskal-google All tests passed now and the changeset is minimal. Please take a look :)