Vertx-web: Route#respond method should adapt the response content type to the object class

[vietj]

The implementation of Route#respond method will always encode the future result as a JSON payload. When a Buffer is returned then it's base 64 encoded.

Instead we should adapt the type to the object type:

1. buffer : use the buffer as body response and use content type application/octet-stream ?
2. string : encode the string and assume it is HTML ?
3. any other should be encoded with JSON#encode and send as JSON payload
4. null ?

NOTE: we might see how this works with templating.

Perhaps we need a default mapping and have ability to override it in conf ?

In addition we can add new method that force the content type, e.g respondWithJson(...)

[pmlopes]

@vietj @slinkydeveloper

I propose the following rules:

if response not null
  if status-code not set then 200
  if content-type not then
    when
      buffer -> content-type = application/octet-stream
      string -> content-type = text/html
      other -> content-type = application/json
  fi
else
  if status-code not set then 204 (no content)
fi

Plus we can have the defaults overridden with:

-Dvertx.web.respond.buffer=application/octet-stream
-Dvertx.web.respond.string=text/plain
-Dvertx.web.respond.other=application/json

WDYT?

[pmlopes]

@vietj here's a less magic alternative:

https://github.com/vert-x3/vertx-web/compare/poc/1798

[slinkydeveloper]

@pmlopes I think we can take both approaches: the rules you proposed are good as defaults, then the user can specify the ResponseType for more control.

I disagree with allowing overriding the defaults, if the user don't like the defaults i prefer he/she ends up writing its own ResponseType thingy

[vietj]

we should try by using BodyCodec instead of ResponseTime, because this is
more or less the same.

On Thu, Nov 26, 2020 at 8:31 PM Paulo Lopes [e-mail]
wrote:

@vietj https://github.com/vietj here's a less magic alternative:

https://github.com/vert-x3/vertx-web/compare/poc/1798

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/vert-x3/vertx-web/issues/1798#issuecomment-734453466,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABXDCSHO7X7O6E7QW3NTCDSR2UJZANCNFSM4UD4KAPQ
.

[pmlopes]

I'm looking at BodyCodec and it's not that trivial, we need 2 methods:

String contentType();
<T> Buffer encode(T result);

Adding the content type is trivial, we add the method and add it the the implementation constructor but there is a problem. HTTP content type header serves 2 purposes:

• content type
• charset

Currently the body codec only deals with charset for String bodies.

Another issue is that the current codecs:

• String
• Buffer
• None
• JSON Array
• JSON Object

Are not content type friendly, for example String is it text/plain or text/html? What is none null handling maybe?

If we want this to be tested with 4.0.0 then I'd say lets add the current respond allowing null with 204 always assuming the mapping:

buffer -> content-type = application/octet-stream
string -> content-type = text/html
other -> content-type = application/json

And for 4.1 we can rework the BodyCodec to be a real codec and handle the encoding.

WDYT @slinkydeveloper @vietj ?

[vietj]

do you mean that

• for 4.0 we fix the current respond method so that it adapts the encoding
  according to the type of the object.

• for 4.1 we improve the BodyCodec so we have a respond method with a
  BodyCodec argument

?

On Fri, Nov 27, 2020 at 5:02 PM Paulo Lopes [e-mail]
wrote:

I'm looking at BodyCodec and it's not that trivial, we need 2 methods:

String contentType();
Buffer encode(T result);

Adding the content type is trivial, we add the method and add it the the
implementation constructor but there is a problem. HTTP content type header
serves 2 purposes:

• content type
• charset

Currently the body codec only deals with charset for String bodies.

Another issue is that the current codecs:

• String
• Buffer
• None
• JSON Array
• JSON Object

Are not content type friendly, for example String is it text/plain or
text/html? What is none null handling maybe?

If we want this to be tested with 4.0.0 then I'd say lets add the current
respond allowing null with 204 always assuming the mapping:

buffer -> content-type = application/octet-stream
string -> content-type = text/html
other -> content-type = application/json

And for 4.1 we can rework the BodyCodec to be a real codec and handle the
encoding.

WDYT @slinkydeveloper https://github.com/slinkydeveloper @vietj
https://github.com/vietj ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/vert-x3/vertx-web/issues/1798#issuecomment-734898054,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AABXDCXYSKBSTR23W72IRFTSR7EP7ANCNFSM4UD4KAPQ
.

[pmlopes]

@vietj yes.

[slinkydeveloper]

String is it text/plain

String is kinda an edge case... It might be text/plain, text/json, application/json, ... For sure, we shouldn't allow none as content type.

I like the plan of reworking BodyCodec later.

[pmlopes]

I've updated the javadoc to follow this discussion. In 4.1 we will rework the body codec.

[pmlopes]

See https://github.com/vert-x3/vertx-web/commit/d0876a9e340fe91d55ad2a258e9ffbea7b136e1f