Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Why is there no ByteString => T deserializer? #3

Closed
agemooij opened this issue Aug 18, 2013 · 16 comments
Closed

Why is there no ByteString => T deserializer? #3

agemooij opened this issue Aug 18, 2013 · 16 comments
Milestone

Comments

@agemooij
Copy link

I can do redis.set("key", MyCustomType(42)) as long as I have a valid RedisValueConverter in implicit scope for my custom type.

But when I do redis.get("key") all I get is a raw Option[ByteString] and I have to convert it back to my custom type myself.

Why is there not a get[T: RedisValueDeserializer](key: String): Future[Option[T]] with some basic support for common types (Strings, Ints, Longs, Booleans, Lists, Sets, Maps, etc.)? Then I can have my own RedisValueDeserializer in scope to deserialize my custom types.

@etaty
Copy link
Owner

etaty commented Aug 18, 2013

Yes good idea!
I publish version 1.1 then i will work on that next week.
If you want to do a pull request, you should wait version 1.1 ;)

@agemooij
Copy link
Author

For an example of type-class based (de)serialization, have a look at my Riak library: https://github.com/agemooij/riak-scala-client

The serialization stuff is here: https://github.com/agemooij/riak-scala-client/blob/master/src/main/scala/com/scalapenos/riak/RiakSerialization.scala

I guess you would need different types of serializers depending on the Redis data type but I'm no Redis expert.

@etaty
Copy link
Owner

etaty commented Aug 24, 2013

What do you think of that ?

// method api

  def getT[T: ByteStringDeserializer](key: String): Future[Option[T]] =
    send(Get(key))

  def get(key: String): Future[Option[ByteString]] =
    send(Get[ByteString](key))


// case class API

case class Get[T](key: String)(implicit bsDeserializer: ByteStringDeserializer[T]) extends RedisCommandBulk[Option[T]] {
  val encodedRequest: ByteString = RedisProtocolRequest.multiBulk("GET", Seq(ByteString(key)))

  def decodeReply(bulk: Bulk): Option[T] = bulk.response.map(bsDeserializer.deserialize)
}

// Example

      for {
        _ <- redis.set("getKey", "Hello")
        getBS <- redis.get("getKey")
        getString <- redis.getT[String]("getKey")
      } yield {
        getBS mustEqual Some(ByteString("Hello"))
        getString mustEqual Some("Hello")
      }

@agemooij
Copy link
Author

I would get rid of the original 'get' method and rename getT to get because it is much cleaner to provide a NoOp deserializer implementation for ByteString to ByteString deserialization. It would look something like this:

@implicitNotFound(msg = "Cannot find ByteStringDeserializer type class for ${T}")
trait ByteStringDeserializer[T] {
  def deserialize(data: ByteString): T
}

object ByteStringDeserializer extends LowPriorityDefaultByteStringDeserializerImplicits

trait LowPriorityDefaultByteStringDeserializerImplicits {
  implicit def noopDeserializer = new ByteStringDeserializer[ByteString] {
    def deserialize(data: ByteString): ByteString = data
  }
}

Note the low priority implicits trick that is very common in such situations where you want any implicit the user specifies to always win over your own defaults. Putting them one level deeper in the inheritance chain makes them lower priority but putting them in the companion object makes sure that your low priority default is always in scope. Ultimate easy of use combined with ultimate customizability :)

Your example would the look something like this:

case class SomeCaseClass(value: String)
object SomeCaseClass {
  implicit val deserializer = new ByteStringDeserializer[SomeCaseClass] {
    def deserialize(data: ByteString): SomeCaseClass = SomeCaseClass(data.utf8String)
  }
}

for {
  _ <- redis.set("key1", "Hello")
  _ <- redis.set("key2", SomeCaseClass(42))
  getBS <- redis.get("key1")
  getCustom <- redis.get("key2")
} yield {
  getBS mustEqual Some(ByteString("Hello"))
  getCustom mustEqual Some(SomeCaseClass(42))
}

I would use exactly the same design for implementing Serializers so you have a nice symmetric API.

@agemooij
Copy link
Author

Bye the way, looking at your current RedisValueConverter I notice that you are serializing all the primitives by first converting them to String. That is not very eficient. Try using akka.util. ByteStringBuilder instead.

Also have a look at the Akka docs for encoding/decoding binary data using ByteStrings: http://doc.akka.io/docs/akka/2.2.0/scala/io-codec.html

@etaty
Copy link
Owner

etaty commented Aug 25, 2013

RedisValueConverterconvert Numbers to String then ByteString, because Redis will allow operation like INCR : http://redis.io/commands/incr

