`After` handler to turn `null` into `404`

[kliushnichenko]

There is a typical use-case for get/find/fetch API like below

get("/users/{userId}", ctx -> {
  var userId = ctx.path("userId").value();
  return userRepository.findUserById(userId); // can return null, and this is OK for repo
}

where null can be returned from a route handler, some time ago the encoder was simply failing on this. Right now it returns just null as text. But it is still not a legal JSON response. The desired behavior is, typically, to return a 404 response in this case.

It is a bit boilerplate to add a null check in every handler like above, so, what I usually do in this situation, is just add a dead simple after() handler that does the null-check and throws 404.

Would it be OK if I add a similar after handler to the project?

[jknack]

Signature for handler is:

@NonNull Object apply(@NonNull Context ctx) throws Exception;

So it must be non-null, but guess it depends on the encoder... Adding an after handler seems too much for me, user should never returns null as HTTP response.

[agentgt]

I agree with @jknack . It also begs the question of things like HTTP method delete. If it returns null does it 404? You probably want to check the HTTP method.

How we deal with this is a custom NotFoundException exception that gets mapped to 404.

return NotFoundException.throwIfNull(userRepository.findUserById(userId)); 

[kliushnichenko]

With delete everything is pretty much cool now. void methods automatically return desired 204 No-Content, or you can simply return StatusCode.NO_CONTENT

@agentgt Nice tip with the custom exception, I'll make a note of it, thanks!

I agree that after handler is more of a W/A here.
But I still think it is not the best option to return a null string when there is a proper Accept: application/json header and corresponding JSON encoder available.
So, maybe it is a topic for the next major release, to work out the Encoder(s) corner-case with null.

Maybe even with some configuration options, bc sometimes null needs to be treated as 404 and sometimes as 204

[jknack]

How bad is to add method like @agentgt suggest to StatusCode?

  return StatusCode.NOT_FOUND.throwIfNull(userRepository.findUserById(userId))

Also: throwIfFalse, throwIfTrue etc...

Or probably assertXXX?

[kliushnichenko]

Yeah, I guess we need something built-in for that.

However, with return StatusCode.NOT_FOUND.... I'm afraid that we're shifting the focus away from the normal/positive flow and prioritizing the corner case of NOT_FOUND. It feels a bit odd to see it comes first.

I'd prefer more classic approach, like Result.ok(user).orElse(NOT_FOUND).

For MVC, we could also introduce corresponding annotations that automatically wrap responses in a Result at build time. I'm not sure how much value this adds, it's just a thought, for example:

@OnNullResult(NOT_FOUND, "User not found")

[jknack]

However, with return StatusCode.NOT_FOUND.... I'm afraid that we're shifting the focus away from the normal/positive flow and prioritizing the corner case of NOT_FOUND. It feels a bit odd to see it comes first.

It doesn't. Look:

class StatusCode {

   <T> T throwIfNotnull(T value) {
      if (value ==null) {
          throw new StatusCodeException(this);
      }
      return value;
   }
}

So what ever: StatusCode.CONFLICT.throwIfNotNull

[kliushnichenko]

it is similar, but not the same from my point of view.

return Result.ok(user).orElse(StatusCode.NOT_FOUND)

This style reads more declaratively as you're asking for a result and specifying an alternative if the result isn't available. It's closer to how you'd express business logic, it's more readable. But it requires some overhead due to the Result object, that's the downside, agree.

return StatusCode.NOT_FOUND.throwIfNull(user)

This approach is more imperative and direct. It’s saying, “Here’s the resource; if it's null, throw an exception.” It is more focused on how things are done, which can be harder to read at a glance. The introduction of StatusCode as a null-checking utility breaks the expectation because StatusCode doesn’t naturally fit into the role of null validation. Yet simple and effective, no overhead.

For me, readability and clarity prevail in this case, but this is just my point of view

[agentgt]

return Result.ok(user).orElse(StatusCode.NOT_FOUND)

This is the Optional style.

That requires you return Result<T> which I don't think exists in Jooby. I assume your idea is analogous to Spring's ResponseEntity: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/ResponseEntity.html

As far as more readable or declarative that is point of view. A Lisp programmer prefers right to left reading of code but I admit that is a weak argument.

The real big thing is that Result.ok takes a @Nullable. That is a no no no in my book. I make null parameter input methods super clear that they are taking a nuke usually something immediately happens (the exception is Optional). When I mean immediate Result.ok would 404 exception if null.

But going back to Optional this works today:

return Optional.ofNullable(user).orElseThrow(NotFoundException::new);

EDIT using @jknack notation (that avoids a ton of classes):

return Optional.ofNullable(user).orElseThrow(StatusCode.NOT_FOUND::exception);

[kliushnichenko]

I'm not trying to make Jooby be like Spring in any chance.

You are absolutely right, this is just a syntactic sugar of the typical Optional flow, but we narrow it down to the
use-case of return entity or statusCode. Thus, no need to build a new Exception, and orElse is restricted to status codes. 
So when the developer sees this structure Result... he knows from half of a glance what this is about.
Maybe the naming needs to be improved of course.

Nowadays with all these clouds and microservices developers produce tons of code per second, and we are spending more and more time reading/discovering the code rather than writing it. So when there is a little chance to make this reading process a bit simpler/faster for a developer I usually go for it.

Do I like this Result... construction much?  No. It sucks. I don't even want to specify this NOT_FOUND anywhere at all.
But throwing from the StatusCode option has even less of my sympathy.

The whole Result implementation could be:

public class Result<T> {

  private final T value;

  private Result(T value) {
    this.value = value;
  }

  public static <T> Result<T> of(T value) {
    return new Result<>(value);
  }

  public T orElse(StatusCode code) {
    if (value == null) {
      throw new StatusCodeException(code);
    }
    return value;
  }

  public T orElse(StatusCode code, String message) {
    if (value == null) {
      throw new StatusCodeException(code, message);
    }
    return value;
  }
}

that's it, I didn't get about "ton of classes"

BTW, Jooby 1.x had Result, not like this one above, but it was there.

[agentgt]

that's it, I didn't get about "ton of classes"

The ton of classes was loaded with multiple things. If we did the Result I would want it like Spring's and have it a Builder which would require two classes and possibly three if I had my way (e.g. sealed interface of Result with an EmptyBodyResult which is the null one).

The other reason I mentioned ton of classes was to avoid an exception per status code which is what we have in our code base. The reason we have that is I try to make exceptions happen as close as possible to the error. That is I'm not even really a fan of orElseThrow(someExceptionSupplier) but for an API and ease of use I like @jknack approach.

That is this is awkward

return Optional.ofNullable(user).orElseThrow(() -> new StatusCodeException(StatusCode.NOT_FOUND));

EDIT luckily though Jooby includes NotFoundException which is really mostly what you want for this null case.

It really is not that unreadable:

// Most people use `Optional` for service calls. I don't even like `Optional` and I use it for Service calls because many return `Stream` of something.
var user = findUser(userId); 
return user.orElseThrow(NotFoundException::new);

Now going back:

The whole Result implementation could be:

Is not the whole implementation and if it is it is not worth it. The Spring implementation is vastly superior and not because it is more complex but because it treats nullable like you expect and is not a glorified Optional. It follows the builder pattern so it is clear when we are in building mode and has of as ofNullable. That is why I showed the Optional stuff because your Result class is basically that but worse.

I'm not trying to be rude but willy nilly adding API for elegance just bloats shit. Jooby already has StatusCode so with my approach of the Optional @jknack would only have to add one method.

StatusCodeException StatusCode.exception()
// and maybe
StatusCodeException StatusCode.exception(String message)

Which is a lot less than 4 or so classes to correctly implement a Result type.

[agentgt]

Also the Result type would need to handle various shapes like redirect.

Actually the above is a far bigger pain for us than dealing with 404.

You see before everyone built SPAs with RESTful backends the flow was this assuming a FORM post:

1. User enters form data and submits
2. app parse request parameters
3. app validates if failed return status code 400 with ModelAndView with validation errors and partially filled object with user submitted data
4. if no validation problems CRUD database
5. return 302 or some redirect to result page.

Like 90% of the web is still this way and that last step is painful in Jooby (well the validation was painful but there have been updates on that front). The last step is to avoid double submission.

So if we did this Result thing I would expect integration or reuse with ModelAndView and I would want to handle the redirect case. I would probably want to have the SneakyThrow.Function for mapping which makes me wonder how much of there is an overlap with the Value API.

BTW @jknack if you are curious how I handle 302 I have a custom TemplateEngine:

	@Override
	public @Nullable DataBuffer render(
			Context ctx,
			ModelAndView modelAndView)
			throws Exception {
		if (!supports(modelAndView)) {
			throw new IllegalStateException();
		}
		RedirectView rv;
		if (modelAndView instanceof RedirectView _rv) {
			rv = _rv;
		}

RedirectView is this:

public interface RedirectView {

	public URI getRedirectUri();

	default Set<RedirectFlag> getRedirectFlags() {
		return EnumSet.noneOf(RedirectFlag.class);
	}

	default RedirectType getRedirectType() {
		return RedirectType.FOUND;
	}

	public enum RedirectFlag {
		CONTEXT_RELATIVE,
		DISALLOW_RELATIVE;

		public boolean isSet(
				Set<RedirectFlag> flags) {
			return flags.contains(this);
		}
	}

	public enum RedirectType {
		MOVED_PERMANENTLY,
		FOUND,
		SEE_OTHER,
		TEMPORARY_REDIRECT
	}

}