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

Initial Newtonsoft datum serializer #151

Merged
merged 18 commits into from
Nov 2, 2013
Merged

Initial Newtonsoft datum serializer #151

merged 18 commits into from
Nov 2, 2013

Conversation

bchavez
Copy link
Collaborator

@bchavez bchavez commented Oct 31, 2013

Initial implementation of Newtonsoft's datum serializer for RethinkDB.

In reference to #149.

Core Changes

Two file changes to the core rethinkdb-net core were made since I couldn't find any other way to "hook" the Newtonsoft serializer. I think this is related to #150. You'll see these changes in the following:

DefaultConnectionFactory.cs
ConfigurationAssembler.cs

I tried to set my serializer just soon after the connection was built, but I ran into an issue that the DatumConverterFactory on the connection could not be set due it being wrapped. So, please feel free to revert this change and make it more appropriate for #150.

Newtonsoft Datum Serializer

Generally speaking, the Newtonsoft Datum serializer uses rethinkdb-net's native datum converters for primitive types, nullables, and DateTimes. There was no sense in reinventing the wheel here.

Conceptually, Newtonsoft as uses a Reader and Writer to write formats.

The core of the serializer is DatumReader and DatumWriter that read and write datums, respectively.

DatumWriter

There is nothing too exciting with the DatumWriter, it was the first, and easiest to write. Basically, it uses a stack to keep track of the write signals we get from Newtonsoft. Ultimately, when everything is done writing, you can retrieve the root datum by calling GetRootDatum() as done by DatumConvert.SerializeObject().

The following DatumWriter.WriteValue(TimeSpan) overloads are _intentionally_ left not implemented:

        public override void WriteValue( TimeSpan value )
        {
            throw new JsonException( "TimeSpans can only be converted by including the DatumTimeSpanConverter in JsonSeralizerSettings. See source comment for: DatumTimeSpanConverter." );
        }

        public override void WriteValue( TimeSpan? value )
        {
            throw new JsonException( "TimeSpans can only be converted by including the DatumTimeSpanConverter in JsonSeralizerSettings. See source comment for: DatumTimeSpanConverter." );

        }

The reason for this is because we are doing custom serialization for TimeSpan using float seconds.

TimeSpan serialization is handled by a special TimeSpanConverter that must be added to the serializer's JsonSerializerSettings.

 new JsonSerializerSettings
                {
                    Converters =
                        {
                            new TimeSpanConverter()
                        },
                    ContractResolver = new CamelCasePropertyNamesContractResolver()
                };

See NewtonsoftDatumConverterFactory for how this is done. You'll notice the default JsonSerializerSettings is using CamelCasePropertyNamesContractResolver, this is so all properties get serialized with camel case property names.

DatumReader

The reader is somewhat more complicated. Took me a few times to get right.

The meat of the DatumReader is the Read() which calls ReadInternal() which stages / syncs, the internal JsonReader state machine.

Given a CurrentState:

  • You must SetToken() appropriately and return true if there is more to deserialize.
  • Or, you must return false to signal the end of deserialization.

Sometimes this simple state checking can get short-circuited when Newtonsoft knows more about the "type" it's reading. Newtonsoft can call ReadAsDateTime() in the middle of reading which kinda throw things off when you're expecting a Read(). This is why the ReadInternal has a signature ReadInternal(ReadAs? readAs = null). If ReadAs is set, it's an indication that Newtonsoft is telling us to do a more "forceful" read of the underlying data. ReadAs is eventually carried to ReadAsSpecialType where special case datums are converted (for example DateTime $reql_type$).

More about TimeSpanConverter

Don't attempt to squeeze the TimeSpan conversion code into the DatumReader because it won't work. There's not enough type information at read time to differentiate what is currently being deserialized: a TimeSpan or a decimal.

This is why it's essential for the TimeSpan conversion code to exist as a TimeSpanConverter. Otherwise, dangerous dragons will be met.

Keep DefaultSeralizerSettings public

