kroki icon indicating copy to clipboard operation
kroki copied to clipboard

added head http method

Open amirabramovich opened this issue 3 years ago • 4 comments

resolved issue #677

amirabramovich avatar Oct 09 '21 14:10 amirabramovich

Hey, thanks for your contribution!

Before jumping into the implementation, I think we should thoroughly describe how we are going to implement this feature on the GitHub issue. They are a few open questions and since it will be part of the public API we have to be considerate.

For instance, the HEAD request should not have a body. It should return the Content-Type and the Content-Length (of the generated diagram) but it should not actually send the diagram.

In this regard, the current implementation is incorrect because it uses diagramHandler.createGet(name) which will return the same result whether we use GET or HEAD.

Anyway, if you want to implement this feature, please use the GitHub issue to describe how you will implement it so we can discuss it.

ggrossetie avatar Oct 13 '21 06:10 ggrossetie

we might need most of 'GET' service implementation for all the validations needed (which requires us to construct the diagram), but only not to return it in the response. I checked using JUnit (you can see the assertion) and postman and saw that POST HEAD won't return the content OOB so I think it is ok to use the whole 'GET' service as it is.

amirabramovich avatar Oct 13 '21 20:10 amirabramovich

we might need most of 'GET' service implementation for all the validations needed (which requires us to construct the diagram), but only not to return it in the response. I checked using JUnit (you can see the assertion) and postman and saw that POST HEAD won't return the content OOB so I think it is ok to use the whole 'GET' service as it is.

Indeed, Vert.x (or Netty) seems to not send a body on HEAD requests even if we call the end method with a non-empty buffer:

response
  .putHeader(HttpHeaders.CONTENT_TYPE, contentType)
  .end(buffer);

However, the Content-Length is missing from the response:

$ http HEAD :8000/graphviz/svg/eNpLyUwvSizIUHBXqPZIzcnJ17ULzy_KSanlAgB1EAjQ
HTTP/1.1 200 OK
cache-control: public, max-age=432000
content-type: image/svg+xml
date: 1635407085238
etag: 2.40.1X-P4LW6vNuh1dPqzkVpE3G00
expires: 1635839085238
last-modified: 1567581178724
vary: origin

I've opened a feature request https://github.com/eclipse-vertx/vert.x/issues/4150 but in the meantime we should define this header on HEAD requests (using buffer.length()).

ggrossetie avatar Oct 28 '21 07:10 ggrossetie

I've opened a feature request https://github.com/eclipse-vertx/vert.x/issues/4150 but in the meantime we should define this header on HEAD requests (using buffer.length()).

Since my request has been rejected on Vert.x, could you please set the Content-Length otherwise the caller won't be able to check the file size.

Since we are working on HEAD requests, we might want to return a "200" when calling a valid output format for a given diagram library:

$ http HEAD :8000/graphviz/svg
HTTP/1.1 200 OK
$ http HEAD :8000/graphviz/pdf
HTTP/1.1 404 Not Found

Arguably, GET /graphviz/svg could return a 204 No Content or a 400 Bad Request instead of a 404.

What do you think?

ggrossetie avatar Aug 24 '22 21:08 ggrossetie