Remove Groovy dependencies from framework

[cies]

I may be wrong, so please bear with me.

The existence of the fastergt plugin hints that RePlay provides GroovyTemplates as a plugin. But GroovyTemplates are also used by framework, hence framework depending on Groovy.

A good reason for this is that the error pages (in framework/errors/) are GT-based.

Groovy is quite a heavy dependency, and we are well underway to port all GT code over to Kotlinx.html. Would it be cool if I took a stab at removing Groovy as a dependency of framework: error pages should still work out of the box (I'll probably code them in Java then), but GT functionality then depends loading the fastergt plugin.

Maybe I miss that fastergt only makes the GTs faster, and that this functionality is intended to be "slow as part of framework" and only fast when the plugin is loaded.

[asolntsev]

Yes, that's right: initially module framework (inherited from Play1 framework) contained Groovy-based templates engine. Later we started using "faster Groovy Templates" plugin and added it to Replay as fastergt module. It's enabled by default, so all Groovy code in framework is not needed anymore.

Good point, I will remove it.

[cies]

I think Groovy clocks in as one of RePlay's heaviest dependencies.

Good point, I will remove it.

As said, this will require a new way to deal with the error page rendering (currently Groovy based).

[asolntsev]

@cies But how do you solve error page rendering in your Kotlin-based project?

[cies]

We have not come to the error pages yet. They are at the bottom of our list. We are currently on 95% Kotlinx.html in the "customer login" part, but for the "shops part" we will port over the GT pages to an SPA approach later next year.

The error pages are the same for both those parts and we override some that come with RePlay with out own:

$ ls app/views/errors/
400.html  403.html  404.html  500.html  errorLayout.html

I've briefly looked into how it is possible to port the GT for error pages to Kotlinx.html and found that would likely require some changes in RePlay as it currently is. It was a quick assessment some time ago, so I'm not really sure. Back then I thought it was acceptable, given that that our error pages are few in number and low in logic.

If you are interested in the approach I'd take for this, I can have a look at it:

• removing GT from framework
• porting the error pages in framework to pure Java
• designing some way to override the default error pages

Off the bat see some ways overriding can be performed:

• It could use the router to find an error route (say on /404) and if that does not exist, it can render the default.
• RePlay could provide an interface with a default implementation

[asolntsev]

@cies Probably I see a simple solution here. We can use the fact that every Replay plugin can override method loadTemplate. So,

1. Files "400.html" , "403.html" etc. in framework module will be simple html files, without any Groovy code.
2. Module fastergt will override these files by generating these pages with Groovy code.
3. You will be able to customize these page by Kotlin in your project.

[cies]

Not sure if that works for us. To illustrate this, 2 snippets of template invocation:

return viewResult().with("bulkRefunds", refunds);

and

return new views.intern.ui.refunds.Index(this).render(refunds);

As you can see we do not use the template selecting/loading/rendering engine at all! Our templates are now simple method (or function) calls. This makes 'm very type safe (no Map<String, Any?> and/or renderArgs and/or get/set in Groovy code to pass the parameters/arguments around).

So the approach you suggest would:

• select the template based on a String 404.html
• pass the arguments as Map<String, Object>
• require us to make a KotlinTemplate class that extends the abstract Template

Certainly this is doable, and a great way to rid ourselves eventually from the dependency on Groovy, so that's great!

Yet I was more interested in an approach that does not involve Map<String, Any?>-based coupling. Like an interface that implemented by a default-RePlay implementation, but where for a own implementation can be supplied during RePlay's start up.

Either way I'd be very happy with the result :) Just that we are trying to move towards typed APIs.

[cies]

Just did a bit of research on removing Groovy from framework. In order to do so:

• The Play class' injectStaticFields() method (and friends) need to be removed and called from fastergt instead of Play.start().
• play.templates.JavaExtensions needs to be moved tofastergt.
• play.templates.GroovyInlineTags needs to be moved tofastergt.
• FastTags needs to be moved to fastergt, and given that it literally depends on fastergt: that's clearly where it belongs.
• ExecutableTemplate needs to be moved to fastergt.
• Then framework's build.gradle can have it's dependency on Groovy removed.

But this is not enough. Stopping here will make the templates in framework/src/errors/ not renderable, at least not without the fastergt plugin. So we need a means to providing the framework with a specification of how to render these instead. We could use an interface for that:

public interface ErrorHandler {
  Result serve400(ActionContext actionContext);
  Result serve403(ActionContext actionContext);
  Result serve404(ActionContext actionContext);
  Result serve500(ActionContext actionContext);
}

But this would change the way a replay app needs to be started. So there's probably some way in Java to allow for a default implementation that can be overwritten. The default can then simply fail at runtime if fastergt is not loaded, hinting in the error msg that there are to solutions: adding fastergt or providing an alternative implementation of ErrorHandler.

It's quite a bit of work. I'm interested in doing it but only if you, @asolntsev, agree with the approach and think it is both feasible and worthwhile. Thanks for looking into this, again (we're close to having all groovy code ported to kotlinx-html and I would not mind dropping this dependency entirely -- dynamic execution is box of attack vectors in my opinion, and it's the largest dependency, larger than hibernate).

[asolntsev]

@cies Generally, I agree with your suggestions. Let's do it.

The only thing I would suggest is the default implementation of ErrorHandler methods.
By default, they could generate "500 error", "400 error" by using plain text templates. Just hardcode string response - either in constants or in "*.html" files. And maybe do some trivial string substitution if needed.

[cies]

Sounds good. What about the defaults? Simply by making the methods default (with implementations you just mentioned) on the interface of ErrorHandler?

And to supply the an implementation of ErrorHandler we add a constructor to Play? E.g.:

public Play(ConfLoader confLoader, BeanSource beanSource, SessionStore sessionStore, ErrorHandler errorHandler) {
  // ...
}

[cies]

After my first attempt (take it a s a draft), these are my findings:

• The interface suggested turned out to be a bit different: one method for app-errors and one for framework-errors as they require different arguments/ return values.
• I made two implementations of the interface: SimpleErrorHandler and TemplateErrorHandler. With the last one I tried to keep things working as before and thus requires templates to work.
• The framework itself can only produce 404 and 500 errors. The serve400 method -- as implemented in the different server backend's PlayHandler classes (here netty3's) -- immediately responds from the PlayHandler not invoking any further framework code (so it never used templates before). Question: I'd love to make serve400 also use the ErrorHandler interface to generate its response body, making it behave the same as the 404 and 500 framework errors; shall I?
• I've put the three new files ending on ErrorHandler in the play package. Question: leave them here or put them in a sub-package of play? Any suggestions on which (new) package name are welcome!
• Quite a bit of code has moved from play.templates to fastergt's template2 package. I had to make initialRender() public on Template to achieve this.
• I did not attempt to move all GT-based templating functionality to fastergt as I expect that it is very "built in" into RePlay to the point that it's not possible to achieve it by means of a plugin, i.e. fastergt. Looking into it, it seems TemplateLoader.cleanCompiledCache(), Template, BaseTemplate, View, RenderView and Controller are the places where the coupling is tight. An other PR I submitted, Make "play.mvc.Controller".setContext implementable #223, has been merged and removes the tight coupling on Controller (by adding an interface) which should make it possible to move all templating code to fastergt except for TemplateLoader.cleanCompiledCache() (that could be fixed with a new "hook" method on the PlayPlugin abstract class, or achieved with reflection, or you have the option to supply a TemplateLoader to the Play constructor as we do with Guice, session store, conf loader and error handler). Finally if all template code gets moved to fastergt then pdf also needs to depend on fastergt which it currently does not. I'd love to implement this "total decoupling" but is not as per the PR in it's current shape. Question: shall I proceed to implement this "all the way" decoupling, or do we want to merge the PR in its more/less current form (first)?
• The play.template package contains, apart from templating code, also some formatters/escapers: once the templating code is removed, I suggest these are also put in a different place. The "safe formatters" seem unused (but could be used by someone who built an app on top of RePlay). JavaScriptEscaper is only used in fastergt (as per the PR) so I think it should be moved to the fastergt. Then the play.template package is empty if Template, TemplateLoader and BaseTemplate are also move to fastergt which they are currently not in the PR. Question: can I remove the "safe formatters" and move the JavaScriptEscaper to fastergt?
• The isRenderingTemplate() method found in Result and subclasses is never used it seems (could by used in projects built on RePlay). Question: shall I remove it?
• I made NotFound a subclass of Error (like the others: Forbidden/BadRequest/Unauthorized), so I could remove some logic and make sure all errors are handled by whatever implements the ErrorHandler interface.
• I moved JavaExtensions to fastergt while extracting one method to play.utils.Utils as that was also used by the framework package.

This only when it's decided that we want to decouple template "all the way":

• When all templating code is moved to fastergt then the new TemplateErrorHandler should also move there in which case I feel the framework/src/errors/ folder should also be move to fastergt.

The main question is: shall I proceed to implement "all the way" decoupling (as explained above), or do we want to merge the PR in its approximate current form (first)? In it's current form Groovy is no longer a dependency of framework which was my initial goal. I'd love to hear your thoughts on this.