-
Notifications
You must be signed in to change notification settings - Fork 2
Retiring Model Binding
During my initial implementation of Javalin MVC, I made the choice to use a ModelBinder class. With appropriate caching and optimization, there's little runtime overhead for using this approach. Furthermore, most applications will mostly utilize JSON models which are going to require significantly more runtime reflection to parse than any overhead the model binder will incur.
Some negatives of using a model binder is it always works in terms of objects. If you are binding a primitive, such as a an int, there's an unavoidable boxing and unboxing operation which puts the integer on the heap (Integer) and then immediately converts it back to an int. The overhead is even more exaggerated when binding object fields and setters. Firstly, setting the field or calling the setter must be done reflectively, similar to how JSON parsing works. Secondly, the same boxing/unboxing occurs for each property. This could be avoided partially but it's probably not worth it.
Since Javalin MVC is a compile-time annotation processor tool, it makes more sense to try to generate code at compile time. For example, if the route is expecting an int like so:
public ActionResult getGreeting(String name, int age) {
return new ContentResult("Hello, " + name + "! You are " + age + " years old!");
}At compile time I can generate a handful of helper methods:
private static int getInteger(String value) {
try {
return Integer.parse(value);
} catch (NumberFormatException ex) {
return 0;
}
}
private static String getHeader(HttpRequest request, String key) {
return request.getHeader(key);
}
// ... and other value source getters
private static String getValue(HttpRequest request, String key) {
if (request.hasHeader(key)) {
return request.getHeader(key);
} /** Other value source getters **/
else {
return null;
}
}
// ... and then call the controller method
ActionResult result = controller.getGreeting(getValue(request, "name"), getInteger(getValue(request, "age")));This is effectively the same code you'd write by hand. Take special note of the getValue method. If the source of the data isn't specified with a @From* annotation, it will look through each data source one after the other until a match is found. This logic is already present in the current implementation, although it's highly recommended for the absolutely best performance to always specify the source. This is especially important when the value provides security information, like an authentication header - you probably wouldn't want to allow such a value to be overridden with a query string, for example.
One piece of functionality you'd lose using generated code is the ability to set private backing fields. Technically private fields and setters could be set reflectively by generating code to set them reflectively, but this feels like it's a step back. To me, I think dumping this functionality is justifiable.
I am really excited to pursue this idea. It has the potential to make it extremely simple to generate blisteringly fast code.
I've already made really good progress in just a couple days. At this point all primitives, boxed types, Java 8 date types, BigInteger, BigDecimal, and UUIDs are using compile time code generation to avoid the model binder. I started making similar progress on WebSockets but realized I need significantly better unit test coverage there. I almost want to wait on WebSockets and instead focus on binding complex types. I even think there might be a way to support nested complex types without too much work. I feel like I should be done soon.
I also had the idea that I should allow registering arbitrary conversion methods. Each method could be static or a member and would be decorated with an @Converter("converter-name") annotation. The processor would check that the method accepted an HttpContext and the parameter name. Then the action method parameter or model field/setter would be decorated using @UseConverter("my-converter"). This will allow compete control over the conversion process. For non-static conversion methods, the surrounding class will be instantiated using the injector if necessary.
I am still making wonderful progress. One thing I was really missing was unit testing for WebSockets, so I took a day just to familiarize myself with how to establish a WebSockets connection from Java. Previously, all my testing was limited to calling my endpoints from the browser. I ended up make a pretty simple test harness for executing WebSocket requests and inspecting events as they happened. As part of this, I made substantial improvements to the asynchronous code I wrote to test my HTTP functionality. Overall, I think it gives a much better example of how you would write unit/integration tests against an API written in Javalin [MVC].
Model binding is a complicated subject once you start talking about binding objects and not just simple primitive types and immutable reference types. While it's fairly easy to inspect an object's fields and methods, it important to remember that real-world models are strange. There's things like inheritance to consider, where you need to recursively descend into an object hierarchy looking for fields and methods. There's checking for things like abstract classes and methods, static methods, checking that setters only accept a single argument. Most complicated of all, some models might be self-referential, like an Employee having a field for manager, which is also an Employee -- you don't want to accidentally hit infinite recursion or generate a StackOverflowException while trying to bind a type and all its referenced types that way.
I have a list of unit tests I want to write to shake out any remaining bugs or odd edge cases related to model binding. It occurred to me that if I do the @Converter/@UseConverter annotations later, I will just need to circle back and write additional tests. For example, without a converter, trying to bind to an abstract class or interface should be a compile-time error. However, if you provide a converter, your code might be smart enough to determine the type dynamically and return the correct implementation/sub-class.
I introduced a toJson method as a fallback whenever not converting a primitive or bound model. This was the last thing keeping model binders alive, so they have been removed. That means all the conversion code is now generated at compile time and there's no runtime overhead anymore. I think I am going to add a @FromJson annotation to make it explicit that you want to bind from the JSON body. This should avoid some unnecessary CPU cycles during compilation but should also allow you to use model binding by default and then use JSON in other cases.
As I go through the code to extend it, I am seeing a lot of places where I could stand to do some clean up. It's hard to believe a lot of this code is stuff I was writing only a few months after I start programming in Java professionally 2 years ago. I see things like (TypeElement)e without a space between and I see my C# background shining through. But more than that, there's just a lot of complexity related to searching source code elements (class, methods, fields, etc.) and code generation. I might try to do a little cleanup before I release this into the wild.
Okay, so I got my first call to a static conversion method working with the @Converter and @UseConverter annotations. There's a lot of edge cases to handle here but I've PoC'd that my approach is going to work. I just need to spit out the rest of that code and I can close the chapter on converters.
One thing I was thinking about in regards to code quality and the maintainability of this project: I am debating whether I need to break up the code a little bit. There's an emerging pattern where I loop over the source elements (classes, methods, fields, annotations, etc.) and I extract bits of information I need later. Then there's another phase where I combine all that information and start generating the code and I seem to be struggling with passing around of the information that I need. So once I get all my tests passing and features in place, I will probably take a short period of time to try to make some sense of the code... because, to be honest, it is getting hard to see the forest through the trees. This code also just needs a good once over to clean up some of my bad coding style from back when. Going through this code will also be good for identifying additional edge cases I should write tests for.
So... my short term goals: finish up @Converter/@UseConverter, implement the @Prefix annotation, implement the @FromJson annotation, implement the @Unbound annotation, expand unit test coverage, and refactor annotation processing code. Then I will think about releasing to the wild. I might also spend some time trying to figure out why when developing locally with Intellij it seems to not pick up changes to my controllers until I delete my target directory. I am hoping it is because I am working in the same project as the annotation processor itself. We'll see...
I also might spend some time thinking about how to tie together session information for WebSockets. It'd be nice if there was a way to share state across calls to the same session, even if it's just a Map<String, Object> or something I maintain in the background using a ConcurrentMap but there's also no reason users can't do that for themselves and I don't want to introduce features no one is asking for.