Support RethinkDB JSON protocol w/ sinesignal/ottoman's minimal JSON library #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mfenniak
changed the title
The state of JSON in .NET is a bit abysmal. I don't want to add new third-party dependencies like JSON.NET, the .NET's own JSON support is highly coupled to using data contracts rather than free-form objects, and I can't find a decent, simple parser that I can incorporate with the correct licensing and an approach that doesn't build an object model which I would have to translate into a Response object model. On the plus side, I benchmarked a simple JsonWriter against the Protobuf library for writing queries. It showed a bit of a speed-up, but not enough to salivate over. I'm shelving this work until RethinkDB threatens to kill the protobuf protocol. :-)
This branch is now partially working with the JSON protocol; some tests pass. There's obviously a few mistakes in that all tests don't pass, and there are a few FIXMEs for things that I've half-assed to get a start on, and there's a lot of code added to Connection that probably doesn't belong there. It might be a good idea to separate out the protocol details from the Connection class entirely, and make rethinkdb-net capable of using either protocol. Mostly that will make it clear what code is related to the JSON protocol, but it would also be easy to support both in the driver if there was a pressing need to.
This keeps support for V0_2, V0_3 / Protobuf, and V0_3 / JSON.
mfenniak
added a commit
that referenced
this pull request
Oct 24, 2014
Support RethinkDB JSON protocol w/ sinesignal/ottoman's minimal JSON library
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work-in-progress to convert to the JSON protocol.
Incorporated a small JSON library from sinesignal/ottoman, SineSignal.Ottoman.Serialization, with minor modifications; mostly deleting a few things that weren't required. This eliminates the need for us to add a large third-party dependency for a JSON library.
TODO:
Fix whatever issues are breaking unit tests.FIXME on ability to read backtraces from the server.FIXME around using the correct JSON encoding for text->bytes, currently using UTF-8 and need to verify that's correct. Also remove encoding creation on every command run.Converting Query objects into JSON, and JSON into Response objects, is currently intermingled in the Connection class. I'm thinking I'd like to separate that into a "JSONProtocol" class, and possibly even restore the protobuf-based code into it's own class. This would allow comparison between the code (eg. performance testing, bug tracking down), and maybe even runtime selection between the protocol (although I don't know why, it just seems practical). The biggest goal would be code cleanup first.Review all code changes and see if I've left anything else ugly lying around.Incorporate the requirements of the Apache License with regard to the code that will be redistributed from sinesignal/ottoman. Requirements in section 4 of the Apache License will be followed.Update release notes.This PR will address issue #175.