Spring data DomainClassConverter broken with Spring 5.3 in case of missing entity

[clemstoquart]

Hi Spring team,

I think that I've found an issue migrating a project from Spring 5.2 to 5.3.
Indeed I'm using the DomainClassConverter provided by the Spring Data team and there's a change in the way missing entities are handled.

With Spring 5.2 a missing entity was nicely handled and a 404 HTTP response was sent. To me, that's the expected behavior.

But with Spring 5.3 it returns a 500 HTTP code with a MissingPathVariableException. I think that this new behavior comes from this commit.

I can share more details if needed.
Have a nice day :)

[jhoeller]

What specifically happened in the 5.2 code path there that led to a 404? Was that handler method entered with a null value after conversion (which would lead to this change of behavior in 5.3), in which case a required=false or @Nullable declaration would help on your signature? Or was it turned into a 404-triggering exception somewhere else?

[clemstoquart]

Hi @jhoeller

Thanks for your quick answer.
The 404 is triggered by a custom ExceptionHandler that is catching the AccessDeniedException return by the @PreAuthorize on the controller :
@securityService.canUse(#my-entity)

Without this exception handler the return code is 403 because the security check is failed (because the entity is null).
Without the @PreAuthorize we had an NPE.

With Spring 5.3 we don't reach the @PreAuthorize code anymore and we just have a MissingPathVariableException.

[jhoeller]

@PreAuthorize intercepts the method invocation there, so it's indeed not reached anymore. If you declared your entity parameter as @PathVariable(required=false) or as @Nullable, we'd still invoke the method (potentially exposing it to an NPE, effectively restoring the previous behavior)... which would also help in your case, I suppose, since we'd still invoke the security interceptor?

Alternatively, you could specifically handle MissingPathVariableException and turn it into a 404 directly.

[clemstoquart]

Hi,

You're right, adding @PathVariable(required=false) or @Nullable makes it work as before. And handle the MissingPathVariableException is ok too.
What should be considered as the best practice ?

By the way, should we consider updating the DomainClassConverter documentation or just write something in the migration notes ?

[jhoeller]

Good question, we should document the best practices there (and the fact that we're strongly guarding against NPEs in handler methods now with this revised behavior in 5.3), and strongly hint towards required=false as a possible solution for regression scenarios. I'll keep this ticket open for that purpose.