From 05b00c50ad07a30974681005ef553eb546a12ce1 Mon Sep 17 00:00:00 2001 From: Wouter van Oortmerssen Date: Wed, 20 Jul 2016 17:24:50 -0700 Subject: [PATCH] Added way to test two schemas for safe evolution. Change-Id: I1dfc867e6df5932ab61dad431eb3cb02f15d04df Tested: on Linux. Bug: 30202327 --- docs/source/Compiler.md | 4 ++ include/flatbuffers/idl.h | 12 ++++++ src/flatc.cpp | 91 +++++++++++++++++++++++++-------------- src/idl_parser.cpp | 53 +++++++++++++++++++++++ tests/test.cpp | 19 ++++++++ 5 files changed, 146 insertions(+), 33 deletions(-) diff --git a/docs/source/Compiler.md b/docs/source/Compiler.md index 1ff2c8d4e..b71b8c46d 100755 --- a/docs/source/Compiler.md +++ b/docs/source/Compiler.md @@ -108,5 +108,9 @@ Additional options: to the reflection/reflection.fbs schema. Loading this binary file is the basis for reflection functionality. +- `--conform FILE` : Specify a schema the following schemas should be + an evolution of. Gives errors if not. Useful to check if schema + modifications don't break schema evolution rules. + NOTE: short-form options for generators are deprecated, use the long form whenever possible. diff --git a/include/flatbuffers/idl.h b/include/flatbuffers/idl.h index a3027a0a9..1f4501629 100644 --- a/include/flatbuffers/idl.h +++ b/include/flatbuffers/idl.h @@ -311,6 +311,14 @@ struct EnumDef : public Definition { Type underlying_type; }; +inline bool EqualByName(const Type &a, const Type &b) { + return a.base_type == b.base_type && a.element == b.element && + (a.struct_def == b.struct_def || + a.struct_def->name == b.struct_def->name) && + (a.enum_def == b.enum_def || + a.enum_def->name == b.enum_def->name); +} + struct RPCCall { std::string name; SymbolTable attributes; @@ -473,6 +481,10 @@ class Parser : public ParserState { // See reflection/reflection.fbs void Serialize(); + // Checks that the schema represented by this parser is a safe evolution + // of the schema provided. Returns non-empty error on any problems. + std::string ConformTo(const Parser &base); + FLATBUFFERS_CHECKED_ERROR CheckBitsFit(int64_t val, size_t bits); private: diff --git a/src/flatc.cpp b/src/flatc.cpp index 631eaf1af..a568bbb24 100644 --- a/src/flatc.cpp +++ b/src/flatc.cpp @@ -84,14 +84,14 @@ const Generator generators[] = { flatbuffers::CPPMakeRule }, }; -const char *program_name = nullptr; -flatbuffers::Parser *parser = nullptr; +const char *g_program_name = nullptr; +flatbuffers::Parser *g_parser = nullptr; static void Error(const std::string &err, bool usage, bool show_exe_name) { - if (show_exe_name) printf("%s: ", program_name); + if (show_exe_name) printf("%s: ", g_program_name); printf("%s\n", err.c_str()); if (usage) { - printf("usage: %s [OPTION]... FILE... [-- FILE...]\n", program_name); + printf("usage: %s [OPTION]... FILE... [-- FILE...]\n", g_program_name); for (size_t i = 0; i < sizeof(generators) / sizeof(generators[0]); ++i) printf(" %-12s %s %s.\n", generators[i].generator_opt_long, @@ -128,19 +128,34 @@ static void Error(const std::string &err, bool usage, bool show_exe_name) { " This may crash flatc given a mismatched schema.\n" " --proto Input is a .proto, translate to .fbs.\n" " --schema Serialize schemas instead of JSON (use with -b)\n" + " --conform FILE Specify a schema the following schemas should be\n" + " an evolution of. Gives errors if not.\n" "FILEs may be schemas, or JSON files (conforming to preceding schema)\n" "FILEs after the -- must be binary flatbuffer format files.\n" "Output files are named using the base file name of the input,\n" "and written to the current directory or the path given by -o.\n" "example: %s -c -b schema1.fbs schema2.fbs data.json\n", - program_name); + g_program_name); } - if (parser) delete parser; + if (g_parser) delete g_parser; exit(1); } +static void ParseFile(flatbuffers::Parser &parser, const std::string &filename, + const std::string &contents, + std::vector &include_directories) { + auto local_include_directory = flatbuffers::StripFileName(filename); + include_directories.push_back(local_include_directory.c_str()); + include_directories.push_back(nullptr); + if (!parser.Parse(contents.c_str(), &include_directories[0], + filename.c_str())) + Error(parser.error_, false, false); + include_directories.pop_back(); + include_directories.pop_back(); +} + int main(int argc, const char *argv[]) { - program_name = argv[0]; + g_program_name = argv[0]; flatbuffers::IDLOptions opts; std::string output_path; const size_t num_generators = sizeof(generators) / sizeof(generators[0]); @@ -152,6 +167,7 @@ int main(int argc, const char *argv[]) { std::vector filenames; std::vector include_directories; size_t binary_files_from = std::numeric_limits::max(); + std::string conform_to_schema; for (int argi = 1; argi < argc; argi++) { std::string arg = argv[argi]; if (arg[0] == '-') { @@ -163,6 +179,9 @@ int main(int argc, const char *argv[]) { } else if(arg == "-I") { if (++argi >= argc) Error("missing path following" + arg, true); include_directories.push_back(argv[argi]); + } else if(arg == "--conform") { + if (++argi >= argc) Error("missing path following" + arg, true); + conform_to_schema = argv[argi]; } else if(arg == "--strict-json") { opts.strict_json = true; } else if(arg == "--no-js-exports") { @@ -230,12 +249,20 @@ int main(int argc, const char *argv[]) { if (opts.proto_mode) { if (any_generator) Error("cannot generate code directly from .proto files", true); - } else if (!any_generator) { + } else if (!any_generator && conform_to_schema.empty()) { Error("no options: specify at least one generator.", true); } + flatbuffers::Parser conform_parser; + if (!conform_to_schema.empty()) { + std::string contents; + if (!flatbuffers::LoadFile(conform_to_schema.c_str(), true, &contents)) + Error("unable to load schema: " + conform_to_schema); + ParseFile(conform_parser, conform_to_schema, contents, include_directories); + } + // Now process the files: - parser = new flatbuffers::Parser(opts); + g_parser = new flatbuffers::Parser(opts); for (auto file_it = filenames.begin(); file_it != filenames.end(); ++file_it) { @@ -246,8 +273,8 @@ int main(int argc, const char *argv[]) { bool is_binary = static_cast(file_it - filenames.begin()) >= binary_files_from; if (is_binary) { - parser->builder_.Clear(); - parser->builder_.PushFlatBuffer( + g_parser->builder_.Clear(); + g_parser->builder_.PushFlatBuffer( reinterpret_cast(contents.c_str()), contents.length()); if (!raw_binary) { @@ -256,17 +283,17 @@ int main(int argc, const char *argv[]) { // does not contain a file identifier. // We'd expect that typically any binary used as a file would have // such an identifier, so by default we require them to match. - if (!parser->file_identifier_.length()) { + if (!g_parser->file_identifier_.length()) { Error("current schema has no file_identifier: cannot test if \"" + *file_it + "\" matches the schema, use --raw-binary to read this file" " anyway."); } else if (!flatbuffers::BufferHasIdentifier(contents.c_str(), - parser->file_identifier_.c_str())) { + g_parser->file_identifier_.c_str())) { Error("binary \"" + *file_it + "\" does not have expected file_identifier \"" + - parser->file_identifier_ + + g_parser->file_identifier_ + "\", use --raw-binary to read this file anyway."); } } @@ -275,36 +302,34 @@ int main(int argc, const char *argv[]) { if (contents.length() != strlen(contents.c_str())) { Error("input file appears to be binary: " + *file_it, true); } - if (flatbuffers::GetExtension(*file_it) == "fbs") { + auto is_schema = flatbuffers::GetExtension(*file_it) == "fbs"; + if (is_schema) { // If we're processing multiple schemas, make sure to start each // one from scratch. If it depends on previous schemas it must do // so explicitly using an include. - delete parser; - parser = new flatbuffers::Parser(opts); + delete g_parser; + g_parser = new flatbuffers::Parser(opts); + } + ParseFile(*g_parser, *file_it, contents, include_directories); + if (is_schema && !conform_to_schema.empty()) { + auto err = g_parser->ConformTo(conform_parser); + if (!err.empty()) Error("schemas don\'t conform: " + err); } - auto local_include_directory = flatbuffers::StripFileName(*file_it); - include_directories.push_back(local_include_directory.c_str()); - include_directories.push_back(nullptr); - if (!parser->Parse(contents.c_str(), &include_directories[0], - file_it->c_str())) - Error(parser->error_, false, false); if (schema_binary) { - parser->Serialize(); - parser->file_extension_ = reflection::SchemaExtension(); + g_parser->Serialize(); + g_parser->file_extension_ = reflection::SchemaExtension(); } - include_directories.pop_back(); - include_directories.pop_back(); } std::string filebase = flatbuffers::StripPath( flatbuffers::StripExtension(*file_it)); for (size_t i = 0; i < num_generators; ++i) { - parser->opts.lang = generators[i].lang; + g_parser->opts.lang = generators[i].lang; if (generator_enabled[i]) { if (!print_make_rules) { flatbuffers::EnsureDirExists(output_path); - if (!generators[i].generate(*parser, output_path, filebase)) { + if (!generators[i].generate(*g_parser, output_path, filebase)) { Error(std::string("Unable to generate ") + generators[i].lang_name + " for " + @@ -312,7 +337,7 @@ int main(int argc, const char *argv[]) { } } else { std::string make_rule = generators[i].make_rule( - *parser, output_path, *file_it); + *g_parser, output_path, *file_it); if (!make_rule.empty()) printf("%s\n", flatbuffers::WordWrap( make_rule, 80, " ", " \\").c_str()); @@ -320,13 +345,13 @@ int main(int argc, const char *argv[]) { } } - if (opts.proto_mode) GenerateFBS(*parser, output_path, filebase); + if (opts.proto_mode) GenerateFBS(*g_parser, output_path, filebase); // We do not want to generate code for the definitions in this file // in any files coming up next. - parser->MarkGenerated(); + g_parser->MarkGenerated(); } - delete parser; + delete g_parser; return 0; } diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp index a5f325de4..fc2136757 100644 --- a/src/idl_parser.cpp +++ b/src/idl_parser.cpp @@ -2093,4 +2093,57 @@ flatbuffers::OffsetGetFullyQualifiedName(struct_def.name); + auto struct_def_base = base.structs_.Lookup(qualified_name); + if (!struct_def_base) continue; + for (auto fit = struct_def.fields.vec.begin(); + fit != struct_def.fields.vec.end(); ++fit) { + auto &field = **fit; + auto field_base = struct_def_base->fields.Lookup(field.name); + if (field_base) { + if (field.value.offset != field_base->value.offset) + return "offsets differ for field: " + field.name; + if (field.value.constant != field_base->value.constant) + return "defaults differ for field: " + field.name; + if (!EqualByName(field.value.type, field_base->value.type)) + return "types differ for field: " + field.name; + } else { + // Doesn't have to exist, deleting fields is fine. + // But we should check if there is a field that has the same offset + // but is incompatible (in the case of field renaming). + for (auto fbit = struct_def_base->fields.vec.begin(); + fbit != struct_def_base->fields.vec.end(); ++fbit) { + field_base = *fbit; + if (field.value.offset == field_base->value.offset) { + if (!EqualByName(field.value.type, field_base->value.type)) + return "field renamed to different type: " + field.name; + break; + } + } + } + } + } + for (auto eit = enums_.vec.begin(); eit != enums_.vec.end(); ++eit) { + auto &enum_def = **eit; + auto qualified_name = + enum_def.defined_namespace->GetFullyQualifiedName(enum_def.name); + auto enum_def_base = base.enums_.Lookup(qualified_name); + if (!enum_def_base) continue; + for (auto evit = enum_def.vals.vec.begin(); + evit != enum_def.vals.vec.end(); ++evit) { + auto &enum_val = **evit; + auto enum_val_base = enum_def_base->vals.Lookup(enum_val.name); + if (enum_val_base) { + if (enum_val.value != enum_val_base->value) + return "values differ for enum: " + enum_val.name; + } + } + } + return ""; +} + } // namespace flatbuffers diff --git a/tests/test.cpp b/tests/test.cpp index 37ed4c114..6ec4e678c 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -1060,6 +1060,24 @@ void ParseUnionTest() { "{ X:{ A:1 }, X_type: T }"), true); } +void ConformTest() { + flatbuffers::Parser parser; + TEST_EQ(parser.Parse("table T { A:int; } enum E:byte { A }"), true); + + auto test_conform = [&](const char *test, const char *expected_err) { + flatbuffers::Parser parser2; + TEST_EQ(parser2.Parse(test), true); + auto err = parser2.ConformTo(parser); + TEST_NOTNULL(strstr(err.c_str(), expected_err)); + }; + + test_conform("table T { A:byte; }", "types differ for field"); + test_conform("table T { B:int; A:int; }", "offsets differ for field"); + test_conform("table T { A:int = 1; }", "defaults differ for field"); + test_conform("table T { B:float; }", "field renamed to different type"); + test_conform("enum E:byte { B, A }", "values differ for enum"); +} + int main(int /*argc*/, const char * /*argv*/[]) { // Run our various test suites: @@ -1091,6 +1109,7 @@ int main(int /*argc*/, const char * /*argv*/[]) { UnicodeInvalidSurrogatesTest(); UnknownFieldsTest(); ParseUnionTest(); + ConformTest(); if (!testing_fails) { TEST_OUTPUT_LINE("ALL TESTS PASSED");