Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option so that CrudRepository.findOne throws an unchecked exception if entity is not found [DATAJPA-118] #544

Closed
spring-projects-issues opened this issue Oct 26, 2011 · 28 comments
Assignees
Labels
has: votes-jira status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@spring-projects-issues
Copy link

Adrian opened DATAJPA-118 and commented

For a clean design it would be great if I could set an option (maybe on the jpa:repository tag) that will throw a javax.persistence.EntityNotFoundException (or some other unchecked exception) instead of returning null if CrudRepository.findOne does not find the entity.


Issue Links:

  • DATAJPA-301 Add CrudRepository#findExpected(ID)
    ("is duplicated by")

14 votes, 25 watchers

@spring-projects-issues
Copy link
Author

Vincent Demeester commented

I think the "option" (via jpa:repository tag) should be more generic : It should enable/disable the throwing of Persistence Exception (like NotFoundException, …)

@spring-projects-issues
Copy link
Author

Marcel Stör commented

I first meant to whole-heartedly support this feature request but I'm not so sure anymore...
In general, Clean Code rules (at least those proclaimed by Uncle Bob) mandate that you a) never pass null and b) never return null. I try to adhere to that in my code. However, with repositories I think the case is slightly different. With methods like findOne(id) I'd expect an exception since the argument is an ID that I must have gotten from somewhere i.e. I'm not searching in the wild. With everything else though I suppose it's fine to return null as the caller can't expect to find a record, null is always an option.
That being said I don't think you can/should apply the same standards to all repository methods. And since I like expressive method names I'd much rather see ??an extra method?? like findOneWithException(id) being added

@spring-projects-issues
Copy link
Author

Michael Andrews commented

I originally voted up (after all, I ended up here looking to see if I was doing something wrong, and hoping there was an option to throw such an exception automatically). But after some thought - It should be left up to the exported Service Layer or controller layer to throw the exception. (IMO).

@spring-projects-issues
Copy link
Author

Sergey Parhomenko commented

Agreed that extra findOneWithException(id) method would be better (although the name is not ideal). I think the logic of throwing this exception belongs more in the repository layer.
Was quite surprising not to find such method... in context of a JAX-RS application, where you need to return 404 if an entity requested in GET or PUT requests was not found, such method would be quite useful.
CrudRepository.delete(id), btw, already throws EmptyResultDataAccessException if the entity is missing, which is exactly as we need it

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

Some thoughts here:

  1. We're never going to throw a persistence mechanism specific exception from repositories. A core responsibility of the repository layer is to abstract the persistence technology used. This if at all (and I currently have doubts we'll ever implement this) you're going to see an EmptyResultDataAccessException.

  2. I'd like to see a reasonable use case to prefer

