I'm not familiar with inject. I understand how it works but this return putawayItems + seems strange to me and IMO is less clear than a regular for loop. Is there some advantage that we get over just doing a for loop? I try to avoid using closures whenever possible since it really garbles up stack traces.

It's difficult for me to explain how .inject works if you are not familiar with JS, because it's somewhat the same what .reduce is in JS: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce
I will defend the immutable approach wherever possible, because I was taught coding this way instead of creating variables and mutating them usually with for loops (iterative approach).
So I'd say it's just a matter of preference and something that we probably shouldn't try to standardize, since everyone has their "own" style of coding (it doesn't mean we should not standardize anything, I just mean such the techniques of some instructions, not e.g. naming or folder structure or the things we've been trying to standardize recently).
I'm wondering though, what do you mean about garbling up the stack traces, could you elaborate or show any example where the loop would be more useful in terms of stack traces than the immutable methods? 🤔

it's also similar to foldLeft from other languages

ultimately I'm fine with your solution so won't ask you to change it.

We weren't allowed things like collection streams or lambdas in my last job unless it was absolutely required so I think I just got used to not using them. That was a very extreme decision that I don't agree with btw. I can't remember why they were so adamant about that... maybe there was some performance-related aspect.

I tend to think of static operations like this as "functional programming adjacent", and so they should be used for operations that themselves are largely static. I don't want to chain into other services from within a closure for example.

So my usual rule is: if it's a very simple mapping or reducing operation, then lambdas/closures are fine. Otherwise if we're doing complex operations, especially ones that could interface with a database, then do it via regular for loop.

Because we're doing lookups on inventory item and bin location which may result in database calls being made, I'd put this in a loop.

But again, I'm fine with your approach

as to the stack trace question, I tested it out with the following code:

InvoiceApiController.list()

List<PutawayItem> putawayItems2 = invoices.inject ([], { putawayItems2, binLocationEntry ->
    invoiceService.getInvoicesCsv(invoices)
    return putawayItems2 + new PutawayItem()
})

Then in InvoiceService.getInvoicesCsv I immediately throw new RuntimeException('oops!')

which output this:

Caused by: java.lang.RuntimeException: oops!
        ... a bunch of crap that's in both stack traces...
        at org.pih.warehouse.api.InvoiceApiController$_list_closure1.doCall(InvoiceApiController.groovy:41)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.springsource.loaded.ri.ReflectiveInterceptor.jlrMethodInvoke(ReflectiveInterceptor.java:1427)
        at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:98)
        at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
        at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:264)
        at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1034)
        at groovy.lang.Closure.call(Closure.java:420)
        at org.pih.warehouse.api.InvoiceApiController$_list_closure1.call(InvoiceApiController.groovy)
        at org.codehaus.groovy.runtime.DefaultGroovyMethods.inject(DefaultGroovyMethods.java:5518)
        at org.codehaus.groovy.runtime.DefaultGroovyMethods.inject(DefaultGroovyMethods.java:5459)
        at org.codehaus.groovy.runtime.dgm$339.invoke(Unknown Source)
        at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoMetaMethodSiteNoUnwrapNoCoerce.invoke(PojoMetaMethodSite.java:274)
        at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:56)
        at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:136)
        at org.pih.warehouse.api.InvoiceApiController.list(InvoiceApiController.groovy:40)
        ... 65 common frames omitted

switching the controller code to:

for (InvoiceList invoice : invoices) {
    invoiceService.getInvoicesCsv(invoices)
}

gives us this stack trace:

Caused by: java.lang.RuntimeException: oops!
        ... a bunch of crap that's in both stack traces...
        at org.pih.warehouse.api.InvoiceApiController.list(InvoiceApiController.groovy:41)
        ... 65 common frames omitted

so the for loop is a shorter stack trace by about 20 lines and is easier to read IMO. And the closure stack traces only get more and more complex as you add more chained calls within them. But I will admit that it's not nearly as bad as I originally thought and it's still readable enough with your solution.

So just to be clear, before we were doing a big select on internal locations, then filtering down to the bin locations, and now we're using the "supportedActivities" field in the bin location to avoid that first lookup.

Side note: Do you know how hasMany = [supportedActivities: String] is being stored at the database level?

There are two tables, location_type_supported_activities and location_supported_activities.

Aside: I just wrote a larger description of how supported activities work, but then realized that does not pertain to your question so I'm going to use self-restraint and refrain from posting 😛 But if you'd like to see more context, I'll post it somewhere more useful.

Note it would be better for the initial query (persumably against product_availability) to limit the inventory items returned based on the supported activities associated with the "bin location", but I suspect the query for that would be more complicated due to the inheritance relationship between the two supported actitivities tables. That's why I think we're pulling out everything and then filtering. But if we could move the "supports" method logic to the database query, that would improve performance even more. #butprobablyarefactoringforanotherday

I'm assuming this is pre-fetching the relationships in batches so that they're not lazy fetched one by one when creating the putaway item. Is this the recommended approach? A google search shows there's also these options:

Book.list(fetch:[authors:"eager"])

A.withCriteria() {
    fetchmode "cs", FetchMode.JOIN
    fetchMode "cs.b", FetchMode.JOIN
}

I'm not sure the differences behind the scenes but we should probably determine which of these eager fetching approaches we want to stick with for future refactors

Is this the recommended approach?

I would say there is no big difference in those approaches since they generate the same sql statement that joins the tables and pre-fetch the relationships so they do not create n+1 problem when accessing them in a loop.

In terms of the approach, I would say that the context is important - if you don't need any filtering logic, you could potentially use the Book.list(fetch:[authors:"eager"]), but if you need the filtering, I would say the .createCriteria + createAlias is the most readable and has been used by us since the Grails 3 migration and the lazy intialization exception problems we faced.
As for the fetchMode, we have it in a few places, and I assume they work, but surprisingly when I tried it some time ago, it didn't seem to work, so probably worth to investigate when there would be an occasion, but I would recommend the createAlias anyway.
One more possible approach is a data service with the @Join annotation (only possible for one association) or @Query annotation with the HQL query that joins assocations (you can check it out, we have it in a few places).

Maybe we should add the comment that was originally here about removing the available items?