Specialized transcoding to/from Json for specific types about protobuf-csharp-port HOT 12 OPEN

I'm afraid I don't have any time to implement this in the short/medium term. We 
*could* potentially add some sort of optional interface, implemented via 
partial classes on the message and its builder, but it's not terribly ideal.

I was going to go with locally subclassing the JsonFormatWriter/Reader and add 
whatever I need to there, but was looking for some feedback on what would be 
ideal.

I tried with the interface, and I didn't like it that much.  Instead I tried 
again with a JsonConverter class, like Newtonsoft.Json (Json.Net) has and it 
seemed to work pretty well.

I did it by extending JsonFormatReader and JsonFormatWriter in my local project 
and adding the converter stuff there.  The trouble is that JsonFormatReader and 
JsonFormatWriter have key extension points locked down, like private inheriting 
concrete classes that make it impossible to extend without modification.

The first change to protobuf-csharp should be to just open those classes and 
move JsonTextWriter and JsonStreamWriter into a separate class so they don't 
inherit from JsonFormatWriter.  I'll put together a patch for that.

I'm certainly happy to look at a patch, but I personally generally prefer 
composition over inheritance in various places. I think I'll have to have a 
look at various options before putting anything into the code base.

Of course, you'd be welcome to use your own fork in the meantime :)

Can you please review the two commits I made?  I separated them, so you can 
decide how far you want to go with this.

The first commit, eb6d0ea94e05, enables the extensability needed, some of it is 
required even if the JsonConverter functionality is built in.
http://code.google.com/r/nathan-protobuf-csharp-port/source/detail?r=eb6d0ea94e0
58fdd31a1524df449f172e99f995c&name=issue-44

The second commit, 9d5c01bc5e20, builds on that and builds JsonConverter 
functionality in.
http://code.google.com/r/nathan-protobuf-csharp-port/source/detail?r=9d5c01bc5e2
00823c084c1773db7c3a49007d6b5&name=issue-44

Thanks

Nathan,

Not sure if we want to expose the inner workings of these classes as they could 
change drastically in the future.  This was the reason they are essentially 
sealed classes (you can't derive from them).

One viable option would be to replicate the entire JsonReader/Writer classes in 
your local project.  You should have no issues deriving from the 
AbstractTextReader/Writer classes to perform whatever magic you need.  This 
really isn't that much code and will isolate from changes to those classes in 
the future.

The other option of course is to implement the ICodedInput/OutputStream 
interfaces and aggregate the behavior of the json/xml reader/writer.  Mostly 
you will pass-through the calls directly to the underlying implementation.  The 
special casing should be able to be handled in WriteMessage(...) for the 
ICodedOutputStream, and ReadMessage(...) for ICodedInputStream.  I think this 
option would prove the most durable against future changes if you can make it 
work for you.

If you what to pursue the later option here let me know, I'll lend you what aid 
I can.

I agree with Roger - I'd rather not start making the guts internal. Once folks 
(such as yourself) have started taking dependencies on the implementation 
details, it's going to make it much harder to change them later.

Given that Roger's worked much more on the JSON than I have, I'd suggest you 
take him up on his offer of help ;)

Nathan,

The attached zip file contains three sources files than can be added to the 
"ProtocolBuffers.Test" project.  Two of these, AggregateInputStream.cs and 
AggregateOutputStream.cs are simple aggregations of the reader/writer 
interfaces.  The last file, TestCustomReaderWriter.cs, contains derivations 
that customize the behavior of ReadMessage and WriteMessage to perform the same 
behavior as your example unit test.  The test also demonstrates that this 
technique can be applied to any format.

If Jon agrees, I would be very willing to include the AggregateXxxxStream 
objects as these are quite difficult to produce due to the verbosity of the 
interfaces.  Additionally having these defined in the library would isolate 
implementations from minor interface changes.  

Anyway, take a look at the CustomMessageReader/CustomMessageWriter 
implementations for your example and let me know what you think.

I think I'd called it DelegatingMessageReader (etc) instead of Aggregate - it's 
not aggregating multiple streams, after all. But the overall approach seems 
sound, yes. I think such code will always have to be written pretty carefully, 
mind you.

I'd also prefer the InnerStream to be a property instead of a protected field, 
but that's a minor detail :)

@Jon, No problem on the rename and use of property.

@Nathan, Have you had the chance to try this out and see if it works for you?