Cannot read own enum values

[loheander]

It seems that the code currently requires enums to be keywords in the original case when loading them from a map, but converts them to lower case as soon as they are loaded. This gives an error if you persist it to a database and then load it and try to convert it to protobuf again.

Minimum example:

example.proto

option java_package = "com.example";

package Example;

enum Answer {
    YES = 1;
    NO = 2;
    MAYBE = 3;
}

message Quiz {
    optional string question = 1;
    optional Answer answer = 2;
}

Failing test:

(ns proto-test.test.core
  (:use proto-test.core
        protobuf.core
        clojure.test))

(def Quiz (protodef com.example.Example$Quiz))

(deftest same-map-after-encoding
  (let [quiz-map {:question "Is it raining outside?"
                  :answer :MAYBE}
        quiz (protobuf Quiz quiz-map)]
    (is (= quiz quiz-map))
    (is (= quiz (protobuf Quiz quiz)))))

[ninjudd]

@amalloy does the naming strategy stuff you added help with this?

On May 30, 2012, at 2:07 AM, jheander wrote:

It seems that the code currently requires enums to be keywords in the original case when loading them from a map, but converts them to lower case as soon as they are loaded. This gives an error if you persist it to a database and then load it and try to convert it to protobuf again.

Minimum example:

example.proto

option java_package = "com.example";

package Example;

enum Answer {
   YES = 1;
   NO = 2;
   MAYBE = 3;
}

message Quiz {
   optional string question = 1;
   optional Answer answer = 2;
}

Failing test:

(ns proto-test.test.core
 (:use proto-test.core
       protobuf.core
       clojure.test))

(def Quiz (protodef com.example.Example$Quiz))

(deftest same-map-after-encoding
 (let [quiz-map {:question "Is it raining outside?"
                 :answer :MAYBE}
       quiz (protobuf Quiz quiz-map)]
   (is (= quiz quiz-map))
   (is (= quiz (protobuf Quiz quiz)))))

---

Reply to this email directly or view it on GitHub: https://github.com/flatland/clojure-protobuf/issues/27

[amalloy]

Yes, you can pass a naming strategy to the protodef call now which will let you convert from protobuf to keywords and back in whatever manner you like.

[ninjudd]

Great. @amalloy, can you post a quick example here?

[amalloy]

It should work to simply modify the protodef to use a naming strategy that doesn't change the protobuf names at all:

    (def Quiz (protodef com.example.Example$Quiz
                        (reify PersistentProtocolBufferMap$Def$NamingStrategy
                          (protoName [this clojure-name]
                            (name clojure-name))
                          (clojureName [this proto-name]
                            (keyword proto-name)))))

But I'm not totally sure I understand the problem behind this issue. @jheander, does this resolve your problem?

[loheander]

Well, I guess it would resolve the problem. However I really like the default naming strategy and the convenience of using idiomatic names within clojure. My problem is basically just that the naming transformations are not reversible. Currently I accept streamed protobuf as input, do some transformations in clojure and output protobuf again but the last step fails if I don't add an extra step where I explicitly convert all the enums back to uppercase (and remember which enums that are likely to be in the output). This feels a bit weird since its the same tool that is used for the whole chain :)

Maybe implement some kind of name lookup table that will map the lowercase keyword to the correct enum or disable case transformations for enums in the default policy like in previous versions of clojure-protobuf did?

[goodwink]

I agree with @jheander about the desirability of having it work both ways. I was surprised to find that

(protobuf Request :type :foo :action :get)

did not work even though that is what the repl shows me when I do a protobuf-load for that data.

[goodwink]

I added code to address this issue in https://github.com/goodwink/clojure-protobuf/commit/43c7cb213664ab690eca14bdfe641ef22c9cdfe4 if anyone else cares to use them or if you'd like me to open a pull request for it.

[loheander]

I think your updated convertUnderscores strategy is better than before since it now only does what the name advertises. It is also reversible much more often and generally seems in-line with how googles implementations for other languages choose to transform the names. And as a bonus it solves our common problem :)

The new naming strategy convertUnderscoresAndCase could probably break both enums and keys since it is not really reversible if the original name has mixed case (CamelCase or other weird convention :)) or if it is already lower-case. Since small-caps are recommended by google for keys it would probably break most keys, but work perfecly with enums.

// Johan

[goodwink]

@jheander agreed on the new naming strategy...the main problem is not having separate methods to handle enums and keys when their naming rules are different on the protobuf side. I worked around this by ignoring the google recommendation that keys be lower case and made them upper case just like enums, but this is at best temporary since it's ugly and impacts usability negatively in non-clojure clients. Probably the solution is either to expand the naming strategy functionality to allow separate naming strategies for keys and enums or else to just put up with upper-case on the clojure side for enums and only convert underscores to dashes.

[loheander]

Yes, actually for enums I kind of prefer the latter alternative to only convert underscores to dashes since this would make the keys idiomatic (if following the google recommendations of lowercase, underscore separated keys) and would keep the enums usable and also make it easy to convert the enums to integers, without first running it backwards through the naming strategy, if needed for DB storage:

(defn answer->int [kw]
  (.getNumber (Enum/valueOf Example$Answer (name kw))))

[goodwink]

I think the enums would still need to have dashes converted back to underscores in order to use them like that, right?

[loheander]

Yeah, you're right, didn't think of that first :) Guess you would have to use the naming strategy then anyways...

[richcole]

me too. I have the trouble that when I print a protobuf message and then to convert back into a protobuf structure it doesn't work for enums. That is the most important thing to fix. Secondarily I like the ideomatic names, so WORK_TYPE becomes :work-type. I understand such a conversion isn't well defined since it could be lossy if protobuf enums are case sensitive. If that is so, perhaps just do the conversion in print for "standard" enums, that is enums that are all the same case.