pinterest / riffed

Provides idiomatic Elixir bindings for Apache Thrift
Apache License 2.0
308 stars 37 forks source link

Problems of deserializing nils and enums #31

Closed jduan closed 8 years ago

jduan commented 8 years ago

I've been playing with Riffed and I encountered a problem when writing tests.

My service is a simple CRUD service that allows clients to create, get, update, and delete animals. (think of the service as a zoo service)

In the test where I try to get an animal that doesn't exist, the assertion fails because of:

  1) test get animal that doesn't exist (ZooServerTest)
     test/zoo_server_test.exs:26
     Assertion with == failed
     code: response.error() == error
     lhs:  %ZooServer.Models.Error{errors: [%ZooServer.Models.ErrorEntry{message: "animal not found", location: nil, reason: 7}]}
     rhs:  %ZooServer.Models.Error{errors: [%ZooServer.Models.ErrorEntry{message: "animal not found", location: :undefined, reason: %ZooServer.Models.ErrorReason{ordinal: :resource_not_found, value: 7}}]}
     stacktrace:
       test/zoo_server_test.exs:32: (test)

So there are two problems:

  1. The service doesn't specify a location so the client deserializes location as "nil". However, when I create an ErrorEntry manually on the client side, the location would default to ":undefined". The Riffed doc clearly says so but I wonder why struct fields won't default to nil?
  2. The 2nd problem relates to enums. When the client deserializes the reason, it sets it to 7. However, when I create an ErrorEntry manually, I use "Models.ErrorReason.resource_not_found" and I get a struct with ordinal and value. Obviously, you can't compare 7 with a struct object. So why won't Riffed or elixir-thrift deserializes 7 into a nice struct object? They should be able to do so based on number 7 I think.
scohen commented 8 years ago
  1. The erlang version uses :undefined as its nil sentinel. Automatically converting from :undefined to nil could be dangerous, if your intention was to serialize :undefined in the first place. I know it's not ideal, but that's how it is.
  2. You need to tell riffed that you want your reason to be converted into a struct. You do this with the enumerize_struct macro. There are some docs here: https://github.com/pinterest/riffed and It'll look like this:

    enumerize_struct ZooServer.Models.ErrorEntry, reason: ZooServer.Models.ErrorReason
    
    # or, if you're already in the context of ZooServer.Models...
    enumerize_struct ErrorEntry, reason: ErrorReason
jduan commented 8 years ago

@scohen thanks for your quick response. I fixed the code by doing the following:

  1. I explicitly set "location" to nil when creating an object so Riffed won't use :undefined. That makes the equality assertion pass.
  2. For the enum issue, I actually made a mistake myself. I should've had used "reason" in the enumerize_struct directive but I used "type" instead.

Thanks again!