try {
  Person person = repository.findOne(id);

  // more code in case of success
catch (EmptyResultDataAccessException e) {
  // error handling
}

over

Person person = repository.findOne(id);

if (person == null) {
  // error handling
}

// more code in case of success

If you really want to throw an exception in this case (e.g. to automatically indicate a 404 in a web framework) you'll need to catch and wrap the original exception anyway. So I currently fail to see how the exception way of resolving the issue creates any benefit except a second exception instance to be created.

From a pure design point of view I'd argue the case of not finding an entity for a given id is not really an exception but a standard case. So I feel like abusing the concept of an exception to implement control flow to some degree here.

However, I'm definitely in favor to rethink the issue in case we find a few use cases that explain benefits of the exception approach. Still, please keep in mind that changing the behavior of the existing method is close to impossible as we cannot break existing clients. The same would apply for an addition of a new method as we'd have to implement the method by all implementations across supported stores

@spring-projects-issues
Copy link
Author

Richard Eckart de Castilho commented

null values tend do propagate through an application, making it harder later to trace down the origin on the null. Exceptions fail fast and can be handled further up the call-chain - possibly even display a reasonable message in the UI.

However, if the "read" method throws an exception when an entity is not available, there should be some other "exists" method to check for the presence. Otherwise, odd code like this might pop up:

// Create person if it does not exist
try {
  Person person = repository.findOne(id);
  // ok, person is already in DB, nothing more to do here
catch (EmptyResultDataAccessException e) {
  // create person in the database
}

10 cents from Twitter

@spring-projects-issues
Copy link
Author

Vivek Kondur commented

I agree with Oliver & Richard.

null values could propagate and would be harder to debug. An exception is preferred, which lets the Application developer to handle the scenario in a desired manner

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

Richard Eckart de Castilho: This feels a bit like trading one way working around the lack of a proper Optional type for the other. I've always considered the decision to work around such a scenario by asking: is the null value a reasonable outcome of the method. If it is, return null (yeah I know). If it's not and null is really indicating an exceptional state, use an exception. As this is highly varies in this case we went for the null route.

Your code snippet could be written as follows:

if (!repository.exists(id)) {
  return repository.save(new Person(…));
} else {
  return repository.findOne(id)
}

That causes an additional query in every case. As the you want to return the existing object in case it can be found anyway we usually see code like this, if you really want to create a default instance in case the lookup fails (although I'd argue it's more likely you want to implement dedicated error behavior as indicated in my previous post, e.g. assemble a dedicated more semantic exception or the like):

Person person = repository.findOne(id);
return person == null ? repository.save(new Person(…)) : person;

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

Vivek Kondur: Agreeing with me and voting for the exception doesn't work together :). The JavaDoc clearly states that the method might return null. So code not checking the return value for null has a bug. See my previous post to read about why I think introducing the exception is actually just another hacky workaround for a missing Optional type

@spring-projects-issues
Copy link
Author

Adam Thody commented

A separate method that throws an exception would be pretty ugly API design. The API shouldn't be leaking its implementation details.

I agree with Oliver, there are many valid reasons why a query for a particular id would return an empty result – an entity has been deleted, a URL has been mistyped, etc. From my perspective, this makes throwing an exception unsuitable, and a control flow hack.

Now, returning null isn't desirable either, but I wouldn't arbitrarily trade one faux pas for another.

A Null Object or Optional object approach is the way to go.

@spring-projects-issues
Copy link
Author

Vivek Kondur commented

@Oliver: In the current project, handling a similar case the way you have described in your earlier post. The code snippet is from Service Layer.

  final User user = userRepository.findOne(id);
        if(user !=null)
            return user;

        throw new EntityNotFoundException("User with id '"+ id +"' not found.");

However, I would have preferred an exception (EmptyResultDataAccessException) thrown from the findOne(id) method

@spring-projects-issues
Copy link
Author

Stevo Slavić commented

+1 for returning null, and using exceptions for exceptional situations only. Searching and not finding in this case IMO is not an exceptional situation, like it is e.g. persisting entity when (FK) referenced entity is not found in repository.

When Java 8 comes, replace returning null with java.util.Optional

@spring-projects-issues
Copy link
Author

Brian Clozel commented

+1 for null and Optional when it is available.
Using exceptions in that case would be using exceptions for flow control, right? Isn't that a code smell?

@spring-projects-issues
Copy link
Author

Romain Manni-Bucau commented

Null or exception are equivalent. JPA uses exception, sprin data chose null which is fine in more cases

@spring-projects-issues
Copy link
Author

Ricardo Gladwell commented

I would recommend making the default behavior to throw an exception. Returning null by default its a problem because developers may forget to handle the null, and it may propagate elsewhere. An exception will fail fast

@spring-projects-issues
Copy link
Author

Ranie Jade Ramiso commented

+1 for returning null. I have to agree with @Stevo, searching and not finding anything is not an exceptional situation.

@Ricardo Making the default behavior to throw an exception, would be problematic to upgrading users

@spring-projects-issues
Copy link
Author

Ricardo Gladwell commented

@Ranie not sure I agree, sometimes searching and not finding anything is an exceptional situation and sometimes not. It depends on the context, which is why I believe we should favour a fail-fast approach

@spring-projects-issues
Copy link
Author

Ranie Jade Ramiso commented

@Ricardo It really boils down to be a matter of opinion. In my case, CrudRepository.findOne returning null (not finding anything) is not an exceptional situation

@spring-projects-issues
Copy link
Author

Thomas Darimont commented

Hello,

if one really need this functionality now, one could (IMHO) implement this easily with an Aspect that advises / wraps
calls to CrudRepository+.findOne(...) with a null check in conjunction with the desired exception-throwing behaviour.

Best regards,
Thomas

@spring-projects-issues
Copy link
Author

Bogdan Calmac commented

A compelling use case for throwing an exception is a @RestController where you can have a separate exception handler that deals with the HTTP status. Here is an example (from Spring in Action):

  @RequestMapping(value="/{id}", method=RequestMethod.GET)
  public Spittle spittleById(@PathVariable Long id) {
    return spittleRepository.findOne(id);
  }

  @ExceptionHandler(SpittleNotFoundException.class)
  @ResponseStatus(HttpStatus.NOT_FOUND)
  public @ResponseBody Error spittleNotFound(SpittleNotFoundException e) {
    return new Error(4, "Spittle [" + e.getSpittleId() + "] not found");
  }

@spring-projects-issues
Copy link
Author

Bogdan Calmac commented

For my use case, I've solved the problem by extending CrudRepository and adding a default method findOneX(). Now it's up to the caller to use the return null or throw exception behavior. For a @RestController, the exception version is more convenient.

@NoRepositoryBean
public interface CrudRepositoryX<T, ID extends Serializable> extends CrudRepository<T, ID> {
    default T findOneX(ID id) throws ObjectRetrievalFailureException {
        T result = findOne(id);
        if (result == null) throw new ObjectRetrievalFailureException("Entity " + id + " not found", null);
        return result;
    }
}

@spring-projects-issues
Copy link
Author

Armando Garcia commented

+1 for throwing an exception.

My two cents: This method doesn't search by some random property, it searches by an specific Primary Key, imo there should not be a "valid" scenario where a user searches for an unexistant ID. If you actually have an ID you want to search for is because the object was already created previously, and not finding it should be treated as an exception, not part of the regular flow. That's why we have the 404 error anyway, if you are making up the route, then you should throw an error, not just an empty response, if you are following a url that previously existed, then that url is considered a broken url, and you should not be given broken urls to the user, you must consider it an exception in the backend because the user shouldn't have access to the broken link in the first place imo, the root cause would be in the client sending the user to it

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

I'd argue the exact opposite: the fact, that there's a 404 status code which is not special at all, it indicates that the absence of a resource is not an exceptional case but one explicitly integrated into the protocol. Just imagine a client trying to access the resource after it has been deleted by another client. That URI is not "invalid", it just points to a resource, that doesn't exist (anymore). Also, I think focussing on the HTTP case is shortsighted as it's just one scenario. Other clients using the repository might have to resort to catch an exception to handle the missing entity case which is suboptimal, too, as creating exceptions is expensive and it ultimately leads the user to misuse them for flow control.

Generally speaking, I think both solutions (returning null and throwing exceptions) are a somewhat wrong approach to this as they both work around the problem in different suboptimal ways. We're going to solve this by switching to the use of Optional in 2.0 as it's the appropriate means to express the potential absence of a value in Java

@spring-projects-issues
Copy link
Author

Armando Garcia commented

Hi Oliver, thanks for taking the time to review my comment. Actually I do see something very special about the 404 code, the fact that its in the 4XX range means its a Client Error. If you were to search for a resource , say a customer, by its name, you'd return an empty list or a null object, but a 2xx code, the search was successfully ran, however the query returned an empty result. But if you look for it by its ID, you return a 404, which means its an invalid request. I might have been valid once when the resource existed, but not anymore, hence the uri does become invalid, just as your access card to your company becomes invalid once you're no longer working for them, the fact that it existed at some point doesn't mean its going to be valid forever. As for the "Other clients using the repository might have to resort to catch an exception" part, I wonder what's the purpose of the exists method if not this precisely, not having to deal with the exception. Also exceptions mean exceptional cases, that's what the catch is for, the fact that they exist doesn't mean you can't do anything for them, it means, its an exceptional case, and you might or might not be able to do something about it, if you can, you catch it and do something about it, If you can't, you let it bubble up. Its not about HTTP specifically, but its an example of how this is usually treated, another example its JPA which does throw an exception

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

Two things: no, a 404 is not something exceptional. It just another status code. A URI does not become "invalid" just because the resource pointed to is absent and the server returns 404. That's how HTTP works from the ground up. There's nothing special at all about that. It might be from your application point of view. That doesn't make it special on the protocol level.

As the name suggests, the purpose of the exists method is to check whether something exists or not. That check can be used for arbitrary things ("If exists, do this, else that."). It's in place to avoid the object to be materialized in the first place because it might not even be needed. I don't quite get what that has to do with whether findOne(…) returns null or throws an exception.

I stated that I consider returning null and throwing exceptions to be equally wrong. However, we decided for null in 2008 and we now stay with it until we can move to a better solution (2.0, using Optional<T>). However, there's no value in adding another wrong to the existing one

@spring-projects-issues
Copy link
Author

Anders Norgaard commented

+1 from me for the option to have not found exception.

Maybe I am biased but mapping to a REST-ish interface (eg. via Spring RestController) is just so common that I think the option would be used quite a lot

@spring-projects-issues
Copy link
Author

Jens Schauder commented

With the changed API in Spring Data 2.x this issue seems solved, and either way Olli made his opinion pretty clear I think

@spring-projects-issues spring-projects-issues added status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement labels Dec 30, 2020
@Azahe
Copy link

Azahe commented Mar 3, 2021

(can I comment on closed issues?)
A word to people missing exception based api (like me):

Optional or null based results force repository user to check something.

To give an example:

Entity findByName(name); 

Will force user of repository to repeat result == null check (and then doing something).

Optional<Entity> findByName(name); 

Will force user of repository to repeat result.isEmpty() (or do something clever like result.map(...) or result.stream() - still multiple times in the code, check or flow branching will be duplicated).

Now, advantage of exception based apis is possibility to create exception handlers working in an aspect-like manner (and just not care about empty results).
What I mean by this is we can have something in code:

handleDbException(BaseDbException ex) {
  // logic like reporting it to error tracker, maybe returning specific error code, maybe mapping to some other exception
}

Now by the above I don't necessarily mean controller-level (infrastructure) exception handler - you might want something like this in your domain (making a decision to couple with aspect library or being very careful with using some base class). While having an option to have unchecked exception as a result of repository call would make things simpler (especially with some convention like getEntity throwing on missing and findEntity not - personally I always thought of finding as not implying success) - you might achieve it with after returning aspect that does mapping of null or empty optional (I would recommend null as optional needs to be unwrapped and it kinda breaks the point) to your desired exception

Please note that using this approach we can free domain logic from empty-result handling concern. Imho it can tremendously improve readability retaining design security that Optional introduces - exception will propagate to somewhere. Such approach is extremely useful when failing is just ok and there is no backup strategy like repeating or changing flow of the code (which, to me, seems to major case in code).
It will not work where you want to do something specific with missing result (like try another source).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has: votes-jira status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants