From 345d49a51ee37a2b5cb953e5d1dcb872e7ffdfbf Mon Sep 17 00:00:00 2001 From: Feng Xiao Date: Wed, 1 Oct 2014 17:31:28 -0700 Subject: Fix a bug that causes DynamicMessage.setField() to not work for repeated enum fields. --- .../java/com/google/protobuf/DynamicMessage.java | 21 +++++++++++++++++++-- .../com/google/protobuf/DynamicMessageTest.java | 16 ++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) (limited to 'java') diff --git a/java/src/main/java/com/google/protobuf/DynamicMessage.java b/java/src/main/java/com/google/protobuf/DynamicMessage.java index 6bba8eb8..b8bf5746 100644 --- a/java/src/main/java/com/google/protobuf/DynamicMessage.java +++ b/java/src/main/java/com/google/protobuf/DynamicMessage.java @@ -38,6 +38,7 @@ import com.google.protobuf.Descriptors.OneofDescriptor; import java.io.InputStream; import java.io.IOException; import java.util.Collections; +import java.util.List; import java.util.Map; /** @@ -483,8 +484,13 @@ public final class DynamicMessage extends AbstractMessage { public Builder setField(FieldDescriptor field, Object value) { verifyContainingType(field); ensureIsMutable(); + // TODO(xiaofeng): This check should really be put in FieldSet.setField() + // where all other such checks are done. However, currently + // FieldSet.setField() permits Integer value for enum fields probably + // because of some internal features we support. Should figure it out + // and move this check to a more appropriate place. if (field.getType() == FieldDescriptor.Type.ENUM) { - verifyEnumType(field, value); + verifyEnumValue(field, value); } OneofDescriptor oneofDescriptor = field.getContainingOneof(); if (oneofDescriptor != null) { @@ -573,7 +579,7 @@ public final class DynamicMessage extends AbstractMessage { } /** Verifies that the value is EnumValueDescriptor and matchs Enum Type. */ - private void verifyEnumType(FieldDescriptor field, Object value) { + private void verifySingleEnumValue(FieldDescriptor field, Object value) { if (value == null) { throw new NullPointerException(); } @@ -587,6 +593,17 @@ public final class DynamicMessage extends AbstractMessage { } } + /** Verifies the value for an enum field. */ + private void verifyEnumValue(FieldDescriptor field, Object value) { + if (field.isRepeated()) { + for (Object item : (List) value) { + verifySingleEnumValue(field, item); + } + } else { + verifySingleEnumValue(field, value); + } + } + private void ensureIsMutable() { if (fields.isImmutable()) { fields = fields.clone(); diff --git a/java/src/test/java/com/google/protobuf/DynamicMessageTest.java b/java/src/test/java/com/google/protobuf/DynamicMessageTest.java index 2f17f0b0..55144e7c 100644 --- a/java/src/test/java/com/google/protobuf/DynamicMessageTest.java +++ b/java/src/test/java/com/google/protobuf/DynamicMessageTest.java @@ -30,6 +30,7 @@ package com.google.protobuf; +import com.google.protobuf.Descriptors.EnumDescriptor; import com.google.protobuf.Descriptors.FieldDescriptor; import com.google.protobuf.Descriptors.OneofDescriptor; @@ -307,4 +308,19 @@ public class DynamicMessageTest extends TestCase { message = builder.build(); assertSame(null, message.getOneofFieldDescriptor(oneof)); } + + // Regression test for a bug that makes setField() not work for repeated + // enum fields. + public void testSettersForRepeatedEnumField() throws Exception { + DynamicMessage.Builder builder = + DynamicMessage.newBuilder(TestAllTypes.getDescriptor()); + FieldDescriptor repeatedEnumField = + TestAllTypes.getDescriptor().findFieldByName( + "repeated_nested_enum"); + EnumDescriptor enumDescriptor = TestAllTypes.NestedEnum.getDescriptor(); + builder.setField(repeatedEnumField, enumDescriptor.getValues()); + DynamicMessage message = builder.build(); + assertEquals( + enumDescriptor.getValues(), message.getField(repeatedEnumField)); + } } -- cgit v1.2.3 From 725326f1eef7549349f6712322c8e073a9dd7fe9 Mon Sep 17 00:00:00 2001 From: Feng Xiao Date: Thu, 2 Oct 2014 17:46:39 -0700 Subject: Update verification methods' names. --- java/src/main/java/com/google/protobuf/DynamicMessage.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'java') diff --git a/java/src/main/java/com/google/protobuf/DynamicMessage.java b/java/src/main/java/com/google/protobuf/DynamicMessage.java index b8bf5746..c9ce667b 100644 --- a/java/src/main/java/com/google/protobuf/DynamicMessage.java +++ b/java/src/main/java/com/google/protobuf/DynamicMessage.java @@ -490,7 +490,7 @@ public final class DynamicMessage extends AbstractMessage { // because of some internal features we support. Should figure it out // and move this check to a more appropriate place. if (field.getType() == FieldDescriptor.Type.ENUM) { - verifyEnumValue(field, value); + ensureEnumValueDescriptor(field, value); } OneofDescriptor oneofDescriptor = field.getContainingOneof(); if (oneofDescriptor != null) { @@ -578,8 +578,9 @@ public final class DynamicMessage extends AbstractMessage { } } - /** Verifies that the value is EnumValueDescriptor and matchs Enum Type. */ - private void verifySingleEnumValue(FieldDescriptor field, Object value) { + /** Verifies that the value is EnumValueDescriptor and matches Enum Type. */ + private void ensureSingularEnumValueDescriptor( + FieldDescriptor field, Object value) { if (value == null) { throw new NullPointerException(); } @@ -594,13 +595,14 @@ public final class DynamicMessage extends AbstractMessage { } /** Verifies the value for an enum field. */ - private void verifyEnumValue(FieldDescriptor field, Object value) { + private void ensureEnumValueDescriptor( + FieldDescriptor field, Object value) { if (field.isRepeated()) { for (Object item : (List) value) { - verifySingleEnumValue(field, item); + ensureSingularEnumValueDescriptor(field, item); } } else { - verifySingleEnumValue(field, value); + ensureSingularEnumValueDescriptor(field, value); } } -- cgit v1.2.3