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

Simon24601 · 2016-05-23T23:33:14Z

A ValidSchema with no records produced invalid JSON when converting to JSON in C++. This is now fixed.

…no fields produced an invalid JSON

zivanfi

Hey Simon,

I noticed your pull request in the issue, it is a pity that it went unnoticed for so long. Thanks for the fix, if you are still willing to go forward with it, please see the suggestion in my comment.

Thanks,

Zoltan

zivanfi · 2016-09-21T11:44:15Z

lang/c++/test/SchemaTests.cc

+    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


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.

Simon24601 · 2016-09-21T23:45:57Z

Thanks Zoltan.
I certainly agree about the style - not sure why I have a different spacing
on that line.

With regards whitespace in general, the reason for eradicating whitespace
in this test is that the round-trip of JSON->Schema->JSON can insert exotic
whitespace like '\n' and '\r'. So we could have
{"type" : "double"}
being replaced with
{"type"\r\n:"double"\r\n}
In this case, compression to a single whitespace character isn't going to
allow these to compare equal - we need to convert all whitespace to ' '
first, then compress. Furthermore, whitespace in a schema is mostly
irrelevant, except inside tokens, so that
{"type" : "newType"}
and
{"type" : "new Type"}
are not considered identical. However, from what I understand of the Avro
specification, whitespace is not permitted inside tokens.

But if you think it's a change worth making, I'm happy to make it.

With regards the length of time it took to pick up this Jira, obviously I
could have been more aggressive in chasing it, but is there a separate
mailing list for e.g. C++ Avro or C# Avro developers? I have encountered
some issues in C# that I will be submitting pull requests for as soon as
I've written the appropriate tests, but I gather most devs are working on
the java implementation, so it would be better for me to target emails more
appropriately.

Thanks and regards
Simon

On Wed, Sep 21, 2016 at 12:49 PM, Zoltan Ivanfi notifications@github.com
wrote:

@Zicl requested changes on this pull request.

Hey Simon,

I noticed your pull request in the issue, it is a pity that it went
unnoticed for so long. Thanks for the fix, if you are still willing to go
forward with it, please see the suggestion in my comment.

Thanks,

Zoltan

In lang/c++/test/SchemaTests.cc
#97 (review):

@@ -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);

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

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.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#97 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIUPmn6QLWdduBVvQeRHPuPp18D5kTGmks5qsRnDgaJpZM4Ik-X4
.

zivanfi · 2016-09-22T12:51:49Z

Regarding the mixture of different whitespaces, my suggestion was meant to include replacing all of them with spaces before collapsing them. I should have mentioned it explicitly, sorry.

On the other hand, your argument is valid as well, and what's more, there are cases where the existence of a whitespace doesn't make a difference - for example "[{" vs. "[ {". I haven't noticed it yesterday, but your test cases do not contain spaces in any location where their removal would matter. As such, apart from the minor styling difference, the current implementation is fine by me.

Regarding your question about specific mailing lists for different programming languages, I don't believe they exist.

zivanfi · 2016-09-22T12:39:31Z

lang/c++/test/SchemaTests.cc

