Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Make avro serde accept clj maps with underscores, add option to skip mangling names. #126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Make avro serde accept clj maps with underscores, add option to skip mangling names. #126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6307689
Make avro serde accept clj maps with underscores,add option to skip m…
sathyavijayan Apr 18, 2019
8b7a894
restructure avro mangling tests to improve readability
sathyavijayan Jun 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/jackdaw/serdes/avro.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,20 @@
(when schema-str
(.parse (Schema$Parser.) ^String schema-str)))))

(def ^:dynamic *mangle-names*
"When true, record field names will be mangled during schema parse and
record de/serialization. Default value is `true`."
true)

(defn- ^String mangle [^String n]
(str/replace n #"-" "_"))
(if *mangle-names* (str/replace n #"-" "_") n))

(defn- ^String unmangle [^String n]
(str/replace n #"_" "-"))
(if *mangle-names* (str/replace n #"_" "-") n))

(defn- ->field-key
[field-name]
(keyword (unmangle field-name)))

(defn- dispatch-on-type-fields
[^Schema schema]
Expand Down Expand Up @@ -398,7 +407,7 @@
(comp (map first)
(map (fn [^Schema$Field field]
(let [field-name (.name field)
field-key (keyword (unmangle field-name))
field-key (->field-key field-name)
[_ field-coercion :as entry] (get field->schema+coercion field-key)
value (.get ^GenericData$Record avro-record field-name)]
(when-not field-coercion
Expand All @@ -425,7 +434,7 @@
new-k
(.getName schema))
{:path path, :clj-data clj-map})))
(let [[_ field-coercion] (get field->schema+coercion k)
(let [[_ field-coercion] (get field->schema+coercion (->field-key new-k))
new-v (clj->avro field-coercion v (conj path k))]
(.set record-builder new-k new-v))))

Expand All @@ -442,7 +451,7 @@
[schema->coercion ^Schema schema]
(let [fields (into {}
(map (fn [^Schema$Field field]
[(keyword (unmangle (.name field)))
[(->field-key (.name field))
[field (schema->coercion (.schema field))]]))
(.getFields schema))]
(RecordType. schema fields)))
Expand Down
193 changes: 193 additions & 0 deletions test/jackdaw/serdes/avro_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,199 @@
"topic"
(uuid/to-string uuid/+null+))))))))

(deftest accept-unmangled-input
(let [schema {:name "testRecord"
:type "record"
:fields [{:name "string_field"
:type "string"}
{:name "long_field"
:type "long"}
{:name "optional_field"
:type ["null" "int"]
:default nil}
{:name "nil_field"
:type "null"}
{:name "default_field"
:type "long"
:default 1}
{:name "bytes_field"
:type "bytes"}
{:name "enum_field"
:type {:type "enum"
:name "weird_values"
:symbols ["a_1" "B3"]}}
{:name "map_field"
:type ["null" {:type "map"
:values bananas-schema}]}
{:name "array_field"
:type ["null" {:name "subrecords"
:type "array"
:items "banana"}]}
{:name "uuid_field"
:type {:type "string",
:logicalType "uuid"}}]}
schema-str (json/write-str schema)
serde (->serde schema-str)]

(is (= (round-trip serde "bananas"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while before I realized that the difference between the 1st and 2nd round-trip tests here was that the enum field was expressed as a string in the first, and a keyword in the second. Would you mind putting each one inside a testing block so that they can more easily be distinguished?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have restructured the tests and added documentation to make them more readable.

I used a dynamic var for *mangle-names* as it made more sense for me to have it as an 'application-level configuration'. It will lead to a consistent low level design within an application - either use idiomised values or not. (although, I agree the degree of ambiguity wouldn't be much as there will always be a default setting).

I noticed that the mangling is done only for the serde and not for json etc., So, as you mentioned, it would make sense to drop it altogether and push the responsibility to the apps if they need such a feature. The dynamic var will be a nice way to provide backward compatibility during transition if you decide to drop mangling in a future release.

Let me know your thoughts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cddr, @creese any thoughts on this PR ? Let me know if you still like the dyn var to be moved to the topic-config map (last arg to the serde function) instead. The map is named topic-config which makes it a weird place for the mangle-names key - what do you think ?

{:string_field "hello"
:long_field 3
:optional_field 3
:nil_field nil
:default_field 1
:bytes_field (ByteBuffer/wrap (.getBytes "hello"))
:enum_field "a_1"
:map_field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}

:array_field [{:color "yellow"}]
:uuid_field uuid/+null+})
{:string-field "hello"
:long-field 3
:default-field 1
:nil-field nil
:bytes-field (ByteBuffer/wrap (.getBytes "hello"))
:map-field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}
:enum-field :a-1
:optional-field 3
:array-field [{:color "yellow"}]
:uuid-field uuid/+null+}))

(is (= (round-trip serde "bananas"
{:string_field "hello"
:long_field 3
:optional_field 3
:nil_field nil
:default_field 1
:bytes_field (ByteBuffer/wrap (.getBytes "hello"))
:enum_field :a_1
:map_field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}

:array_field [{:color "yellow"}]
:uuid_field uuid/+null+})
{:string-field "hello"
:long-field 3
:default-field 1
:nil-field nil
:bytes-field (ByteBuffer/wrap (.getBytes "hello"))
:map-field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}
:enum-field :a-1
:optional-field 3
:array-field [{:color "yellow"}]
:uuid-field uuid/+null+}))


(is (= (round-trip serde "bananas"
{"string_field" "hello"
"long_field" 3
"optional_field" 3
"nil_field" nil
"default_field" 1
"bytes_field" (ByteBuffer/wrap (.getBytes "hello"))
"enum_field" "a_1"
"map_field" {"banana" {"color" "yellow"}
"ripe b4nana$" {"color" "yellow-green"}}
"array_field" [{"color" "yellow"}]
"uuid_field" uuid/+null+})
{:string-field "hello"
:long-field 3
:default-field 1
:nil-field nil
:bytes-field (ByteBuffer/wrap (.getBytes "hello"))
:map-field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}
:enum-field :a-1
:optional-field 3
:array-field [{:color "yellow"}]
:uuid-field uuid/+null+}))))

(deftest no-mangling-test
(binding [jackdaw.serdes.avro/*mangle-names* false]
(let [schema {:name "testRecord"
:type "record"
:fields [{:name "string_field"
:type "string"}
{:name "long_field"
:type "long"}
{:name "optional_field"
:type ["null" "int"]
:default nil}
{:name "nil_field"
:type "null"}
{:name "default_field"
:type "long"
:default 1}
{:name "bytes_field"
:type "bytes"}
{:name "enum_field"
:type {:type "enum"
:name "weird_values"
:symbols ["a_1" "B3"]}}
{:name "map_field"
:type ["null" {:type "map"
:values bananas-schema}]}
{:name "array_field"
:type ["null" {:name "subrecords"
:type "array"
:items "banana"}]}
{:name "uuid_field"
:type {:type "string",
:logicalType "uuid"}}]}
schema-str (json/write-str schema)
serde (->serde schema-str)]

(is (= (round-trip serde "bananas"
{:string_field "hello"
:long_field 3
:optional_field 3
:nil_field nil
:default_field 1
:bytes_field (ByteBuffer/wrap (.getBytes "hello"))
:enum_field :a_1
:map_field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}

:array_field [{:color "yellow"}]
:uuid_field uuid/+null+})
{:string_field "hello"
:long_field 3
:default_field 1
:nil_field nil
:bytes_field (ByteBuffer/wrap (.getBytes "hello"))
:map_field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}
:enum_field :a_1
:optional_field 3
:array_field [{:color "yellow"}]
:uuid_field uuid/+null+}))

(is (= (round-trip serde "bananas"
{"string_field" "hello"
"long_field" 3
"optional_field" 3
"nil_field" nil
"default_field" 1
"bytes_field" (ByteBuffer/wrap (.getBytes "hello"))
"enum_field" "a_1"
"map_field" {"banana" {"color" "yellow"}
"ripe b4nana$" {"color" "yellow-green"}}
"array_field" [{"color" "yellow"}]
"uuid_field" uuid/+null+})
{:string_field "hello"
:long_field 3
:default_field 1
:nil_field nil
:bytes_field (ByteBuffer/wrap (.getBytes "hello"))
:map_field {"banana" {:color "yellow"}
"ripe b4nana$" {:color "yellow-green"}}
:enum_field :a_1
:optional_field 3
:array_field [{:color "yellow"}]
:uuid_field uuid/+null+})))))

(deftest schemaless-test
(let [serde (->serde nil)]
(is (= (round-trip serde "bananas" "hello")
Expand Down