CodeSOD: ToArray, then REST a bit

Mandy's company was brought on to port some REST APIs to Java for a client. Those APIs were written in an in-house, proprietary programming language. Possibly this was a deployment of BobX? Regardless of what the in-house language was, it was everything one would expect from the Inner-Platform Effect. Weird, complicated, tortured logic.

Somewhere in Mandy's company, a pointy-haired-boss decided they'd throw a junior developer at this. "They did a REST API during their internship, how hard could translating this logic be?" Well, that junior developer didn't understand the ins-and-outs of Java that well. They certainly didn't understand the original APIs they were trying to port. The result is that they tried to follow the twisted logic of the in-house language while trying to fumble through Java.

One API method provided a map of countries: {"US": "United States", "DE": "Germany", "CN": "China","}. They wanted to map this into objects, in the structure {"label": "US", value: "United States"}. This was their solution.

private void mapCountries(BackendServiceResponse response, ApiResponse apiResponse) { Map<String, String> countries = response.getCountries(); int size = countries.size(); String[] stringArray = new String[size]; String[] keySet = countries.keySet().toArray(stringArray); String[] values = countries.values().toArray(stringArray); apiResponse.setCountries(executeMapping(keySet, values));}private List<Country> executeMapping(String[] keySet, String[] values) { List<Country> countries = new ArrayList(); for(int i = 0; i < keySet.length; i++) { Country country = new Country(); country.setLabel(keySet[i]); country.setValue(values[i]); countries.add(country); } return countries;}

Which gives us this. Note how they take the Map of countries and split it into the pairs of keys and values, using keySet and values. This is where the real problem lurks: there's no guarantee that the internal implementation of Map is going to guarantee that ordering. In fact, lots of possible internal implementations absolutely won't return keySet and values in the same order.

These split key/value pairs, which might be in the same order, are then passed off to another method to iterate across them. If, instead, they iterated across the original Map in the first method, the code would be cleaner, easier to understand, and wouldn't have the ordering problem. Of course, they could have also used the built-in entrySet method would would return a collection of sets, where each set was the key/value pair.

Except this code doesn't have the ordering problem, because it's not doing what it claims to do. The expectation was that it would return {"label": "DE", "value": "Germany"}. What it actually returns is {"label": "Germany", "value": "Germany"}, and that's down to the toArray call. toArray(), with no arguments, returns an Object[]. toArray(T[]) returns an array of type T, but not just returns an array: it uses that array as its allocation space. From the docs:

Returns an array containing all of the elements in this collection; the runtime type of the returned array is that of the specified array. If the collection fits in the specified array, it is returned therein. Otherwise, a new array is allocated with the runtime type of the specified array and the size of this collection.

So, stringArray is passed to collect the keys, and then passed to collect the values, and the end result is that it contains only the values. No one noticed. Either the tests weren't checking that, or the tests were assuming the incorrect behavior was correct. The end users of the RESTful API didn't notice the change. Regardless, it was not the behavior of the original API, which- despite being the kind of tortured logic which drives junior developers to drink- managed to stumble into correctly outputing label/value pairs for countries.

[Advertisement] Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!