Fix public key token serialization issue #800
Conversation
|
|
||
| namespace Microsoft.Fx.Portability.Utils.JsonConverters | ||
| { | ||
| internal class JsonPublicKeyTokenConverter : JsonConverter<PublicKeyToken> |
There was a problem hiding this comment.
Where is this serialization used and what is it optimized for? Serializing as a string would be a much more efficient and easy to deal with encoding, but this looks like it would work.
There was a problem hiding this comment.
It's probably used in returning a response or sending a request. Must not have any tests associated with it so my change wasn't caught.
There was a problem hiding this comment.
@twsouthwick This PR fix the current problem if both client and server sides using the new binary contains PublicKeyToken struct, but if old version of client without knowledge of PublicKeyToken works with new Server, the serialization will still break. Looks like PR #784 broke backward compat serialization.
There was a problem hiding this comment.
You should be able to have a converter that would work for both. Do a string serialization (maybe that's what @marklio was referring to) and it should then work for backwards compatibility
There was a problem hiding this comment.
You can serialize with PublicKeyToken.ToString() and deserialize with PublicKeyToken.Parse(string). That should provide round tripping
There was a problem hiding this comment.
Submitted a new commit. Verified working with backward compat.
|
|
||
| namespace Microsoft.Fx.Portability.Utils.JsonConverters | ||
| { | ||
| internal class JsonPublicKeyTokenConverter : JsonConverter<PublicKeyToken> |
There was a problem hiding this comment.
It's probably used in returning a response or sending a request. Must not have any tests associated with it so my change wasn't caught.
twsouthwick
dismissed
their stale review
This should still ensure backwards compatibility
…s released ApiPort tool
|
dotnet-appport-dev-CI failed 14 tests for known SDK issue of ilasm is missing. |
#799, which is a breaking change introduced by PR #784