get() method which throws when entity is not found

[gavinking]

This is an issue that has been bothering me for a while (a couple of years), though the time to deal with it was not really ripe. This is an ancient error I made many years ago, and which was copied by the JPA spec. Nobody really seems to have ever drawn attention to it in all these years.

The problem is this: Session.get() and EntityManager.find() return null if there is no matching row. Therefore, almost every single call to either of these methods should be followed by an immediate null check. I very much doubt that this is actually done in a disciplined way in most programs that use Hibernate. And that makes these programs rather vulnerable to NPEs.

And an NPE really is a significantly worse thing to deal with than a NoResultException, which can in principle be handled much further up the call stack and reported in a sensible way. NPEs are bugs; a NoResultException doesn't have to be.

Therefore, I think we need to rectify this ancient mistake by adding a new method to Session. The main problem is that all the good names are already taken: we've already used get(), find(), and load(). So what would we call this?

• Well, getOrThrow() is pretty clear and explicit but a bit ugly.
• And retrieve() is available but unlovely.
• Since load() is already deprecated in H6, we could in principle come back in a couple of releases or so and undeprecate it, but change its semantics to eager rather than lazy loading ... this is actually a suprisingly clean solution, since it's just the semantics that this method should always have had, and from a very strict point of view it can be argued that it's a non-breaking change. You just get the error a bit earlier, though it will be a different kind of error, and recoverable. There's no need to change the signature of load() in any way, AFAICS. And I think it's a good name that is suggestive of the difference in semantics.

For completeness I'll mention that session.getOptional(Foo.class,id).orElseThrow() looks like it sorta does what I want, though in a verbose way. But it doesn't really solve the problem since NoSuchElementException is almost as bad as NullPointerException. Another reason to keep disliking Optional.

Of the above options I think I would lean towards waiting for a bit, and then undeprecating load().

Thoughts? Any good alternative names?

[gavinking]

Note that fortunately we don't have this problem in SelectionQuery, since getSingleResult() vs getSingleResultOrNull() already correctly models this distinction.

[yrodiere]

IMO we should go with getOptional. Because it's simple: it does what it says, on contrary to load whose semantics are tainted by a (long) history. And also because it's standard: you'll find many similar methods in other libraries.

The fact that it throws NoSuchElementException is secondary; the point of Optional is to deal with missing values explicitly. If users chose to call orElseThrow(), they will get a meaningless exception, but that's on them: they should have handled the missing value properly by calling .orElseGet(...), orElseThrow( () -> new MyException() ), .map(...), or whatever fits their use case.

Just calling orElseThrow() (without arguments) is bad practice, only fit for tests, throwaway code, or places where you really don't expect the optional to be empty (ever).

[beikov]

How about we try aligning the naming with the existing IdentifierLoadAccess#loadOptional and call this loadOptional?

[gavinking]

OK, so perhaps I need to give a better example of the sort of thing I want to write:

try {
    Book book = session.get(Book.class, id);
    book.blahBlahBlah();
    // .... lots of stuff
}
catch (EntityNotFoundException e) {
   // this catch might even be a couple of frames up the stack!
   reportToUser();
}

These are bad alternatives:

try {
    Book book = session.getOptional(Foo.class,id).orElseThrow()
    book.blahBlahBlah();
    // .... lots of stuff
}
catch (NoSuchElementException e) {
   // this catch might even be a couple of frames up the stack!
   reportToUser();
}

Verbose, and NoSuchElementException is just not precise enough here. (Indeed, it's hardly better than NPE: it doesn't even hint that an object was not found in the database.)

try {
    Book book = session.getOptional(Foo.class,id).orElseThrow(() -> new EntityNotFoundException("message", id)) 
    book.blahBlahBlah();
    // .... lots of stuff
}
catch (NoSuchElementException e) {
   // this catch might even be a couple of frames up the stack!
   reportToUser();
}

Is absurdly verbose. I would rather just write if (book == null).

The point is that I want to make correct null handling automatic, something users get with almost no work, not something they have to jump through hoops to do.

FTR, I'm not implacably opposed to adding a getOptional(), it's just that it doesn't at all solve the problem I'm worried about here, which is that users are too lazy or distracted to write code that doesn't NPE.