One last note, it's really important keep the DefaultSeralizerSettings for the serializer public so that it can be modified by the user. It's important because this is how the behavior of the Newtonsoft serializer can be modified. For example, the user might want to add an additional converter like StringEnumConverter which converts enums to their string representation instead of their underlying type values.

Other

  • FluentAssertions is referenced and added to rethinkdb-net-newtonsoft-test project to help with datum object graph comparisons. You'll see quite a few ShouldBeEquivalentTo() datum comparisons which itterate over the datum object graph that checks for datum equality between what is expected and what the Newtonsoft serializer does. Couldn't help it, it comes in really helpful. See http://fluentassertions.codeplex.com/documentation.

Thanks,
Brian Chavez

Initial implementation of Newtonsoft's datum serializer for RethinkDB.
@bchavez
Copy link
Collaborator Author

bchavez commented Oct 31, 2013

Also, please feel free to write more integration tests as needed. I only wrote a few just to see that everything was working.

@mfenniak
Copy link
Owner

mfenniak commented Nov 1, 2013

Hi @bchavez,

I have a few review comments. Most of these are little issues that I ran into when pulling down the PR and running through the tests:

  • The .sln file has been upgraded to Visual Studio 2012. Could you change the version of the sln file back to 11? 2012 sln files don't work with the version of MonoDevelop that Debian & Ubuntu currently distribute.
  • Similarly, the csproj files for the new assemblies have ToolsVersion="12.0"; can you change that to ToolsVersion="4.0"? Shouldn't have any functional impact, but makes it work in MonoDevelop again.
  • build.sh should be updated to include the new test assembly on the nunit-console.exe command line.
  • Integration tests in rethinkdb-net-newtonsoft-test don't operate the same as rethinkdb-net-test, in automatically starting up RethinkDB and having the config file match the started server. Probably easy to fix by copying IntegrationTestSetup into rethinkdb-net-newtonsoft-test, but if there's a way it can be done without a wholesale copy that'd be great.
  • bin and obj directories of the new assemblies should be included in .gitignore.
  • Logging category in the new test assembly should be changed to Warning, not Debug.

I'm not a big fan of the static ConnectionFactoryProvider. Global statics like this can make a library unusable if there are two subsystems in an application that require different settings. So, this will have to come out as part of #150, but it's "alright" for now. One alternative that I might ask you to consider, how would you feel about just copying the ConfigurationAssembler and customizing it in the newtonsoft asssembly? That way we wouldn't have to remove an API as part of #150 (which isn't really a big deal, though), and a user of the newtonsoft approach would just need to code against the correct ConfigurationAssembler.

@mfenniak
Copy link
Owner

mfenniak commented Nov 1, 2013

Oh, I forgot to mention in that last comment: this is awesome work, and thank-you for sharing it. :-)

@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

Hi @mfenniak,

Could you please provide more information about your Mono/MonoDevelop build environment?

What version of MonoDevelop are you running on Ubuntu?
What version of Mono core are you running on Ubuntu?

Also, what version of Ubuntu are you running 12.04/10 or 13+?

Otherwise, I might not be able to test and make these changes.

@mfenniak
Copy link
Owner

mfenniak commented Nov 2, 2013

I think primarily you'd need MonoDevelop 3.0.3.2 to test the changes; I think all the of the problems were caused by that. I wish I could run a more recent version of MonoDevelop, but it's been difficult to build a newer version or find a more up-to-date package. I'm also running Mono 3.2.3.

Sorry about this; these changes are dumb to have to make.

@karlgrz
Copy link
Collaborator

karlgrz commented Nov 2, 2013

They are most certainly NOT dumb, says the Linux lover over here :-)
On Nov 1, 2013 8:43 PM, "Mathieu Fenniak" notifications@github.com wrote:

I think primarily you'd need MonoDevelop 3.0.3.2 to test the changes; I
think all the of the problems were caused by that. I wish I could run a
more recent version of MonoDevelop, but it's been difficult to build a
newer version or find a more up-to-date package. I'm also running Mono
3.2.3.

