From 1d39f7881874de0396fcf0e88b0616df741a8033 Mon Sep 17 00:00:00 2001 From: Rob Widmer Date: Fri, 8 Jan 2021 11:54:47 -0500 Subject: [PATCH] Fix jruby support to handle messages nested more than 1 level deep --- .../jruby/RubyFileBuilderContext.java | 57 +++++++++++-------- ruby/tests/multi_level_nesting_test.rb | 20 +++++++ ruby/tests/multi_level_nesting_test_pb.rb | 25 ++++++++ 3 files changed, 79 insertions(+), 23 deletions(-) create mode 100644 ruby/tests/multi_level_nesting_test.rb create mode 100644 ruby/tests/multi_level_nesting_test_pb.rb diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyFileBuilderContext.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyFileBuilderContext.java index 7d90be1185..00ce3f2158 100644 --- a/ruby/src/main/java/com/google/protobuf/jruby/RubyFileBuilderContext.java +++ b/ruby/src/main/java/com/google/protobuf/jruby/RubyFileBuilderContext.java @@ -52,6 +52,7 @@ import org.jruby.runtime.builtin.IRubyObject; import java.util.HashMap; import java.util.List; +import java.util.TreeMap; @JRubyClass(name = "FileBuilderContext") public class RubyFileBuilderContext extends RubyObject { @@ -154,7 +155,7 @@ public class RubyFileBuilderContext extends RubyObject { } // Make an index of the message builders so we can easily nest them - HashMap messageBuilderMap = new HashMap(); + TreeMap messageBuilderMap = new TreeMap(); for (DescriptorProto.Builder messageBuilder : messageBuilderList) { messageBuilderMap.put(messageBuilder.getName(), messageBuilder); } @@ -176,7 +177,6 @@ public class RubyFileBuilderContext extends RubyObject { EnumDescriptorProto.Builder[] enumBuilders = new EnumDescriptorProto.Builder[enumBuilderList.size()]; enumBuilderList.toArray(enumBuilders); - for (EnumDescriptorProto.Builder enumBuilder : enumBuilders) { String name = enumBuilder.getName(); int lastDot = name.lastIndexOf('.'); @@ -216,28 +216,17 @@ public class RubyFileBuilderContext extends RubyObject { } } - for (DescriptorProto.Builder messageBuilder : messageBuilders) { - String name = messageBuilder.getName(); - int lastDot = name.lastIndexOf('.'); - - if (lastDot > packageNameLength) { - String parentName = name.substring(0, lastDot); - String shortName = name.substring(lastDot + 1); - - messageBuilder.setName(shortName); - messageBuilderMap.get(parentName).addNestedType(messageBuilder); - - builder.removeMessageType(currentMessageIndex); - - } else { - if (packageNameLength > 0) { - // Remove the package name - String shortName = name.substring(packageNameLength + 1); - messageBuilder.setName(shortName); - } + // Wipe out top level message builders so we can insert only the ones that should be there + builder.clearMessageType(); - currentMessageIndex++; - } + /* + * This block is done in this order because calling + * `addNestedType` and `addMessageType` makes a copy of the builder + * so the objects that our maps point to are no longer the objects + * that are being used to build the descriptions. + */ + for (HashMap.Entry entry : messageBuilderMap.descendingMap().entrySet()) { + DescriptorProto.Builder messageBuilder = entry.getValue(); // Rewrite any enum defaults needed for(FieldDescriptorProto.Builder field : messageBuilder.getFieldBuilderList()) { @@ -258,6 +247,28 @@ public class RubyFileBuilderContext extends RubyObject { } } } + + // Turn Foo.Bar.Baz into a correctly nested structure with the correct name + String name = messageBuilder.getName(); + int lastDot = name.lastIndexOf('.'); + + if (lastDot > packageNameLength) { + String parentName = name.substring(0, lastDot); + String shortName = name.substring(lastDot + 1); + messageBuilder.setName(shortName); + messageBuilderMap.get(parentName).addNestedType(messageBuilder); + + } else { + if (packageNameLength > 0) { + // Remove the package name + messageBuilder.setName(name.substring(packageNameLength + 1)); + } + + // Add back in top level message definitions + builder.addMessageType(messageBuilder); + + currentMessageIndex++; + } } descriptorPool.registerFileDescriptor(context, builder); diff --git a/ruby/tests/multi_level_nesting_test.rb b/ruby/tests/multi_level_nesting_test.rb new file mode 100644 index 0000000000..7e5d0ec6b7 --- /dev/null +++ b/ruby/tests/multi_level_nesting_test.rb @@ -0,0 +1,20 @@ +#!/usr/bin/ruby + +# multi_level_nesting_test_pb.rb is in the same directory as this test. +$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__))) + +require 'test/unit' +require 'multi_level_nesting_test_pb' + +# +# Provide tests for having messages nested 3 levels deep +# +class MultiLevelNestingTest < Test::Unit::TestCase + + def test_levels_exist + assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function").msgclass + assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter").msgclass + assert ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Function.Parameter.Value").msgclass + end + +end diff --git a/ruby/tests/multi_level_nesting_test_pb.rb b/ruby/tests/multi_level_nesting_test_pb.rb new file mode 100644 index 0000000000..b51b531499 --- /dev/null +++ b/ruby/tests/multi_level_nesting_test_pb.rb @@ -0,0 +1,25 @@ +# +# Provide tests for having messages nested 3 levels deep +# + +require 'google/protobuf' + +Google::Protobuf::DescriptorPool.generated_pool.build do + add_file("function_call.proto", :syntax => :proto3) do + add_message "Function" do + optional :name, :string, 1 + repeated :parameters, :message, 2, "Function.Parameter" + optional :return_type, :string, 3 + end + add_message "Function.Parameter" do + optional :name, :string, 1 + optional :value, :message, 2, "Function.Parameter.Value" + end + add_message "Function.Parameter.Value" do + oneof :type do + optional :string, :string, 1 + optional :integer, :int64, 2 + end + end + end +end