+        "{\"name\":\"f2\",\"type\":\"int\"}]}",
+/* Avro-C++ cannot do a round-trip on error schemas. 
+ * "{\"type\":\"error\",\"name\":\"Test\",\"fields\":"
+        "[{\"name\":\"f1\",\"type\":\"long\"},"


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

Simon24601 · 2016-09-22T23:11:52Z

Thanks for flagging up these style issues. I believe I've fixed them all now.
If you're happy with the fix, can you push it into avro/master? I don't have write-access.
Thanks for reviewing my changes.
Regards
Simon

zivanfi

Hi Simon,

I added a minor comment regarding BOOST_CHECKPOINT.

I also noticed that avro/lang/c++/build.sh doesn't run SchemaTests. I know that you only added one test case to this already existing test, but if it doesn't take too much of your time could you please update the shell script to actually run the test you extended? If you argue that it is out of scope, I can also do it in a follow-up change.

Thanks,

Zoltan

zivanfi · 2016-09-23T18:19:02Z

lang/c++/test/SchemaTests.cc

+// used to construct it, apart from whitespace changes.
+static void testRoundTrip(const char* schema)
+{
+    BOOST_CHECKPOINT(schema);


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

…ELinux (see Jira 1925). lang/c++/build.sh refers to the lang/c++/build directory which is empty. These are now fixed.

Simon24601 · 2016-09-24T01:02:07Z

Hi Zoltan,

I'm glad you asked for the fix to lang/c++/build.sh - I was going to start
a new fork to patch the build script (I opened Jiras 1925 and 1926 to raise
concerns about build scripts). So I've committed this and the change to
BOOST_TEST_CHECKPOINT.

I've also committed a minor change to the root level build.sh script. When
running "build.sh docker", it would load the docker container without
giving me write permission to most directories. This prevented building,
because compilation requires write access. This is due to SELinux default
policies (see Jira 1925 for details). Adding the :z that you will see in my
branch grants write access and makes it all work fine in SELinux. So when
you review my changes to the lang/c++/build.sh script, please review the
changes to the root-level build.sh.

Thanks
Simon

On Fri, Sep 23, 2016 at 7:33 PM, Zoltan Ivanfi notifications@github.com
wrote:

@Zicl requested changes on this pull request.

Hi Simon,

I added a minor comment regarding BOOST_CHECKPOINT.

I also noticed that avro/lang/c++/build.sh doesn't run SchemaTests. I know
that you only added one test case to this already existing test, but if it
doesn't take too much of your time could you please update the shell script
to actually run the test you extended? If you argue that it is out of
scope, I can also do it in a follow-up change.

Thanks,

Zoltan

In lang/c++/test/SchemaTests.cc
#97 (review):

@@ -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);

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

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#97 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIUPmuo2vUVNPgmkHN0G3SbifeHkCOcMks5qtBuTgaJpZM4Ik-X4
.

zivanfi · 2016-09-26T13:13:27Z

Hi Simon,

I feel that your fix for AVRO-1925 in the top-level build.sh is out of scope for this change, could you create a separate pull request for it instead?

Your change to lang/c++/build.sh is not exactly what I meant either. On my machine, ./build.sh test works correctly apart from not executing SchemaTests, which can be fixed simply by adding an && ./build/SchemaTests \ line. You seem to have addressed a different problem though by running the make command in the current directory instead of in build. This breaks the build script for me, as CMake generates no Makefile in lang/c++, only in lang/c++/build:

~/git/avro/lang/c++ $ git pull github pull/97/head
Updating fcd0a65..f7826c6
 4 files changed, 82 insertions(+), 19 deletions(-)
~/git/avro/lang/c++ $ ./build.sh test
-- Configuring done
-- Generating done
-- Build files have been written to: /home/zi/git/avro/lang/c++/build
make: *** No targets specified and no makefile found.  Stop.

Based on your description in AVRO-1926 it seems to me that things work differently on your computer: Your makefiles are not generated in lang/c++/build but in lang/c++ instead. I don't know why this happens so, but this behaviour seems to be specific to something in your environment and the fix you suggested would break it for other people.

Zoltan

…SchemaTests to the list of tests. Also revert SELinux changes to build.sh as these should be committed separately

Simon24601 · 2016-09-26T22:30:55Z

Hi Zoltan,

I've reverted the changes to the build.sh and lang/c++/build.sh files. I'm
not sure why I encountered the issues with the C++ tests, but reverting to
the master version and starting again has resolved it. I've also added the
SchemaTests to the build.sh file so these are now run as part of the build
process.

Apologies for the multiple changes - I hope this is converging on something
useful.
Thanks
Simon

On Mon, Sep 26, 2016 at 2:13 PM, Zoltan Ivanfi notifications@github.com
wrote:

Hi Simon,

I feel that your fix for AVRO-1925 in the top-level build.sh is out of
scope for this change, could you create a separate pull request for it
instead?

Your change to lang/c++/build.sh is not exactly what I meant either. On
my machine, ./build.sh test works correctly apart from not executing
SchemaTests, which can be fixed simply by adding an &&
./build/SchemaTests \ line. You seem to have addressed a different
problem though by running the make command in the current directory
instead of in build. This breaks the build script for me, as CMake
generates no Makefile in lang/c++, only in lang/c++/build:

~/git/avro/lang/c++ $ git pull github pull/97/head
Updating fcd0a65..f7826c6
4 files changed, 82 insertions(+), 19 deletions(-)
~/git/avro/lang/c++ $ ./build.sh test
-- Configuring done
-- Generating done
-- Build files have been written to: /home/zi/git/avro/lang/c++/build
make: *** No targets specified and no makefile found. Stop.

Based on your description in AVRO-1926 it seems to me that things work
differently on your computer: Your makefiles are not generated in
lang/c++/build but in lang/c++ instead. I don't know why this happens so,
but this behaviour seems to be specific to something in your environment
and the fix you suggested would break it for other people.

Zoltan

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#97 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIUPmvcTddbX_CM_EXfGgrq8oKy-7FpGks5qt8T5gaJpZM4Ik-X4
.

zivanfi

Hi Simon,

No problem, multiple rounds are normal in code reviews. Your contribution is definitely useful, thanks for your time!

Zoltan

zivanfi · 2016-09-28T11:40:58Z

Hi Simon,

Tom committed your change, could you please close the pull request?

Thanks,

Zoltan

…ity based on parsing canonical form (#97) I noticed that we were deriving PartialEq for Schema while my understanding is that the spec asserts that equality between schemas has to rely on parsing canonical form. So I manually impemented PartialEq and Eq based on that. I also created a bunch of issues to follow with the tests that I had to comment because they were failing.

https://docs.gradle.org/current/userguide/task_configuration_avoidance.html Thanks to [dcabasson](https://github.com/dcabasson) for the collaboration

Simon24601 and others added 3 commits May 23, 2016 23:22

AVRO-1849 Fix the issue where converting the schema of a record with …

a2718fe

…no fields produced an invalid JSON

Merge remote-tracking branch 'avroMain/master'

58ecc80

Merge branch 'master' of https://github.com/apache/avro

700af69

zivanfi suggested changes Sep 21, 2016

View reviewed changes

zivanfi suggested changes Sep 22, 2016

View reviewed changes

srw added 3 commits September 22, 2016 23:38

Fix style issues in the code.

39c5195

Merge branch 'master' of https://github.com/apache/avro

3f0ec2c

Merge branch 'master' of https://github.com/Simon24601/avro

d3d0aa8

zivanfi suggested changes Sep 23, 2016

View reviewed changes

srw added 2 commits September 24, 2016 01:47

Fix the build scripts; build.sh requires the :z addition to work on S…

e80fd1a

…ELinux (see Jira 1925). lang/c++/build.sh refers to the lang/c++/build directory which is empty. These are now fixed.

Update to use BOOST_TEST_CHECKPOINT

f7826c6

AVRO-1926 Revert changes to the lang/c++/build.sh script and add the …

3ad5aac

…SchemaTests to the list of tests. Also revert SELinux changes to build.sh as these should be committed separately

zivanfi approved these changes Sep 27, 2016

View reviewed changes

Simon24601 closed this Sep 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Simon24601 commented May 23, 2016

zivanfi left a comment

zivanfi Sep 21, 2016 •

edited

Simon24601 commented Sep 21, 2016

Zoltan

zivanfi commented Sep 22, 2016 •

edited

zivanfi Sep 22, 2016 •

edited

Simon24601 commented Sep 22, 2016

zivanfi left a comment

zivanfi Sep 23, 2016

Simon24601 commented Sep 24, 2016

Zoltan

zivanfi commented Sep 26, 2016

Simon24601 commented Sep 26, 2016

zivanfi left a comment

zivanfi commented Sep 28, 2016

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

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

Conversation

Simon24601 commented May 23, 2016

zivanfi left a comment

Choose a reason for hiding this comment

zivanfi Sep 21, 2016 • edited

Choose a reason for hiding this comment

Simon24601 commented Sep 21, 2016

Zoltan

zivanfi commented Sep 22, 2016 • edited

zivanfi Sep 22, 2016 • edited

Choose a reason for hiding this comment

Simon24601 commented Sep 22, 2016

zivanfi left a comment

Choose a reason for hiding this comment

zivanfi Sep 23, 2016

Choose a reason for hiding this comment

Simon24601 commented Sep 24, 2016

Zoltan

zivanfi commented Sep 26, 2016

Simon24601 commented Sep 26, 2016

zivanfi left a comment

Choose a reason for hiding this comment

zivanfi commented Sep 28, 2016

zivanfi Sep 21, 2016 •

edited

zivanfi commented Sep 22, 2016 •

edited

zivanfi Sep 22, 2016 •

edited