Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-1849 Fix C++ schema to JSON converter #97

Closed
wants to merge 9 commits into from
11 changes: 5 additions & 6 deletions lang/c++/impl/NodeImpl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -189,23 +189,22 @@ NodeRecord::printJson(std::ostream &os, int depth) const
os << "{\n";
os << indent(++depth) << "\"type\": \"record\",\n";
printName(os, nameAttribute_.get(), depth);
os << indent(depth) << "\"fields\": [\n";
os << indent(depth) << "\"fields\": [";

int fields = leafAttributes_.size();
++depth;
for(int i = 0; i < fields; ++i) {
if(i > 0) {
os << indent(depth) << "},\n";
os << ',';
}
os << indent(depth) << "{\n";
os << '\n' << indent(depth) << "{\n";
os << indent(++depth) << "\"name\": \"" << leafNameAttributes_.get(i) << "\",\n";
os << indent(depth) << "\"type\": ";
leafAttributes_.get(i)->printJson(os, depth);
os << '\n';
--depth;
os << indent(--depth) << '}';
}
os << indent(depth) << "}\n";
os << indent(--depth) << "]\n";
os << '\n' << indent(--depth) << "]\n";
os << indent(--depth) << '}';
}

Expand Down
68 changes: 66 additions & 2 deletions lang/c++/test/SchemaTests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include <boost/test/unit_test.hpp>
#include <boost/test/parameterized_test.hpp>


namespace avro {
namespace schema {

Expand All @@ -48,6 +47,7 @@ const char* basicSchemas[] = {
"{ \"type\": \"string\" }",

// Record
"{\"type\": \"record\",\"name\": \"Test\",\"fields\": []}",
"{\"type\": \"record\",\"name\": \"Test\",\"fields\": "
"[{\"name\": \"f\",\"type\": \"long\"}]}",
"{\"type\": \"record\",\"name\": \"Test\",\"fields\": "
Expand Down Expand Up @@ -136,6 +136,57 @@ const char* basicSchemaErrors[] = {
"{\"type\": \"fixed\", \"size\": 314}",
};

const char* roundTripSchemas[] = {
"\"null\"",
"\"boolean\"",
"\"int\"",
"\"long\"",
"\"float\"",
"\"double\"",
"\"bytes\"",
"\"string\"",
// Record
"{\"type\":\"record\",\"name\":\"Test\",\"fields\":[]}",
"{\"type\":\"record\",\"name\":\"Test\",\"fields\":"
"[{\"name\":\"f\",\"type\":\"long\"}]}",
"{\"type\":\"record\",\"name\":\"Test\",\"fields\":"
"[{\"name\":\"f1\",\"type\":\"long\"},"
"{\"name\":\"f2\",\"type\":\"int\"}]}",
/* Avro-C++ cannot do a round-trip on error schemas.
* "{\"type\":\"error\",\"name\":\"Test\",\"fields\":"
"[{\"name\":\"f1\",\"type\":\"long\"},"
Copy link

@zivanfi zivanfi Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Comment style is inconsistent, some lines start with a *, while others don't.

"{\"name\":\"f2\",\"type\":\"int\"}]}"
*/
// Recursive.
"{\"type\":\"record\",\"name\":\"LongList\","
"\"fields\":[{\"name\":\"value\",\"type\":\"long\"},"
"{\"name\":\"next\",\"type\":[\"LongList\",\"null\"]}]}",
// Enum
"{\"type\":\"enum\",\"name\":\"Test\",\"symbols\":[\"A\",\"B\"]}",

// Array
"{\"type\":\"array\",\"items\":\"long\"}",
"{\"type\":\"array\",\"items\":{\"type\":\"enum\","
"\"name\":\"Test\",\"symbols\":[\"A\",\"B\"]}}",

// Map
"{\"type\":\"map\",\"values\":\"long\"}",
"{\"type\":\"map\",\"values\":{\"type\":\"enum\","
"\"name\":\"Test\",\"symbols\":[\"A\",\"B\"]}}",

// Union
"[\"string\",\"null\",\"long\"]",

// Fixed
"{\"type\":\"fixed\",\"name\":\"Test\",\"size\":1}",
"{\"type\":\"fixed\",\"namespace\":\"org.apache.hadoop.avro\","
"\"name\":\"MyFixed\",\"size\":1}",
"{\"type\":\"fixed\",\"name\":\"Test\",\"size\":1}",
"{\"type\":\"fixed\",\"name\":\"Test\",\"size\":1}"
};



static void testBasic(const char* schema)
{
BOOST_CHECKPOINT(schema);
Expand All @@ -154,6 +205,19 @@ static void testCompile(const char* schema)
compileJsonSchemaFromString(std::string(schema));
}

// Test that the JSON output from a valid schema matches the JSON that was
// used to construct it, apart from whitespace changes.
static void testRoundTrip(const char* schema)
{
BOOST_CHECKPOINT(schema);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other tests have been updated to use BOOST_TEST_CHECKPOINT, please update this as well.

avro::ValidSchema compiledSchema = compileJsonSchemaFromString(std::string(schema));
std::ostringstream os;
compiledSchema.toJson(os);
std::string result = os.str();
result.erase( std::remove_if( result.begin(), result.end(), ::isspace ), result.end() ); // Remove whitespace
Copy link

@zivanfi zivanfi Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing all whitespaces, consecutive whitespaces should be collapsed into a single space. It is true that it doesn't matter how many whitespaces there are, but it does matter whether there is or isn't whitespace between specific characters. A slight modification will give the desired result as described in http://stackoverflow.com/a/8362145/5613485

Additionally this line uses a different style than the other lines, there should be no spaces after '('-s or before ')'-s.

BOOST_CHECK(result == std::string(schema));
}

}
}

Expand All @@ -173,6 +237,6 @@ init_unit_test_suite(int argc, char* argv[])
ADD_PARAM_TEST(ts, avro::schema::testBasic_fail,
avro::schema::basicSchemaErrors);
ADD_PARAM_TEST(ts, avro::schema::testCompile, avro::schema::basicSchemas);

ADD_PARAM_TEST(ts, avro::schema::testRoundTrip, avro::schema::roundTripSchemas);
return ts;
}