-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
Initial implementation of Newtonsoft's datum serializer for RethinkDB.
Also, please feel free to write more integration tests as needed. I only wrote a few just to see that everything was working. |
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:
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. |
Oh, I forgot to mention in that last comment: this is awesome work, and thank-you for sharing it. :-) |
Hi @mfenniak, Could you please provide more information about your Mono/MonoDevelop build environment? What version of MonoDevelop 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. |
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. |
They are most certainly NOT dumb, says the Linux lover over here :-)
|
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?
Thanks, |
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. |
I was just giving you a hard time, nothing wrong with a good natured bit of @bchavez, take a look here:
|
@karlgrz Thanks! I'll try to get this figured out. |
No problem. If you have any specific mono questions feel free to ask away,
|
NewtonsoftDatumConverter now supports IObjectDatumConverter for ReQL statement conversions
* Split Reference and Value converters due to base class checking T. * ComplexObject tests pass. * Decorated ComplexObject ID property with one that allows auto-generated IDs by RethinkDB.
…h datums updated.
Hi, @mfenniak , Could you please take a look?
Thanks, |
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 Basically, you should be able to run |
…DB for Newtonsoft integration tests. * And some general cleaning house, removing unnecessary comments.
…he directory and mono pidb.
…b.Newtonsoft.Test.
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 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
}; |
Newtonsoft.Json-based datum serializer
Documentation added: https://github.com/mfenniak/rethinkdb-net/wiki/Newtonsoft-Serialization |
Documentation? You're a very unusual open source contributor. 😀 |
Hi @mfenniak, Can I update the README with links to the current Newtonsoft documentation? Thanks, |
Sure, absolutely. |
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:
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
andWriter
to write formats.The core of the serializer is
DatumReader
andDatumWriter
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 callingGetRootDatum()
as done byDatumConvert.SerializeObject()
.The following
DatumWriter.WriteValue(TimeSpan)
overloads are _intentionally_ left not implemented:The reason for this is because we are doing custom serialization for
TimeSpan
using float seconds.TimeSpan
serialization is handled by a specialTimeSpanConverter
that must be added to the serializer'sJsonSerializerSettings
.See
NewtonsoftDatumConverterFactory
for how this is done. You'll notice the defaultJsonSerializerSettings
is usingCamelCasePropertyNamesContractResolver
, 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 theRead()
which callsReadInternal()
which stages / syncs, the internalJsonReader
state machine.Given a
CurrentState
:SetToken()
appropriately andreturn true
if there is more to deserialize.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 aRead()
. This is why theReadInternal
has a signatureReadInternal(ReadAs? readAs = null)
. IfReadAs
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 toReadAsSpecialType
where special case datums are converted (for exampleDateTime
$reql_type$
).More about
TimeSpanConverter
Don't attempt to squeeze the
TimeSpan
conversion code into theDatumReader
because it won't work. There's not enough type information at read time to differentiate what is currently being deserialized: aTimeSpan
or adecimal
.This is why it's essential for the
TimeSpan
conversion code to exist as aTimeSpanConverter
. Otherwise, dangerous dragons will be met.Keep
DefaultSeralizerSettings
publicOne 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 likeStringEnumConverter
which convertsenum
s to their string representation instead of their underlying type values.Other
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