...
Note: this is a string operation because Redis does not have a dedicated integer type. The string stored at the key is interpreted as a base-10 64 bit signed integer to execute the operation.
...

Thanks for LowPriorityImplicits trait, I didn't know about.
However, if i put another implicit in LowPriorityDefaultByteStringDeserializerImplicits i got a compilation error :

rediscala/src/test/scala/redis/commands/StringsSpec.scala:82: ambiguous implicit values:
[error] both object ByteString in trait LowPriorityDefaultByteStringDeserializerImplicits of type redis.ByteStringDeserializer.ByteString.type
[error] and object String in trait LowPriorityDefaultByteStringDeserializerImplicits of type redis.ByteStringDeserializer.String.type
[error] match expected type redis.ByteStringDeserializer[T]
[error] getString <- redis.getT("getKey")

I pushed a draft branch : https://github.com/etaty/rediscala/tree/bytestring-deserializer
I think the compiler need the return type at the end of the function, and can't get it infered by the environement.

@agemooij
Copy link
Author

The problem is in this call:

redis.getT(k)

This call does not specify the desired outcome type T so the compiler will look for an unambigious implementation of the type class and finds two instead of one. The solution is to call the method like this:

redis.getT[String](k)

If you plan to only provide the two default implementations you have now, then there would be a variation of the same trick that would help make it possible to call get without a type parameter, namely:

@implicitNotFound(msg = "Cannot find ByteStringDeserializer type class for ${T}")
trait ByteStringDeserializer[T] {
  def deserialize(bs: ByteString): T
}

object ByteStringDeserializer extends LowPriorityDefaultByteStringDeserializerImplicits

trait LowPriorityDefaultByteStringDeserializerImplicits extends LowerPriorityDefaultByteStringDeserializerImplicits {
  implicit object stringDeserializer extends ByteStringDeserializer[String] {
    def deserialize(bs: ByteString): String = bs.utf8String
  }
}

trait LowerPriorityDefaultByteStringDeserializerImplicits {
  implicit object byteStringDeserializer extends ByteStringDeserializer[ByteString] {
    def deserialize(bs: ByteString): ByteString = bs
  }
}

Also don't forget to add the @implicitNotFound annotation because it will help end users to find out what they are doing wrong (i.e. the forgot to create a deserializer for their custom type).

@etaty
Copy link
Owner

etaty commented Aug 25, 2013

Thanks :)

@etaty
Copy link
Owner

etaty commented Aug 31, 2013

I have a problem :

getBS <- redis.getT("getKey")
getString <- redis.getT("getKey")

The scala compiler don't know the output type. I have to give it explicitly.

So in the end, we always have to do :

get[String]("key")
get[ByteString]("key")
get[MyType]("key")

@agemooij
Copy link
Author

agemooij commented Sep 1, 2013

There is not really a way around this. If you want to get back a T from something that has been encoded without type information, then you will have to indicate which type you want to be produced. What is wrong with that? That is exactly the API that I would expect and want to find.

In my Riak client I chose to make this a two-step process, returning an untyped RiakValue, which has an as[T] method to perform the deserialization. There too the user will need to pass in the type they expect to be returned. This is a feature that comes back in pretty much all Scala data store client libraries.

@etaty
Copy link
Owner

etaty commented Sep 1, 2013

I added a ByteStringSerializer #4
So the API is now :

  def get[K: ByteStringSerializer, R: ByteStringDeserializer](key: K): Future[Option[R]] =
    send(Get(key))

//...

redis.get[String,String]("getKey") // Future[Option[String]]
redis.get("getKey") // Future[Option[ByteString]]

I don't know if i can improve the [String,String] to only [String]

@agemooij
Copy link
Author

agemooij commented Sep 1, 2013

Well, the first question I would ask is whether you really want people to use non-String keys. What usage scenarios do you see for that? I'm not a very experienced Redis user but I haven't seen any examples where that would make sense.

I would just use String keys and focus the Serializer/Deserializer pair purely on the values.

@etaty etaty closed this as completed in 6956850 Sep 3, 2013
@etaty
Copy link
Owner

etaty commented Sep 3, 2013

I kept the non String Key possible, but you have to use case class commands.
That way with the normal api, we can do redis.get[Int]("mykey")

@agemooij
Copy link
Author

agemooij commented Sep 3, 2013

Looks good!

@darshoka
Copy link

darshoka commented Apr 3, 2018

redis.get("mykey") is returning Future[Option[ByteString]] type , Then how i can deserialize and get the orginal data from the result ?

@darshoka
Copy link

darshoka commented Apr 3, 2018

why empty list " List() " is returning while using redis.get("myKey") ?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

3 participants