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

StatusCode support for ending RoutingContext #2146

Open
thced opened this issue Mar 21, 2022 · 9 comments · May be fixed by #2147
Open

StatusCode support for ending RoutingContext #2146

thced opened this issue Mar 21, 2022 · 9 comments · May be fixed by #2147

Comments

@thced
Copy link
Contributor

thced commented Mar 21, 2022

To end a RoutingContext response, one has to specify:

ctx.response().setStatusCode(204).end();
ctx.response().setStatusCode(HttpResponseStatus.ACCEPTED.code()).end();

An alternative, more pleasant-to-the-eye solution, could be to let RoutingContext accept ending with a Integer or a (netty) HttpResponseStatus, similar to this:

ctx.endWithStatus(Integer statusCode);
ctx.endWithStatus(HttpResponseStatus status);

Reasonable?

@pmlopes
Copy link
Member

pmlopes commented Mar 21, 2022

Why not?

  /**
   * Shortcut to the response end with a given status code.
   * @return future
   */
  default Future<Void> end(int statusCode) {
    return response().setStatusCode(statusCode).end();
  }

  /**
   * Shortcut to the response end with a given status code.
   * See {@link #end(int)}
   */
  @Fluent
  default RoutingContext end(int statusCode, Handler<AsyncResult<Void>> handler) {
    end(statusCode).onComplete(handler);
    return this;
  }

Which would mean:

ctx.end(204);

@thced
Copy link
Contributor Author

thced commented Mar 21, 2022

We would not want a coupling to netty to support e.g. ACCEPTED that would internally call code() ?

@thced thced linked a pull request Mar 21, 2022 that will close this issue
@thced
Copy link
Contributor Author

thced commented Mar 21, 2022

@pmlopes

@pmlopes
Copy link
Member

pmlopes commented Mar 21, 2022

@thced although I proposed it, now that I'm looking at the idea after giving it a bit of thought, I feel this isn't really right. I see 2 ways of achieving the same result, and I'd prefer not to.

I think a better solution would be to stick to the builder API style for consistency:

ctx.statusCode(204).end();

// or

ctx.statusCode(HttpResponseStatus.ACCEPTED).end();

Which means we could create a few shortcuts in the RoutingContext like this:

statusCode(int|Enum); // delegates to response.setStatusCode(int)

Note that we should not use netty specific enums, as the goal of vert.x core is to abstract the underlying transport and protect agains API changes. So, it needs a new enum wrapper, as we already have for other things like http headers.

@pmlopes
Copy link
Member

pmlopes commented Mar 21, 2022

Also like @vietj mentioned on discord, status code must be set before the header are sent, and end() can be to late. For example, in streaming mode, one can do multiple writes and a end():

res.
  .write("hello")
  .write("vert.x")
  .end()

And this point, a user calling the end with a status code argument, will fail because the response has already been written.

@thced
Copy link
Contributor Author

thced commented Mar 21, 2022

Good input, thanks! I can perhaps adjust this PR a bit to solve it using that proposed direction instead?

@vietj
Copy link
Contributor

vietj commented Mar 22, 2022

in vertx core HttpClientRequest we added a send() method that takes care of sending something like a buffer or a stream.

  default Future<HttpClientResponse> send() {
    end();
    return response();
  }

  default Future<HttpClientResponse> send(Buffer body) {
    end(body);
    return response();
  }

  default Future<HttpClientResponse> send(ReadStream<Buffer> body) {
    MultiMap headers = headers();
    if (headers == null || !headers.contains(HttpHeaders.CONTENT_LENGTH)) {
      setChunked(true);
    }
    body.pipeTo(this);
    return response();
  }

@vietj
Copy link
Contributor

vietj commented Mar 22, 2022

it is also on HttpServerResponse

@thced
Copy link
Contributor Author

thced commented Mar 23, 2022

So, basically the issue is not whether we can check if header has been written or not, the issue you see is the inconsistency in the api this might bring?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants