I think this should be null safe in case map won't have product

is there a reason you changed it from load() to get()? I assume maybe load() was used there on purpose, because of performance - it doesn't hit the DB immedietaly as get() and you are inside .each loop.

I was not aware of load() not making aN SQL query, I always thought that the difference between get and load was that load throws and exception when a record is not found in db while get just returns a null.

I have found such a table on stack explaining when to use which

thanks, @kchelstowski for pointing this out, maybe it will be a good idea to change it load to avoid future performance complications, however small they could be.

I'm fine we either approach, but load seems to be more tempting, but it does not look like it gives us a huge performance improvement. Overall I think we should use it more in the system.

I use load when I need to set a foreign key.

The fact that there's a ShipmentItem.get() invocation right before this means that the Product.load() was either arbitrary.

Please note that the following line

partialReceiptItem.product = shipmentItemMap.get("product.id") ? Product.load(shipmentItemMap.get("product.id")) : null

is different from this line

partialReceiptItem.product = Product.get(shipmentItemMap?.product?.id)

in more ways than just the get vs load. You may have inadvertently caused a regression with this change in the case where a product ID is not specified i.e. do we know for sure that product.id will always be not null? Otherwise, we're going to get a loud exception here instead of a null product reference.

I would recommend going back to the logic "if key exists, look up Product by key, otherwise return null"

cc @drodzewicz @awalkowiak @kchelstowski

@jmiranda are you sure it works the way you described? I don't remind myself get() throwing an error if I provide null as an argument.
I checked it via console:

For those, the only that will throw the exception will be load() called with an id that do not exist (but with null it surprisingly works fine)

@kchelstowski I was just pointing out that this code

def variable = checkSomething ? doSomething : null

is not equivalent to

def variable = doSomething

and that there might be side effects to using the latter.

what was the exact problem here?
Also, if you find the pallet, what's the reason to save it?

I'm asking, because we had a discussion that we shouldn't just throw flush: true everywhere if it is not necessary.

The problem was that when creating a shipment item and both provided box and pallet do not exist in the database, it creates both at that moment. When creating a box, it references the newly created pallet as a parentContainer. Since pallet is not yet saved in the database, and the save of all entities only happens at the end of the transaction, box references a non yet existing pallet as a parentContainer causing the parentContainer field to be empty.
So to solve this problem we need to make sure that in the moment of creating a box a pallet already exists.

I am not sure why this exact issue became a problem in grails 3, I wasn't able to find anything in the docs refferecnig this change, do you maybe have an idea ?

@kchelstowski I'd say shipment.findOrCreatePallet(palletName) is not saved properly, hence it is fine (see how this method looks currently in the code)

Ok, I get it, but:

1. flush: true doesn't make the changes being visible immediately in the DB if you are in the service layer
2. calling it without flush should be fine
3. I still don't get why we'd want to call save() if the findOrCreatePallet finds one (not create) - unnecessary save happens
4. if you do it for pallet, why don't you do it also for the box below? The same problem should be visible there
5. I think in Grails 1 it was working because of the OSIV - here, it's fine that you've added this save(), because it is a new entity, but I just wanted to point out that flush might be unnecessary here, and also that we might call the save() for no reason there, if the method finds the Container.

Yeah, I'm a little weary of this solution. I think a save within the findOrCreatePallet() would be more palatable ... pardon the pun. If not then we should look into an alternative solution.