alphaHeavy / protobuf

An implementation of Google's Protocol Buffers in Haskell.
http://hackage.haskell.org/package/protobuf
BSD 3-Clause "New" or "Revised" License
96 stars 18 forks source link

Repeated n (Enumeration a) difficulty #17

Closed jamesthompson closed 9 years ago

jamesthompson commented 9 years ago

I'm struggling to decode repeated Enumerations.

It seems that Optional Enumerations work fine, as per closed PR: https://github.com/alphaHeavy/protobuf/pull/12

{-# Language OverloadedStrings, DeriveGeneric, DataKinds#-}

import Data.Int
import Data.ProtocolBuffers
import Data.Text (Text)
import GHC.Generics (Generic)
import GHC.TypeLits

data Foo = Bar | Baz
  deriving (Enum, Eq, Show)

data Imaginary = Imaginary { x :: Optional 1 (Value Text)
                           , y :: Optional 2 (Enumeration Foo)
                           , z :: Required 3 (Value Text)
                           } deriving (Generic, Eq, Show)

instance Decode Imaginary

But it's not compiling for Repeated Enumeration fields like this, which compiles by protoc ok (java lib version 2.6.1):

data Foo = Bar | Baz
  deriving (Enum, Eq, Show)

data Imaginary = Imaginary { x :: Optional 1 (Value Text)
                           , y :: Repeated 2 (Enumeration Foo)
                           , z :: Required 3 (Value Text)
                           } deriving (Generic, Eq, Show)

instance Decode Imaginary

yields

    No instance for (protobuf-0.2.0.4:Data.ProtocolBuffers.Wire.DecodeWire
                       (Enumeration Foo))
      arising from a use of ‘protobuf-0.2.0.4:Data.ProtocolBuffers.Decode.$gdmdecode’
    In the expression:
      protobuf-0.2.0.4:Data.ProtocolBuffers.Decode.$gdmdecode
    In an equation for ‘decode’:
        decode = protobuf-0.2.0.4:Data.ProtocolBuffers.Decode.$gdmdecode
    In the instance declaration for ‘Decode Imaginary’

I wonder if this is a bug, or just not implemented yet? Any help anybody could give me for this would be greatly appreciated as we need to have repeated enumeration fields if possible for our application. This library otherwise is brilliant and we hope to employ it in production as soon as possible!

Thanks a lot in advance for your help.

NathanHowell commented 9 years ago

If there aren't any tests it may not work. I'll take a look this evening unless someone else does first.. I'd imagine a fix will be very simple.

jamesthompson commented 9 years ago

Absolutely brilliant! Thank you very much indeed for your help!

NathanHowell commented 9 years ago

I have something that compiles but is absolutely not tested at all, if you have a chance to check it out (and improve test coverage, and/or make it work if it's broken) that would be really awesome. The relevant bits of code are just a few lines: 401b3b3875e0f73181714cb5ad28c5e911846dd3

jamesthompson commented 9 years ago

I'm going to try to test this as soon as possible. Thank you very much indeed for the work!

jamesthompson commented 9 years ago

Looks like this still isn't working, I'm getting this when y = [Bar] in the compiled protobuf data.

y = Field {runField = Repeated {runRepeated = [Enumeration {runEnumeration = *** Exception: toEnum{y}: tag (1) is outside of enumeration's range (0,0)

I'll see if I get a chance to dig in further this weekend

--update

On further inspection, it seems that this is almost working. The decoding works, but Enum's are apparently 0-indexed, whereas the tags are 1-indexed in protobufs. If you have a set of ADT constructors > 1 in size, you're golden. Hopefully I can fix the instance you created and update with tests.

jamesthompson commented 9 years ago

Ok. I figured out what was causing the strange behaviour that I was seeing. It turns out Enumerations are 0-indexed, whereas message fields are 1-indexed in protocol buffers. See the protobuf ref for enumerations.

I can confirm that this code does indeed work! I'm planning on adding a few tests for the expected behaviour. Am I able to push to your fix branch?

jamesthompson commented 9 years ago

@NathanHowell I've added a PR for your repeated-enumerations branch which adds tests for this case https://github.com/alphaHeavy/protobuf/pull/18

This code works and should be able to be merged into master.

Thank you very much again for your help!

NathanHowell commented 9 years ago

Just pushed this to Hackage: http://hackage.haskell.org/package/protobuf-0.2.1.0. Thanks again!