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

Added MessagePackEncoder/Decoder for Codable support #61

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

andrew-eng
Copy link

Hi,

I am using MessagePack pod in my Swift project and it is great. However, currently it is quite painful to manually encode and decode my models.

I have written an implementation of MessagePackEncoder that heavily references JSONEncoder.swift from the swift foundation library. The structure and logic is almost identical to that of JSONEncoder.swift. However, it turns out that implementing a Encoder/Decoder is non-trivial and the logic is rather involved.

Therefore, I am not sure if incorporating this code into your project is the the right way forward as your library is very light weight.

Nevertheless, I'll submit a PR to further discuss this issue.


fileprivate func unbox(_ value: MessagePackValue, as type: UInt8.Type) throws -> UInt8? {
switch options.numberDecodingStrategy {
case .noTypeConversion: return value.unsignedIntegerValue.map { UInt8($0) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type cast will cause an EXC_BAD_INSTRUCTION if the underlying integer is larger than UInt8.max. The same applies for every other implementation of unbox that casts to integers smaller than Int64 (including Int which should not be assumed to be 64-bits).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, i'll fix this and add test cases for this

Copy link
Contributor

@mgadda mgadda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this implementation looks pretty much identical to JSONEncoder.swift. My only concern is that it is so similar that I wonder if it may be subject licensing requirements found in https://swift.org/LICENSE.txt. That is, could it be considered a derivative work of JSONEncoder.swift?

@andrew-eng
Copy link
Author

Thats a valid point, I think this is definitely considered as a derivative work of JSONEncoder.swift. A rewrite will be necessary to resolve this and it might take awhile. How is your implementation coming along?

@mgadda
Copy link
Contributor

mgadda commented Oct 11, 2017

@andrew-eng I'm not a lawyer of course. And perhaps there's case to be made that the protocol-based design of Codable basically requires that the code look pretty close to what JSONEncoder and PlistEncoder. So I'm not sure how much a rewrite will remedy the situation. But I think all you need to do is ensure that you follow the steps outlined in Redistribution of https://swift.org/LICENSE.txt and you should be fine.

As for my implementation, I've gone to a good deal of effort to simplify when possible. For example, because the MessagePack serializer is pure swift, there's no need to convert objects to or from anything deriving NSObject. In some places this reduces the total set of possible types that can be encoded or decoded from Any to just MessgePackValue. I skipped the storage class because it didn't seem to offer much value on top of what's already provided by Array. To be fair, though, it's still quite similar in structure to JSONEncoder, which again seems unavoidable.

<rant>
While Codable is a definite improvement over its predecessor, it would have been awesome if the designers had considered just how much duplication is required to actually implement new encoders and decoders. It seems to me like it wouldn't have been that much more work to go one step further and abstract away all that boilerplate from the layer responsible encoding and decoding. As it stands currently, most implementations of Encoder will likely almost exactly the same. This should been obvious when the effort required to implement PlistEncoder involved such a tremendous amount of copy and pasting from JSONEncoder (or vice versa).
</rant>

@mikecsh
Copy link

mikecsh commented Feb 16, 2018

Hello, are there still plans to merge this PR? Sound's very interesting!

@rustle
Copy link

rustle commented Feb 18, 2018

there are swift.org mailing lists where you could ask for guidance https://lists.swift.org/mailman/listinfo

@a2
Copy link
Owner

a2 commented Feb 22, 2018

@rustle @mgadda I've posted this topic on the Swift forums. Let's see what people say.

@andrew-eng
Copy link
Author

Hey guys, sorry for the late reply, recently I decided to write my own MessagePack parser because i encounter some performance issue when parsing large data. I also rewrote the Encoder/Decoder from scratch because the code in the above PR is indeed monstrous.

I am planning to port over the new Encoder/Decoder and submit a new PR in a few days time. Hopefully the new PR will be easier to manage and merge into this project.

@a2
Copy link
Owner

a2 commented Feb 23, 2018

@andrew-eng Sounds good. Would love to see the Encoder/Decoder stuff you're working on.

@cherrywoods
Copy link
Contributor

cherrywoods commented Feb 23, 2018

I would also love to see your implementation @andrew-eng! How did you manage to shrink the code?

@mgadda
Copy link
Contributor

mgadda commented Mar 27, 2018

@andrew-eng I also ended up abandoning this pure swift implementation because it was far too slow for my needs. I ended up using the C-based https://github.com/camgunz/cmp which was many orders faster and integrates easily into an existing swift project.

I did write some wrapping code which may or may not be useful for anyone wishing to use cmp. I'll post that in a separate repo this week. I just pushed it into a new swift project: https://github.com/mgadda/cmpencoder.

@andrew-eng
Copy link
Author

hey guys, I have submitted PR #66 containing a simplified Encoder/Decoder. Still haven't figured out why the tests ran successfully on the Linux machine but not on Mac. Will look at it over the next few days. @cherrywoods, i ended up not implementing several features that are not commonly used, turns out that simplified the code a lot. @mgadda nice find. I'll do a performance test against that. I wrote mine in pure swift while avoiding expensive operations. Curious to see how swift fare against good old C.

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.

None yet

6 participants