ninjudd / clojure-protobuf

Google protocol buffers wrapper for Clojure.
Eclipse Public License 1.0
217 stars 70 forks source link

Bug : Mixed Case Keys do not work. Naming Strategy has no effect #45

Open eRaz0rHead opened 11 years ago

eRaz0rHead commented 11 years ago

a Message with mixedCase fields will have those fields omitted when printing the message.

When printing the schema for the message, the key is printed as all-lower-case, however the value is only associated with the mixed-case key.

When printing the message out, it seems the toString method is referencing the lower-case key from the schema, which returns nil and therefore omits the key:value pair.

This may be related to #39

eRaz0rHead commented 11 years ago

Additionally - this means that the default Naming Strategy does not work as designed.
Moreover, changing the naming strategy as suggested in #27 does not work.

eRaz0rHead commented 11 years ago

Consider this test : MixedCaseTest.proto

option java_package = "org.flatland.test";

package Data;

message MixedCase
{
    optional string shouldpass = 1;
    optional string mightFail = 2;
}

And this code

(ns ticket-portal.services.mixed-case)
(use 'flatland.protobuf.core)
(import 'org.flatland.test.MixedCaseTest$MixedCase)
(def mix-test (protodef MixedCaseTest$MixedCase))
(print (protobuf-schema mix-test))
{:type :struct, :name Data.MixedCase, :fields {:shouldpass {:type :string}, :mightfail {:type :string}}}nil

(def p (protobuf mix-test :shouldpass "pass" :mightfail "fail"))
p
{:shouldpass "pass", :mightfail "fail", :mightfail "fail"}

 (def p (protobuf mix-test :shouldpass "pass" :mightFail "fail"))
p
{:shouldpass "pass"}

using the "plain" naming strategy e.g.

(def mix-test (protodef MixedCaseTest$MixedCase
                         (reify flatland.protobuf.PersistentProtocolBufferMap$Def$NamingStrategy
                          (protoName [this clojure-name]
                            (name clojure-name))
                          (clojureName [this proto-name]
                            (keyword proto-name)))))

has no effect on the above test

Moreover, when loading from bytes the mixedCase key will be the one used in the internal map, thus hiding the data.

eRaz0rHead commented 11 years ago

Ok - turns out this is a "reading the code failure" on my part. The Documentation does not mention the new way of defining naming strategies in a map of options.

(def mix-test (protodef MixedCaseTest$MixedCase
                         {:naming-strategy  (reify flatland.protobuf.PersistentProtocolBufferMap$Def$NamingStrategy
                          (protoName [this clojure-name]
                            (name clojure-name))
                          (clojureName [this proto-name]
                            (keyword proto-name)))}
                         ))

.. does work.

However, I would point out that this means that "naming strategies" might not actually work as intended.