API endpoints should be plural nouns.

#3849 (link to remind me to add to coding conventions)

unitPrice or pricePerUnit

As we've discussed in the past, I do not want to add a DTO layer unless there's a requirement to do so. Revert and make this the default toJson() method for ProductSupplier unless there's a compelling reason to add a DTO here.

We should either follow the existing conventions or spend time on v2.

REST Resource https://docs.grails.org/latest/guide/REST.html
JSON Views https://views.grails.org/latest/

However, since we haven't spiked Grails 3 REST support I would recommend following our existing conventions.

• Add Map toJson() to domain
• Add marshaller definition to Bootstrap
• Use BaseDomainApiController unless there's a need to override REST endpoints
• List and search should be implemented using separate controller actions

I expected you to comment on that one - the reason I did this was just to use types further in the method, not to use "defs" - as you can see, by doing so, I could easily say, what type will be returned from this method:

List<ProductSupplierListDto> getProductSuppliers(...)

I know I could just return the List<ProductSupplier> and to the toJson() parsing in the controller, but I thought this is a more convenient way to parse entity to POJO (in the service layer).
It was kind of instinct to do it this way, because if in Spring I wanted to do it this way, in the controller I'd already get the LazyInitializationException (because the transaction from the service commited). I don't know why it doesn't happen here, if we eventually have the osiv disabled.

Of course I can revert that, but just wanted to explain, why I added the dto here - in a nutshell - not to do the toJson() parsing in the controller and not to make the returned type as def.

I think we should investigate why we don't get LazyInitializationException here in the controller layer. I am pretty sure that we should get this error here. Why do you think @jmiranda?

@alannadolny to be clear - I didn't mean we should get the lazy inside the toJson() now - I meant that we should expect to get it when trying to do the .toJson() in the controller layer (because we are not anymore inside a transaction boundary).

Yeah, I meant doing .toJson() in the controllers

For now I'd like to go with the convention we've been using with the domain providing the toJson() implementation and just returning a map. I totally get what you want to do but I don't want to litter the code with DTOs if we end up refactoring and using JSON views. If you want to try to go down the JSON view path (or whatever Grails has annointed the standard), then give it a shot. Otherwise, just do things the way we have been doing them until we find time to implement it in a more standardized way.

As for the LazyInitializationException ... I suspose you're both right and I'm not exactly sure why we're not seeing the issue. Try to implement the toJson in the domain and have the toJson dig deep into an association that is not loaded in the criteria query.

I don't mind this, but the ProductSupplierListParams implementation seems to defeat the purpose of automatic data binding.

class ProductSupplierListParams extends PaginationParams {
    String product
    String supplier
    ...

In other words, we don't want to bind to String, we want to auto-bind to objects like Product, Organization, etc to allow GORM to help us with the object lookup.

If for some reason that wasn't working for you, then that means we don't have a proper Converter (previously known as a PropertyEditor) for that particular class. However, my understanding is that Grails 3+ has a default data binding mechanism that will take a command class like this

class ProductSupplierListCommand extends PaginationCommand {
    Product product
    Organization supplier
    ...

and bind the objects appropriatey.

Also, this is just a command object so just call it *Command unless we agree on a naming convention for these subtypes.

Sure it would work the way you described, I just thought there is no sense to bind the Product and Organization if I eventually use only the id for filtering, e.g.:

 if (params.product) {
    eq("product.id", params.product)
}

so I didn't see a sense to bind a whole product object if I eventually need only the id going further.

I appreciate you like the idea overall, because it's somewhat my own convention thrown at the deep end, as:

1. I didn't want to pass web params to the service layer, like we do in such methods now, because we had discussed this is not correct.
2. I didn't want to pass X amount of params to the service, because the method would look ugly (passing 10 params to a method)
3. By doing it in a "command way", we exactly know by looking at the code, what params user might provide and we don't have eventually to "debug" the web params in order to see, what is passed.
4. According to 3rd - we just have a full control of passed params and we know what will be binded - if a user makes mistake (e.g. he/she changes params in the URL), we don't proceed this param at all, because it won't be eventually binded to the command.

Let me know if my explanation about using String vs Product/Organization makes sense to you, or if I should try to check the equality by the object so:

 if (params.product) {
    eq("product", params.product)
}

but the question is if we really need the whole object here? 🤔

As for naming - I named it like this just because it's somewhat new convention to the api list endpoints and wanted to distinguish them somehow.

This method is insane. Kudos.

That is supposed to be a compliment, just in case it wasn't clear.

Let's move this to a method if possible, as it is going to need a lot of unit tests

Map sortOrder = getSortOrder(params.sort, params.order, usedAliases)
order(sortOrder.property, sortOrder.direction)

NOTE: I'm not confident with the naming of the properties in the sortOrder map, so if you think there's a more intuitive naming convention please let me know.

I will try if it is possible to do it in an easy way. I mean - I'd have to search for a package that would have the order static method available. It seems not to be available in the Restrictions.
I see there is a class Order from org.hibernate.Criterion so I will eventually try this one.