[beikov]

Aha, I may have misunderstood your text initially then. You problem is with returning null for Session#get where you would rather have it throw an exception. We usually do use find methods to possibly return null and get methods to throw if nothing can be found in our APIs, so Session#get returning null indeed seems like an odd one. Do I understand it right, that you'd ideally want that method to throw an exception now? IMO that would be fine for me. People have an alternative, which is Session#find, though we would have to add some find overloads:

	<T> T find(Class<T> entityType, Object id, LockOptions lockOptions);
	Object find(String entityName, Object id);
	Object find(String entityName, Object id, LockMode lockMode);
	Object find(String entityName, Object id, LockOptions lockOptions);

[yrodiere]

Is absurdly verbose.

I'd rather say a bit more verbose. But then the added verbosity has a point: being explicit about an exceptional case.

 try {
-    Book book = session.get(Book.class, id);
+    Book book = session.getOptional(Book.class, id).orElseThrow(() -> new EntityNotFoundException("message", id)); 
     book.blahBlahBlah();
     // .... lots of stuff
 }
 catch (EntityNotFoundException e) {
    // this catch might even be a couple of frames up the stack!
    reportToUser();
 }

Also note that catching EntityNotFoundException a couple frames up the stack, depending on context, can be dangerous: you don't have any guarantee that the entity which was not found was the book, it could have been another entity that we also did not find... So yes, it's a bit safer than catching NPE, but still not the panacea. With orElseThrow, at least, you can use a custom exception type.

I would rather just write if (book == null).

But your assumption is that the handling code might be several frames up the stack, so that won't work. You need an exception with that assumption... If you remove that assumption, of course book == null has an equivalent with Optional: just call book.isAbsent().

the problem I'm worried about here, which is that users are too lazy or distracted to write code that doesn't NPE.

I'd argue that just throwing another, more specific unchecked exception will help with debugging, but won't prevent the same problem from happening again, just with a different exception. Optional would, because even lazy users would have to go out of their way to handle the "absent" case.

IMO we can try to encourage more concise code, or safer code, but hardly both. At best we can limit the added verbosity resulting from the added safety, but it won't be zero.

Pattern matching might help one day, but even that won't address your "couple frames up the stack" requirement and will still add verbosity.

[gavinking]

Also note that catching EntityNotFoundException a couple frames up the stack, depending on context, can be dangerous: you don't have any guarantee that the entity which was not found was the book, it could have been another entity that we also did not find.

Actually I think it's quite safe most of the time. I don't usually call get() lots of times with lots of different ids in one transaction. I usually do it for at most one or two ids, and then navigate from there. Even if I do call it for a whole batch of ids, it's almost certain that in that case every id in the batch has the exact same semantics, and can be reported uniformly.

Note that the exception type we would use here would very clearly distinguish a "not found" coming from get() from any other sort of "not found" (e.g. one coming from deeper in Hibernate).

Sure, sure, of course you can come up with situations where what I've just said isn't true, but I'm most focussed on the very common cases.

But much more importantly, I'm worried about what happens when things aren't right, when the user doesn't write correct code: how does this error get reported in the log or stack trace?

• Does it get reported as NullPointerException or NoSuchElementException with no information about the primary key or entity?
• Or does it get reported as EntityNotFoundException with the entity name and the primary key?

And sorry, but I just don't think I can rely on the user to always write

session.getOptional(Book.class, id).orElseThrow(() -> new EntityNotFoundException("message", id));

Because I have met users. They don't do that.

[gavinking]

Do I understand it right, that you'd ideally want that method [get()] to throw an exception now?

Yes, that would be completely perfect, but also massively breaking. I don't see it as an option, sadly.

That's why I'm proposing to use load() for this: we've already promised to break it (since 6.0). And it already has the exact signature we need, and a name which is reasonable.

[gavinking]

But your assumption is that the handling code might be several frames up the stack, so that won't work.

Actually I think it's quite safe most of the time. I don't usually call get() lots of times with lots of different ids

Granted, @yrodiere, I usually prefer to write extremely tiny methods, with a very low level of nesting, and I'm a compulsive user of Extract Method, so my "two or three frames up the stack" is actually not so very far away from the source of the exception.