Fixed parsing for string->double maps. (#243)
Map parsing/serializing relies on map entries always
having a predictable order. The code that generates
layout was not respecting this in the case of string
keys and primitive values.
diff --git a/BUILD b/BUILD
index ef2fcd2..9d3ae34 100644
--- a/BUILD
+++ b/BUILD
@@ -354,11 +354,24 @@
],
)
+proto_library(
+ name = "test_proto",
+ testonly = 1,
+ srcs = ["tests/test.proto"],
+)
+
+upb_proto_library(
+ name = "test_upbproto",
+ testonly = 1,
+ deps = [":test_proto"],
+)
+
cc_test(
name = "test_generated_code",
srcs = ["tests/test_generated_code.c"],
deps = [
":test_messages_proto3_proto_upb",
+ ":test_upbproto",
":upb_test",
],
)
@@ -657,6 +670,7 @@
"@com_google_protobuf//:descriptor_proto",
":descriptor_proto_lua",
":test_messages_proto3_proto_lua",
+ ":test_proto_lua",
"tests/bindings/lua/test_upb.lua",
"third_party/lunit/console.lua",
"third_party/lunit/lunit.lua",
@@ -683,8 +697,13 @@
)
lua_proto_library(
+ name = "test_proto_lua",
+ testonly = 1,
+ deps = [":test_proto"],
+)
+
+lua_proto_library(
name = "descriptor_proto_lua",
- visibility = ["//visibility:public"],
deps = ["@com_google_protobuf//:descriptor_proto"],
)
diff --git a/tests/bindings/lua/main.c b/tests/bindings/lua/main.c
index 5b50173..f3714b5 100644
--- a/tests/bindings/lua/main.c
+++ b/tests/bindings/lua/main.c
@@ -27,8 +27,10 @@
"./third_party/lunit/?.lua;"
"external/com_google_protobuf/?.lua;"
"external/com_google_protobuf/src/?.lua;"
+ "bazel-bin/?.lua;"
"bazel-bin/external/com_google_protobuf/src/?.lua;"
"bazel-bin/external/com_google_protobuf/?.lua;"
+ "bazel-bin/external/com_google_protobuf/?.lua;"
"upb/bindings/lua/?.lua"
"'";
diff --git a/tests/bindings/lua/test.proto b/tests/bindings/lua/test.proto
new file mode 100644
index 0000000..322a967
--- /dev/null
+++ b/tests/bindings/lua/test.proto
@@ -0,0 +1,7 @@
+syntax = "proto2";
+
+package lua_test;
+
+message TestLua {
+ map<string, double> map_string_double = 1;
+}
diff --git a/tests/bindings/lua/test_upb.lua b/tests/bindings/lua/test_upb.lua
index 25ec0c1..fb76224 100644
--- a/tests/bindings/lua/test_upb.lua
+++ b/tests/bindings/lua/test_upb.lua
@@ -1,6 +1,7 @@
local upb = require "lupb"
local lunit = require "lunit"
+local upb_test = require "tests.test_pb"
local test_messages_proto3 = require "google.protobuf.test_messages_proto3_pb"
local descriptor = require "google.protobuf.descriptor_pb"
@@ -68,6 +69,32 @@
assert_equal(12, msg2.map_int32_int32[6])
end
+function test_string_double_map()
+ msg = upb_test.MapTest()
+ msg.map_string_double["one"] = 1.0
+ msg.map_string_double["two point five"] = 2.5
+ assert_equal(1, msg.map_string_double["one"])
+ assert_equal(2.5, msg.map_string_double["two point five"])
+
+ -- Test overwrite.
+ msg.map_string_double["one"] = 2
+ assert_equal(2, msg.map_string_double["one"])
+ assert_equal(2.5, msg.map_string_double["two point five"])
+ msg.map_string_double["one"] = 1.0
+
+ -- Test delete.
+ msg.map_string_double["one"] = nil
+ assert_nil(msg.map_string_double["one"])
+ assert_equal(2.5, msg.map_string_double["two point five"])
+ msg.map_string_double["one"] = 1
+
+ local serialized = upb.encode(msg)
+ assert_true(#serialized > 0)
+ local msg2 = upb.decode(upb_test.MapTest, serialized)
+ assert_equal(1, msg2.map_string_double["one"])
+ assert_equal(2.5, msg2.map_string_double["two point five"])
+end
+
function test_msg_string_map()
msg = test_messages_proto3.TestAllTypesProto3()
msg.map_string_string["foo"] = "bar"
diff --git a/tests/test.proto b/tests/test.proto
index e790cbf..c4b7e9c 100644
--- a/tests/test.proto
+++ b/tests/test.proto
@@ -1,68 +1,8 @@
-// A series of messages with various kinds of cycles in them.
-// +-+---+ +---+ +---+
-// V | | V | V |
-// A -> B-+-> C -> D---+--->E---+
-// ^ |`---|--------^
-// +----------+----+ F
-
syntax = "proto2";
-message A {
- optional B b = 1;
-}
+package upb_test;
-message B {
- optional B b = 1;
- optional C c = 2;
-}
-
-message C {
- optional A a = 1;
- optional B b = 2;
- optional D d = 3;
- optional E e = 4;
-}
-
-message D {
- optional A a = 1;
- optional D d = 2;
- optional E e = 3;
-}
-
-message E {
- optional E e = 1;
-}
-
-message F {
- optional E e = 1;
-}
-
-// A proto with a bunch of simple primitives.
-message SimplePrimitives {
- optional fixed64 u64 = 1;
- optional fixed32 u32 = 2;
- optional double dbl = 3;
- optional float flt = 5;
- optional sint64 i64 = 6;
- optional sint32 i32 = 7;
- optional bool b = 8;
- optional string str = 9;
-
- oneof foo {
- int32 oneof_int32 = 10;
- string oneof_string = 11;
- }
-
- oneof bar {
- int64 oneof_int64 = 13;
- bytes oneof_bytes = 14;
- }
-
- message Nested {
- oneof foo {
- int32 oneof_int32 = 10;
- string b = 11;
- }
- }
+message MapTest {
+ map<string, double> map_string_double = 1;
}
diff --git a/tests/test_generated_code.c b/tests/test_generated_code.c
index 3d1f518..ef8575c 100644
--- a/tests/test_generated_code.c
+++ b/tests/test_generated_code.c
@@ -5,6 +5,7 @@
#include "src/google/protobuf/test_messages_proto3.upb.h"
#include "tests/upb_test.h"
+#include "tests/test.upb.h"
const char test_str[] = "abcdefg";
const char test_str2[] = "12345678910";
@@ -114,6 +115,25 @@
ASSERT(!const_ent);
}
+static void test_string_double_map() {
+ upb_arena *arena = upb_arena_new();
+ upb_strview serialized;
+ upb_test_MapTest *msg = upb_test_MapTest_new(arena);
+ upb_test_MapTest *msg2;
+ double val;
+
+ upb_test_MapTest_map_string_double_set(msg, test_str_view, 1.5, arena);
+ serialized.data = upb_test_MapTest_serialize(msg, arena, &serialized.size);
+ ASSERT(serialized.data);
+
+ msg2 = upb_test_MapTest_parse(serialized.data, serialized.size, arena);
+ ASSERT(msg2);
+ ASSERT(upb_test_MapTest_map_string_double_get(msg2, test_str_view, &val));
+ ASSERT(val == 1.5);
+
+ upb_arena_free(arena);
+}
+
static void test_string_map() {
upb_arena *arena = upb_arena_new();
protobuf_test_messages_proto3_TestAllTypesProto3 *msg =
@@ -323,6 +343,7 @@
int run_tests(int argc, char *argv[]) {
test_scalars();
test_string_map();
+ test_string_double_map();
test_int32_map();
test_repeated();
return 0;
diff --git a/upb/def.c b/upb/def.c
index f9de1a9..1b44aec 100644
--- a/upb/def.c
+++ b/upb/def.c
@@ -921,6 +921,32 @@
l->fields = fields;
l->submsgs = submsgs;
+ if (upb_msgdef_mapentry(m)) {
+ /* TODO(haberman): refactor this method so this special case is more
+ * elegant. */
+ const upb_fielddef *key = upb_msgdef_itof(m, 1);
+ const upb_fielddef *val = upb_msgdef_itof(m, 2);
+ fields[0].number = 1;
+ fields[1].number = 2;
+ fields[0].label = UPB_LABEL_OPTIONAL;
+ fields[1].label = UPB_LABEL_OPTIONAL;
+ fields[0].presence = 0;
+ fields[1].presence = 0;
+ fields[0].descriptortype = upb_fielddef_descriptortype(key);
+ fields[1].descriptortype = upb_fielddef_descriptortype(val);
+ fields[0].offset = 0;
+ fields[1].offset = sizeof(upb_strview);
+ fields[1].submsg_index = 0;
+
+ if (upb_fielddef_type(val) == UPB_TYPE_MESSAGE) {
+ submsgs[0] = upb_fielddef_msgsubdef(val)->layout;
+ }
+
+ l->field_count = 2;
+ l->size = 2 * sizeof(upb_strview);align_up(l->size, 8);
+ return true;
+ }
+
/* Allocate data offsets in three stages:
*
* 1. hasbits.
diff --git a/upbc/message_layout.cc b/upbc/message_layout.cc
index bf3eb2b..cb7f7f9 100644
--- a/upbc/message_layout.cc
+++ b/upbc/message_layout.cc
@@ -32,12 +32,7 @@
MessageLayout::SizeAndAlign MessageLayout::SizeOf(
const protobuf::FieldDescriptor* field) {
- if (field->containing_type()->options().map_entry()) {
- // Map entries aren't actually stored, they are only used during parsing.
- // For parsing, it helps a lot if all map entry messages have the same
- // layout.
- return {{8, 16}, {4, 8}}; // upb_stringview
- } else if (field->is_repeated()) {
+ if (field->is_repeated()) {
return {{4, 8}, {4, 8}}; // Pointer to array object.
} else {
return SizeOfUnwrapped(field);
@@ -50,7 +45,7 @@
case protobuf::FieldDescriptor::CPPTYPE_MESSAGE:
return {{4, 8}, {4, 8}}; // Pointer to message.
case protobuf::FieldDescriptor::CPPTYPE_STRING:
- return {{8, 16}, {4, 8}}; // upb_stringview
+ return {{8, 16}, {4, 8}}; // upb_strview
case protobuf::FieldDescriptor::CPPTYPE_BOOL:
return {{1, 1}, {1, 1}};
case protobuf::FieldDescriptor::CPPTYPE_FLOAT:
@@ -111,8 +106,18 @@
void MessageLayout::ComputeLayout(const protobuf::Descriptor* descriptor) {
size_ = Size{0, 0};
maxalign_ = Size{0, 0};
- PlaceNonOneofFields(descriptor);
- PlaceOneofFields(descriptor);
+
+ if (descriptor->options().map_entry()) {
+ // Map entries aren't actually stored, they are only used during parsing.
+ // For parsing, it helps a lot if all map entry messages have the same
+ // layout.
+ SizeAndAlign size{{8, 16}, {4, 8}}; // upb_strview
+ field_offsets_[descriptor->FindFieldByNumber(1)] = Place(size);
+ field_offsets_[descriptor->FindFieldByNumber(2)] = Place(size);
+ } else {
+ PlaceNonOneofFields(descriptor);
+ PlaceOneofFields(descriptor);
+ }
// Align overall size up to max size.
size_.AlignUp(maxalign_);
