Problems with flatten on Java types
The bug
Our automated functional tests caught an exception in a block of functionality that needs to read a sequence of strings from a deeply nested YAML file structure.
(defn load-yaml-contents
[]
;; This is a mocked set of data from parsing the YAML file.
(map identity [["one" "two" "three"] ["four" "five"]]))
(-> (load-yaml-contents)
flatten
(reduce concat []))
=> (\o \n \e \t \w \o \t \h \r \e \e \f \o \u \r \f \i \v \e)
After flatten
we would have a list of strings but then when that sequence is reduced using concat
,
each string value is treated as a sequence of characters and each character is appended to the vector. This is not the result
that we want. I then had to question if this code ever worked. There was a unit test around this behavior that asserted the
result of this code produced a sequence of strings. This unit test was last touched when the code shipped and has been running
successfully in nightly builds. I was perplexed as to why the test passed but the code failed in an end to end test.
It turns out that a recent change introduced a wrapper around the YAML parsing that converted snakeyaml's Java types
into Clojure types. What happens when we mimic Java data structures as a return from load-yaml-content
?
(defn load-yaml-contents
[]
;; This is a mocked set of data from parsing the YAML file.
(map identity [(doto (java.util.ArrayList.)
(.add "one")
(.add "two")
(.add "three"))
(doto (java.util.ArrayList.)
(.add "four")
(.add "five"))]))
(-> (load-yaml-content)
flatten
(reduce concat []))
=> ("one" "two" "three" "four" "five")
We can see that the result of processing the nested data structure is a simple sequence of all the data elements. This matches
the value our test suite was asserting and what the production code expected. Keen readers will start to
question the reduce
statement after the flatten
as a possible redundant statement. If we remove
the reduce
call, we can clearly see that flatten doesn't actually flatten out the nested data structures.
(flatten (load-yaml-contents))
=> (["one" "two" "three"] ["four" "five"])
Getting the code to run again is as simple as removing the reduce
or the flatten
stage in
the threading macro.
Why is flatten
broken?
It does seem like this ia bug in the clojure.core/flatten
function. Let's take a look at the source:
(defn flatten
"Takes any nested combination of sequential things (lists, vectors,
etc.) and returns their contents as a single, flat sequence.
(flatten nil) returns an empty sequence."
{:added "1.2"
:static true}
[x]
(filter (complement sequential?)
(rest (tree-seq sequential? seq x))))
So the implementation of flatten
only flatten out collections in x
if those collections implement
the clojure.lang.Sequential
interface. Obviously, Java types produced from Java libraries wouldn't implement a
Clojure specific interface. Complicating this issue is the ambiguous docstring:
Takes any nested combination of sequential things (lists, vectors, etc.)
In this case, "sequential" actually refers to "Sequential" (aka "clojure.lang.Sequential") and is not a generic use
of the term to mean any list of sequence of data. It's not actually clear here whether or not the intent was for
flatten
to work across data that implements java.util.List
or if the intent is to
restrict the use to only Clojure's native data types. In fact, this particular usage is counter to other built-in function
implementations like map
, reduce
, filter
, into
, doseq
,
for
, etc. Each of these functions is able to manipulate an ArrayList
the same way
that a vector is manipulated.
Why did the unit test not catch this?
Recall that I mentioned there was a unit test in our code base that was running with nightly builds and asserting that
the code in question was implemented correctly. The answer is simple - the test setup a mock return using
with-redef
that called snakeyaml directly. This meant that new logic added in other parts of the code
around the YAML parsing was not exercised as part of this test. Not mocking deep enough to fully exercise functionality
is a common and easy mistake to make.
Another fix to this code included a broader unit test that created a temporary file with the test content. The function
in our code base that returned the YAML file as a java.io.File
instance was redefined to return this
temporary file. Once that change was made to the code, the unit test started failing and it was then easy enough to make
corrections to the code and ensure they worked as intended.
Lessons learned
The first lesson that was (re)learned was about the dangers of mocks in unit tests. They can be invaluable in writing tests but their overuse or mocking the wrong object can lead to a test that misses cases or simply tests the ability of the author to create mocks.
The second lesson here is that the Clojure interop story is a bit more fragile than we might like to admit. To avoid odd
issues, data that is contained in Java objects should be repackaged into Clojure native types before being passed around
in a Clojure app. For objects, the bean
function should be used to produce a map of named fields. Lists and
sets should be transformed with into
. This also implies that Java code should be wrapped in thin facade
functions where this translation can happen.