Refactor getSingleResult() to getResultList()

[yvespp]

We should refactor code that calls getSingleResult() to getResultList() where the NoResultException is caught and it's ok if there is no result. See https://stackoverflow.com/questions/24045342/why-jpa-uses-javax-persistence-noresultexception

If the NoResultException is not caught directly in the method it can unintentionally lead to transaction rollbacks. See: http://glassfish.10926.n7.nabble.com/Unexpected-Behavior-NoResultException-rolls-back-transaction-td20819.html

I'm not sure what the best way to refactor it is:

• Just return null if there is no result
  • Can lead to unintended NullPointer exceptions if not carefully refactored.
  • In the REST services you have to explicitly check for null and convert that into a 404 response.
  • Can still be converted to a runtime exception by services if they can't deal with nulls.
  • It has to be check if there is more then one result.
    Today the NoResultException is automatically mapped to a 404. See REST AMW_rest/resources/resources/{resource}/lte/{RL} http 500 instead of 404 #421
• Return a custom checked exception. I experimented with this here: https://github.com/liimaorg/liima/compare/getResourceGroupByName_NoResultException
  • Exception has to be explicitly declared in method signatures
  • Null has to be checked for and converted to an exception
  • Can be mapped in REST

[mburri]

What about returning Optionals from the repositories?

    public String getResourceName(int resourceId) throws ResourceNotFoundException {
        ResourceEntity resourceEntity = resourceRepository.find(resourceId);
        if (resourceEntity == null) {
            throw new NoResultException("No Resource with id " + resourceId);
        }
        return resourceEntity.getName();
    }
}

would become:

        public String getResourceName(int resourceId) {
               return resourceRepository.find(resourceId)
                                 .map(ResourceEntity::getName)
                                 .orElseThrow(() -> new NoResultException("No Resource with id " + resourceId));
    }

[yvespp]

You mean returning optionals (your example doesn't do that)? Sure that could be another way.
For that all the method calls would have to be refactored and you still would have to map the optional to a http status in the rest service explicitly. Also maybe some calls still want an exception when nothing is found.

Maybe we have to split up some code into two methods: one method throws NoResultException and the other doesn't but can return null.

[mburri]

My example covers the resource, not the repsitory - which would (in this case) return an Optional - yes.
Returning optionals from the respository forces the caller to handle the possibilty that nothing was found - in contrast to the NoResultException thrown by getSingleResult()

Yes - we would have to refactor all calls and do the mapping explicitly - but the nice thing is, that the compiler will tell us about the places where we have to do this - compared to runtime exceptions...