search

ninjaframework / ninja

Ninja is a full stack web framework for Java. Rock solid, fast and super productive.
http://www.ninjaframework.org
Apache License 2.0
1.91k stars 520 forks source link

freemarker InvalidReferenceException not catched in Ninja.java #289

Open danielsawan opened 9 years ago

danielsawan commented 9 years ago

I just try in prod to put a wrong variable in template like ${test123}. It throw an error on log which is : freemarker.core.InvalidReferenceException: The following has evaluated to null or missing

And show on the browser : http://puu.sh/eUiOl/8f13f6826d.png

But nothing is intercepted on any method in the Ninja.java file

t3hc13h commented 9 years ago

These exceptions are being handled by ninja.template.TemplateEngineFreemarkerExceptionHandler. The handler renders the error text but does not re-throw the error which is why you're not seeing them bubble up to nina.NinjaDefault.onException.

Initially I thought we could simply re-throw the exception but when I tried that I found that while the main onException is called, the the response writer is closed by the handler and the client sees a OK response with the rendered template + error instead of seeing a 500 result from getInternalServerErrorResult.

It seems to me that design the method of error handling in the FreemarkerExceptionHandler, while nice for reporting template issues, is at odds with the main error handling design and I think we should probably be seeing a 500 at the client?

@raphaelbauer can you confirm what I'm seeing and let us know your thoughts?

raphaelbauer commented 9 years ago

Yes - I think we should see a proper 500 page at the client... A proper description of the problem is here: http://freemarker.org/docs/app_faq.html#misc.faq.niceErrorPage - the only possibility to achieve that is to use full page buffering imho. But full page buffering may degrade the performance...

What do you think? Any ideas welcome :)

t3hc13h commented 9 years ago

I agree that full page buffering is the only way to achieve a 500 for template errors, once the initial servlet buffer is flushed there's no turning back.

There will be some impact from buffering but I think it will be small and overall we get win with a more consistent developer and user experience by not returning partial responses.

That being said I'm going to run some tests with large templates to understand the impact so nobody gets bitten by this later.

Also, maybe it would be a good idea to include a config switch to enable the current error handling logic, it's a good compromise and some devs may still prefer it.

t3hc13h commented 9 years ago

I've opened #32 to discuss a fix.