OBPIH-6324 Fix downloading the product synonyms import template

[kchelstowski]

The issue at first was difficult to spot, as the error had been throwing in the generic transformObjects method in the DataService and it is working fine on Grails 1.
It turned out, that this line:

 List<Map> objects = [[]]

caused the problem, as the transformObjects method, and this particular line:

value = object.get(element) ?: element.tokenize('.').inject(object) { v, k -> v?."$k" }

expects to run the .get method on a Map, not on a List. This variable is even declared as List<Map>, but the default value for that is [[]], so "list inside the list", not map inside the list.

To fix that issue I just changed the [] to new HashMap<>(), not to refactor the generic method in the transformObjects, as in my opinion, it should also work when the list is empty, and it doesn't if I leave the objects as List<Map> objects = [].

[jmiranda]

Hmm, I feel like this should just be

List<Map> objects = []

because from what I understand the following

List<Map> objects = [new HashMap<>()]

does not do what I think you want it to do ... i.e. initializing a list where every element in a hashmap.

Instead it's creating a new list where the first element is a hash map.

You could probably do something like this to do what I think you want to do

List<Map> objects = [].withLazyDefault { new HashMap<>() }

[jmiranda]

Or perhaps for readability

List<Map> objects = [].withLazyDefault { [:] }

[kchelstowski]

@jmiranda it doesn't work with the withLazyDefault. If I understand the withLazyDefault correctly - it would return an empty map if we did something like this for the empty list: objects.get(200).
The problem is that the:

value = object.get(element) ?: element.tokenize('.').inject(object) { v, k -> v?."$k" }

expects to run the .get on the Map, not on the List, hence the .withLazyDefault doesn't help us in that case, as the object is expected to be a Map already.

To clarify one thing - it turns out that the empty list "works" - by "works" I mean that the transformObjects doesn't throw an error anymore, but then, the documentService.generateExcel method throws the IndexOutOfBoundException while trying to create a header of the document. I could fix that by using .getAt instead of .get in this line:

createExcelHeader(sheet, 0, data.get(0).keySet().toList())

but we would get a template without a header.

Why we need the list with an empty map initialized is that without it, the header of the template is not created.

Result of the transformObjects with the empty map initialized;

vs the result of the transformObjects with the .withLazyDefault:

tl;dr: it needs to stay as the list with one element, which is an empty map in order for the header of the template to be created (due to some logic in the transformObjects method.)

[jmiranda]

Got it! Thanks for the clarification.

So, correct me if I'm wrong but the solution you are advocating for i.e.

List<Map> objects = [new HashMap<>()]

works because we actually want to initial a list with one item (an empty map) and that first element (the empty map) somewhere gets populated in such a way that allows us to pull the header column names out to be displayed in the template.

Is that correct? If so, that sounds good to me.

A more readable initialization might be ...

List<Map> objects = [[:]]

but I'm fine with HashMap<>() as well.

I was under the impression that the problem was with the map not being initialized at all (for the 0th or Nth element in the list) and that was potentially causing problems.

[jmiranda]

To @kchelstowski's point

To fix that issue I just changed the [] to new HashMap<>(), not to refactor the generic method in the transformObjects, as in my opinion, it should also work when the list is empty, and it doesn't if I leave the objects as List objects = [].

I agree that this should work with the list being empty but see my other comment to see another workaround.

Aside: I really dislike the transformObjects method and want it to be smashed to pieces. It was written at a very strange time in my life. If we're being honest, I was probably pissed that we still didn't have access to Groovy 1.8 methods and I decided to take it out on that code. :sadsmile:

[awalkowiak]

Since @jmiranda's suggestion did not work, I am approving as is

[jmiranda]

Since @jmiranda's suggestion did not work, I am approving as is

To be fair, it was trying to solve a different problem. I'm cool with the current solution.