Helidon Nima support
rbygrave opened this issue · 13 comments
So what is remaining before we can add support?
There is a decision I've be pondering around using code gen for performance of JSON marshall/unmarshall vs using the built in media negotiation that nima has.
If we use the built in content negotiation then we generate code like:
Person person = controller.doStuff(...);
response.send(person); // nima internally works out that it will send this as json content (some overhead we can effectively skip)In the "aggressively optimising json marshall/unmarshall" approach we can instead generate code like below:
Person person = controller.doStuff(...);
// explicitly set header and write directly to response outputStream
response.headers().add(Http.HeaderValues.CONTENT_TYPE_JSON);
personJsonType.toJson(person, response.outputStream());This approach skips some overhead with the mindset that we know this response is json and we know it's type. This will be about as good as we can get in terms of minimising overhead.
The personJsonType is obtained in the route handler constructor via Jsonb instance which is passed via dependency injection.
final Jsonb jsonb;
final JsonType<Person> personJsonType;
// Additionally provide Jsonb via dependency injection
PersonController$Route(Jsonb jsonb, SomeDependency someDependency) {
this.jsonb = jsonb;
this.personJsonType = jsonb.type(Person.class); // we know the types that this route will read/write json for
}
...Noting: This Jsonb thing above is avaje-jsonb which is the second fastest json library behind dsl-json (but faster than Jackson and Gson and does this via source code generation - its a library I wrote).
Ideally we support code generation for both mechanisms so that people can opt for either "As fast as we can go style" or built in nima content negotiation style.
So I was pondering how we best support both. Currently the code in the nima branch does code generation using nima content negotiation. I have got busy with other things like getting Loom support into Postgres JDBC Driver and a few other things which sucked me away from sorting this out. Hmmm.
We could probably merge in the nima PR and then release a release candidate version while I ponder options for doing the optimised code generation approach for json content.
Perhaps we could check for the presence of the JsonB dependency and have that as the deciding factor as to which generation style to use?
check for the presence of the JsonB dependency
Yes. I was wondering if that was too subtle but perhaps it really isn't hmmm.
Some more explicit options could be:
- Have 2 nima generators / 2 maven artifacts
- Use a manifest file (I don't like this in that it might be a bit too hard for folks to use)
- Have some explicit annotation or option on
@Controller(I don't like this idea, its a global setting really)
BUT ...
Right now, it feels like I have been over thinking it. Maybe just go with "presence of the JsonB dependency" and see if that is problematic in practice - it might in fact be perfectly fine, especially if we document it well enough. Yeah, I think we can just roll with that.
I have started on the jsonb implementation, but it seems that
// explicitly set header and write directly to response outputStream
res.headers().contentType(io.helidon.common.http.HttpMediaType.APPLICATION_JSON);
testPostMethodReturnJsonType.toJson(result, res.outputStream());
Doesn't work, no response is sent. when I use toJsonBytes and send as byte array it works though
res.headers().contentType(io.helidon.common.http.HttpMediaType.APPLICATION_JSON);
res.send(testPostMethodReturnJsonType.toJsonBytes(result));
my changes:
https://github.com/avaje/avaje-http/compare/feature/nima...SentryMan:avaje-http:nima-jsonb?expand=1
tested on this repo: https://github.com/SentryMan/avaje-helidon-nima-api-example.
hmm, it seems the error is mine. I didn't know that jsonb had released RC2. It works.
I think I got the JsonB support in a good state, please take a look when you get time.
What else remains?
JsonB support in a good state
Yes, excellent work !!
What else remains?
- JsonB does support primitives so looking to add a test for that and adjust generated code as necessary
- Looking to change the JsonType code generation to use the "short type name" /
UType.shortType()(which might be a bit unnecessary but I think it will make the generated code better to read and would also follow the similar code generated for JsonB adapters
So instead of:
private final JsonType<org.example.Person> personReturnedJsonType;
private final JsonType<org.example.Person> postPersonBodyJsonType;
private final JsonType<org.example.Person> postPersonReturnedJsonType;
private final JsonType<java.util.List<org.example.Person>> personListReturnedJsonType;
private final JsonType<java.util.Set<org.example.Person>> personSetReturnedJsonType;
private final JsonType<java.util.Map<String, org.example.Person>> personMapReturnedJsonType;
private final JsonType<org.example.Person> addBodyJsonType;
private final JsonType<java.lang.String> addReturnedJsonType;
private final JsonType<java.util.List<org.example.Person>> addMultipleBodyJsonType;
private final JsonType<java.lang.String> addMultipleReturnedJsonType;
private final JsonType<java.lang.String> formReturnedJsonType;
private final JsonType<java.lang.String> formBeanReturnedJsonType;We'd instead generate and use:
private final JsonType<org.example.Person> personJsonType;
private final JsonType<java.util.List<org.example.Person>> listPersonJsonType;
private final JsonType<java.util.Set<org.example.Person>> setPersonJsonType;
private final JsonType<java.util.Map<String, org.example.Person>> mapStringPersonJsonType;
private final JsonType<java.lang.String> stringJsonType;That should be it I believe. Well done!!!
JsonB does support primitives so looking to add a test for that and adjust generated code as necessary
I'm not sure I'm following your exact meaning, but I have added support for primitive return types in the latest pr.
Great work @SentryMan - I have released this as verison 1.18, it's on it's way to maven central now !!