From 3d46d8dfe43150244a6c52ebfb0ea6ce29756aa2 Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Fri, 14 Dec 2018 16:41:44 -0800 Subject: [PATCH 1/6] Add conformance test for enum alias of the same name with different case --- conformance/conformance_test_impl.cc | 17 +++++++++++++++++ src/google/protobuf/test_messages_proto3.proto | 12 ++++++++++++ 2 files changed, 29 insertions(+) diff --git a/conformance/conformance_test_impl.cc b/conformance/conformance_test_impl.cc index a884deaaab..0597cda898 100644 --- a/conformance/conformance_test_impl.cc +++ b/conformance/conformance_test_impl.cc @@ -1373,6 +1373,23 @@ void ConformanceTestSuiteImpl::RunSuiteImpl() { "EnumField", REQUIRED, R"({"optionalNestedEnum": "FOO"})", "optional_nested_enum: FOO"); + // Enum fields with alias + RunValidJsonTest( + "EnumFieldWithAlias", REQUIRED, + R"({"optionalAliasedEnum": "ALIAS_BAZ"})", + "optional_aliased_enum: ALIAS_BAZ"); + RunValidJsonTest( + "EnumFieldWithAliasUseAlias", REQUIRED, + R"({"optionalAliasedEnum": "QUX"})", + "optional_aliased_enum: ALIAS_BAZ"); + RunValidJsonTest( + "EnumFieldWithAliasLowerCase", REQUIRED, + R"({"optionalAliasedEnum": "qux"})", + "optional_aliased_enum: ALIAS_BAZ"); + RunValidJsonTest( + "EnumFieldWithAliasDifferentCase", REQUIRED, + R"({"optionalAliasedEnum": "bAz"})", + "optional_aliased_enum: ALIAS_BAZ"); // Enum values must be represented as strings. ExpectParseFailureForJson( "EnumFieldNotQuoted", REQUIRED, diff --git a/src/google/protobuf/test_messages_proto3.proto b/src/google/protobuf/test_messages_proto3.proto index 4f295aac49..e47856d07f 100644 --- a/src/google/protobuf/test_messages_proto3.proto +++ b/src/google/protobuf/test_messages_proto3.proto @@ -73,6 +73,17 @@ message TestAllTypesProto3 { NEG = -1; // Intentionally negative. } + enum AliasedEnum { + option allow_alias = true; + + ALIAS_FOO = 0; + ALIAS_BAR = 1; + ALIAS_BAZ = 2; + QUX = 2; + qux = 2; + bAz = 2; + } + // Singular int32 optional_int32 = 1; int64 optional_int64 = 2; @@ -95,6 +106,7 @@ message TestAllTypesProto3 { NestedEnum optional_nested_enum = 21; ForeignEnum optional_foreign_enum = 22; + AliasedEnum optional_aliased_enum = 23; string optional_string_piece = 24 [ctype=STRING_PIECE]; string optional_cord = 25 [ctype=CORD]; From 00b9b2303cc5057691ce131350b9dc14fa4b304c Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Fri, 14 Dec 2018 16:48:06 -0800 Subject: [PATCH 2/6] Add conformance test for enum alias with same name but different case --- conformance/binary_json_conformance_suite.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/conformance/binary_json_conformance_suite.cc b/conformance/binary_json_conformance_suite.cc index bdc3691239..404488c8c7 100644 --- a/conformance/binary_json_conformance_suite.cc +++ b/conformance/binary_json_conformance_suite.cc @@ -1387,6 +1387,23 @@ void BinaryAndJsonConformanceSuite::RunSuiteImpl() { R"({"optionalNestedEnum": "FOO"})", "optional_nested_enum: FOO"); // Enum fields with alias + RunValidJsonTest( + "EnumFieldWithAlias", REQUIRED, + R"({"optionalAliasedEnum": "ALIAS_BAZ"})", + "optional_aliased_enum: ALIAS_BAZ"); + RunValidJsonTest( + "EnumFieldWithAliasUseAlias", REQUIRED, + R"({"optionalAliasedEnum": "QUX"})", + "optional_aliased_enum: ALIAS_BAZ"); + RunValidJsonTest( + "EnumFieldWithAliasLowerCase", REQUIRED, + R"({"optionalAliasedEnum": "qux"})", + "optional_aliased_enum: ALIAS_BAZ"); + RunValidJsonTest( + "EnumFieldWithAliasDifferentCase", REQUIRED, + R"({"optionalAliasedEnum": "bAz"})", + "optional_aliased_enum: ALIAS_BAZ"); + // Enum fields with alias RunValidJsonTest( "EnumFieldWithAlias", REQUIRED, R"({"optionalAliasedEnum": "ALIAS_BAZ"})", From 37dbfd6c463ade173b93202d65911afe084bf781 Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Fri, 14 Dec 2018 17:06:11 -0800 Subject: [PATCH 3/6] Do not require ruby enum to be uppercase --- conformance/binary_json_conformance_suite.cc | 17 ----------------- ruby/ext/google/protobuf_c/message.c | 12 ++++++------ 2 files changed, 6 insertions(+), 23 deletions(-) diff --git a/conformance/binary_json_conformance_suite.cc b/conformance/binary_json_conformance_suite.cc index 404488c8c7..bdc3691239 100644 --- a/conformance/binary_json_conformance_suite.cc +++ b/conformance/binary_json_conformance_suite.cc @@ -1387,23 +1387,6 @@ void BinaryAndJsonConformanceSuite::RunSuiteImpl() { R"({"optionalNestedEnum": "FOO"})", "optional_nested_enum: FOO"); // Enum fields with alias - RunValidJsonTest( - "EnumFieldWithAlias", REQUIRED, - R"({"optionalAliasedEnum": "ALIAS_BAZ"})", - "optional_aliased_enum: ALIAS_BAZ"); - RunValidJsonTest( - "EnumFieldWithAliasUseAlias", REQUIRED, - R"({"optionalAliasedEnum": "QUX"})", - "optional_aliased_enum: ALIAS_BAZ"); - RunValidJsonTest( - "EnumFieldWithAliasLowerCase", REQUIRED, - R"({"optionalAliasedEnum": "qux"})", - "optional_aliased_enum: ALIAS_BAZ"); - RunValidJsonTest( - "EnumFieldWithAliasDifferentCase", REQUIRED, - R"({"optionalAliasedEnum": "bAz"})", - "optional_aliased_enum: ALIAS_BAZ"); - // Enum fields with alias RunValidJsonTest( "EnumFieldWithAlias", REQUIRED, R"({"optionalAliasedEnum": "ALIAS_BAZ"})", diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index 81a782766f..edd14c2ffd 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -697,12 +697,12 @@ VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) { upb_enum_next(&it)) { const char* name = upb_enum_iter_name(&it); int32_t value = upb_enum_iter_number(&it); - if (name[0] < 'A' || name[0] > 'Z') { - rb_raise(cTypeError, - "Enum value '%s' does not start with an uppercase letter " - "as is required for Ruby constants.", - name); - } + // if (name[0] < 'A' || name[0] > 'Z') { + // rb_raise(cTypeError, + // "Enum value '%s' does not start with an uppercase letter " + // "as is required for Ruby constants.", + // name); + // } rb_define_const(mod, name, INT2NUM(value)); } From c11096c10d1dff47f0126fc7eed9565804a02669 Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Fri, 14 Dec 2018 17:06:36 -0800 Subject: [PATCH 4/6] Do not require Ruby enum to be upper case --- ruby/ext/google/protobuf_c/message.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index edd14c2ffd..2f14ef6ecf 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -697,12 +697,6 @@ VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) { upb_enum_next(&it)) { const char* name = upb_enum_iter_name(&it); int32_t value = upb_enum_iter_number(&it); - // if (name[0] < 'A' || name[0] > 'Z') { - // rb_raise(cTypeError, - // "Enum value '%s' does not start with an uppercase letter " - // "as is required for Ruby constants.", - // name); - // } rb_define_const(mod, name, INT2NUM(value)); } From 3c547fcdf377d8caf1f9170f8b76f16306231e8b Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Wed, 19 Dec 2018 11:25:53 -0800 Subject: [PATCH 5/6] Lower the severity of lower-case ruby enum to warning. Add conformance test for allow_alias with lower_case enums --- ruby/ext/google/protobuf_c/message.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index 2f14ef6ecf..b1d6a5e289 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/ruby/ext/google/protobuf_c/message.c @@ -697,6 +697,11 @@ VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) { upb_enum_next(&it)) { const char* name = upb_enum_iter_name(&it); int32_t value = upb_enum_iter_number(&it); + if (name[0] < 'A' || name[0] > 'Z') { + rb_warn("Enum value '%s' does not start with an uppercase letter " + "as is required for Ruby constants.", + name); + } rb_define_const(mod, name, INT2NUM(value)); } From e24d9a8aba909f4d74f7659cf9c98636ed18d05c Mon Sep 17 00:00:00 2001 From: Hao Nguyen Date: Wed, 19 Dec 2018 15:48:57 -0800 Subject: [PATCH 6/6] Add dependency to AliasedEnum in PHP --- conformance/conformance_php.php | 1 + 1 file changed, 1 insertion(+) diff --git a/conformance/conformance_php.php b/conformance/conformance_php.php index cc6d4b9f0a..80860c9504 100755 --- a/conformance/conformance_php.php +++ b/conformance/conformance_php.php @@ -8,6 +8,7 @@ require_once("Conformance/TestCategory.php"); require_once("Protobuf_test_messages/Proto3/ForeignMessage.php"); require_once("Protobuf_test_messages/Proto3/ForeignEnum.php"); require_once("Protobuf_test_messages/Proto3/TestAllTypesProto3.php"); +require_once("Protobuf_test_messages/Proto3/TestAllTypesProto3/AliasedEnum.php"); require_once("Protobuf_test_messages/Proto3/TestAllTypesProto3/NestedMessage.php"); require_once("Protobuf_test_messages/Proto3/TestAllTypesProto3/NestedEnum.php");