Sorry about this; these changes are dumb to have to make.


Reply to this email directly or view it on GitHubhttps://github.com//pull/151#issuecomment-27612431
.

@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

I was able to install MonoDevelop 3.0.3.2 on Ubuntu 13.10.

However, I'm having trouble installing Mono 3.2.3.

Currently, I think 2.10.8.1 is only available through normal distribution channels?

Are you using a custom PPA or build?

MonoDevelop 3.0.3.2
Installation UUID: b9cae7bc-8534-4112-9d39-cfa61a071242
Runtime:
    Mono 2.10.8.1 (Debian 2.10.8.1-5ubuntu2) (64-bit)
    GTK 2.24.20
    GTK# (2.12.0.0)
Build information:
    Git revision: 7bf6ac0ca43c1b12703176ad9933c3484c05c84c-dirty
    Build date: 2012-08-24 05:44:11+0000
Operating System:
    Linux
    Linux cowboy-vm 3.11.0-12-generic #19-Ubuntu SMP Wed Oct 9 16:20:46 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

Thanks,
Brian

@mfenniak
Copy link
Owner

mfenniak commented Nov 2, 2013

Let me clarify what I meant by dumb. :-) I think Linux/Mono support for rethinkdb-net is critical; as RethinkDB isn't even available on Windows, it seems (more) likely to me that RethinkDB users are Mono users. Not all, of course.

But what I'd like to have is a list of IDEs to use w/ rethinkdb-net, documented somewhere. Visual Studio X, MonoDevelop Y, SharpDevelop Z, whatever else. Then make the .sln and .csproj files work on all of those, and hopefully not have this problem occur where I'm asking for random changes to files to work with my random version of MonoDevelop. :-)

Sorry @bchavez, I should have specified that my mono is self-built from source.

@karlgrz
Copy link
Collaborator

karlgrz commented Nov 2, 2013

I was just giving you a hard time, nothing wrong with a good natured bit of
snark :-) I totally agree.

@bchavez, take a look here:
http://stackoverflow.com/questions/13365158/installing-mono-3-x-3-0-x-and-or-3-2-x
On Nov 1, 2013 8:52 PM, "Mathieu Fenniak" notifications@github.com wrote:

Let me clarify what I meant by dumb. :-) I think Linux/Mono support for
rethinkdb-net is critical; as RethinkDB isn't even available on Windows, it
seems (more) likely to me that RethinkDB users are Mono users. Not all, of
course.

But what I'd like to have is a list of IDEs to use w/ rethinkdb-net,
documented somewhere. Visual Studio X, MonoDevelop Y, SharpDevelop Z,
whatever else. Then make the .sln and .csproj files work on all of those,
and hopefully not have this problem occur where I'm asking for random
changes to files to work with my random version of MonoDevelop. :-)

Sorry @bchavez https://github.com/bchavez, I should have specified that
my mono is self-built from source.


Reply to this email directly or view it on GitHubhttps://github.com//pull/151#issuecomment-27612575
.

@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

@karlgrz Thanks! I'll try to get this figured out.

@karlgrz
Copy link
Collaborator

karlgrz commented Nov 2, 2013

No problem. If you have any specific mono questions feel free to ask away,
between all of us I think we can solve whatever problems you are running
into.
On Nov 1, 2013 8:55 PM, "Brian Chavez" notifications@github.com wrote:

@karlgrz https://github.com/karlgrz Thanks! I'll try to get this
figured out.


Reply to this email directly or view it on GitHubhttps://github.com//pull/151#issuecomment-27612615
.

@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

Hi, @mfenniak ,

Could you please take a look?

  • Reverted changes to core.
  • Newtonsoft now has its own ConfigurationAssembler.
  • All integration tests (any test derived from TestBase) now passes. I didn't like the idea of copying the tests either, so I made a small change to test base allowed overriding the connecitonFactory from my unit tests in Newtonsoft.
  • build.sh can now compile and build newtonsoft and run unit tests.
  • app.config reset to localhost RethinkDB and log category set to warn.
  • I have everything building and passing tests on:
=== MonoDevelop ===

Version 4.0.12
Installation UUID: b9cae7bc-8534-4112-9d39-cfa61a071242
Runtime:
    Mono 3.2.1 (Debian 3.2.1+dfsg-1~pre3+raring2) (64-bit)
    GTK 2.24.20
    GTK# (2.12.0.0)

Linux
Linux cowboy-vm 3.11.0-12-generic #19-Ubuntu SMP Wed Oct 9 16:20:46 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux
  • Also added VS2013 solution file.

Thanks,
Brian

@mfenniak
Copy link
Owner

mfenniak commented Nov 2, 2013

Looking good, but the new integration tests still don't startup RethinkDB correctly. The last commit you have failed to build with error, "Network related exception occurred while connecting to endpoint 127.0.0.1:55558: System.Net.Sockets.SocketException: Connection refused". (http://teamcity.codebetter.com/viewLog.html?buildId=97561&buildTypeId=bt991&tab=buildLog#_focus=1596)

The fourth point in my previous review about IntegrationTestSetup was this issue, but I probably didn't communicate it very clearly. :-) That IntegrationTestSetup class (https://github.com/mfenniak/rethinkdb-net/blob/master/rethinkdb-net-test/Integration/IntegrationTestSetup.cs) automatically starts RethinkDB when running all the test fixtures in the same namespace (eg. RethinkDb.Test.Integration). To get the same behavior in your new test assembly, I suggested copying that class into your assembly (although deriving off it might also work, if we make the new test assembly dependent upon the existing one).

Basically, you should be able to run build.sh on your Linux environment and have successful output. The current commit builds, runs the original tests fine, but fails on your new tests because RethinkDB isn't running.

…DB for Newtonsoft integration tests.

* And some general cleaning house, removing unnecessary comments.
@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

@mfenniak

Thanks. I was wondering why I was getting those network errors. :) But then again, it was pretty late last night.

Everthing is green on CI. Please check for review.

I think it's quite magical being able to serialize something like ComplextObject.

                new ComplexObject
                {
                    Name = "Brian Chavez",
                    ProfileUri = new Uri( "http://www.bitarmory.com" ),
                    CompanyUri = null,
                    Balance = 1000001.2m,
                    Clicks = 2000,
                    Views = null,
                    SecurityStamp = Guid.Parse( "32753EDC-E5EF-46E0-ABCD-CE5413B30797" ),
                    TrackingId = null,
                    LastLogin = new DateTime( 2013, 1, 14, 4, 44, 25 ),
                    LoginWindow = new TimeSpan( 1, 2, 3, 4, 5 ),
                    Signature = new byte[] {0xde, 0xad, 0xbe, 0xef},
                    Hours = new[] {1, 2, 3, 4},
                    ExtraInfo = new Dictionary<string, string>()
                        {
                            {"key1", "value1"},
                            {"key2", "value2"},
                            {"key3", "value3"},
                        },
                    Enabled = true,
                    Notify = null,
                    BinaryBools = new[] {true, false, true},
                    NullBinaryBools = new bool?[] {true, null, true},
                    SomeNumber = 1234
                };

mfenniak added a commit that referenced this pull request Nov 2, 2013
Newtonsoft.Json-based datum serializer
@mfenniak mfenniak merged commit 5db521f into mfenniak:master Nov 2, 2013
@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

@mfenniak
Copy link
Owner

mfenniak commented Nov 2, 2013

Documentation? You're a very unusual open source contributor. 😀

@bchavez
Copy link
Collaborator Author

bchavez commented Nov 2, 2013

Hi @mfenniak,

Can I update the README with links to the current Newtonsoft documentation?

Thanks,
Brian

@mfenniak
Copy link
Owner

mfenniak commented Nov 2, 2013

Sure, absolutely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants