HibernateService feels too broad. Maybe HibernateSessionService (if we're going to support both queries as well as inserts/updates/deletes) or HibernateQueryService (if it's for read-only queries).

If we ever plan to support different types of these services then maybe something like PersistenceService, PersistenceSessionService, or DatabaseSessionService as the interface (doesn't feel right though). And then maybe HibernateSessionService or HibernatePersistenceService as the impl. And the SQL query methods from the DataService (or wherever I put those) can move to something like a SqlPersistenceSessionService.

I'll rename to HibernateSessionService. I don't think we currently have a need to support multiple approaches (we'll likely prefer this service over DataService going forward) but it's a good idea to allow the potential to switch in the future if needed. Maybe we'd want to start using spring jdbctemplate for whatever reason. I'll look into adding an interface for it.

Just call this

setParameters(query, params)

or

bindParameters(query, params)

I think i prefer bindParameters.

I chose setParameters because that's what the underlying Query methods are called

We should probably discuss this. I haven't really thought it through but as a developer I might get annoyed if one of my filters isn't filtering results. So I might expect this to be mapped to an IS NULL or maybe throw an exception.

yeah that's fair. Maybe the simplest thing to do for now is just throw an exception

I know we don't need it now, but it might be nice to add all the methods we might need.

def executeUpdate(String sql, Map params = [:]) { 
    return createNativeQuery(sql, params).executeUpdate()
}


def uniqueResult(String sql, Map params = [:]) {
    return createNativeQuery(sql, params).uniqueResult()
}

And we might want to have special handling for pagination. Not sure if the list method is going to handle that explicitly, so check for max, offset and apply appropriately to the query.

I considered adding them in now, but decided against it because I didn't have a use case for those scenarios and so wouldn't be able to test them. What I can do is add those methods to the interface and then just throw not implemented exceptions so that when we eventually need them, we can implement them (as you described).

We can do pagination via:

if (paginate) {
    query.setFirstResult(command.offset).setMaxResults(command.max)
}

I left it out for now since I didn't need it but I'll give it some consideration

I tried for a while to make this use the PersistenceService interface but it wouldn't autowire in HibernateSessionService. According to the docs that I looked at it should have worked because I have the @Primary annotation on HibernateSessionService so I'm not sure...

That's really weird, we should investigate that. You could check if you use @Qualifier if it would autowire then, but still we shouldn't need to do that.....
Maybe interfaces don't become a Grails bean automatically and we have to do more things explicitly?

I think I figured it out. If I add the @Autowired annotation, it works. Maybe there's some Grails weirdness going on that makes it unable to resolve the bean so we need to rely on spring 🤷

I assume the Primary annotation is added in advance so that if we ever create a new implementation of that interface, we'll autowire this one?
I'm asking because it feels like currently it's redundant.

yup it does nothing now, but it's an easy way for us in the future to switch to a different implementation if needed. I'll remove it for now though for clarity

You may consider using PaginationParams here

That's really weird, we should investigate that. You could check if you use @Qualifier if it would autowire then, but still we shouldn't need to do that.....
Maybe interfaces don't become a Grails bean automatically and we have to do more things explicitly?