[jira] [Commented] (GROOVY-5227) AbstractHttpServlet should extend GenericServlet directly instead of HttpServlet

["Yazad Khambata (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GROOVY-5227?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16036524#comment-16036524 ] 

Yazad Khambata commented on GROOVY-5227:
----------------------------------------

[~paulk]

===================
A. In Short
===================
There is logic in the service method in HTTPServlet which is lost when we directly override. While thats definitely an issue, it’s not as bad as it seems, and “most” functionalities seemingly lost are accessible with little or no workarounds.
(Please see the suggested solutions at the bottom)

===================
B. Longer Analysis
===================

First the class hierarchy,

groovy.servlet.GroovyServlet extends groovy.servlet.AbstractHttpServlet extends javax.servlet.http.HttpServlet extends javax.servlet.GenericServlet.
(The code for the HttpServlet is available in the Apache Tomcat project here - https://github.com/apache/tomcat/blob/trunk/java/javax/servlet/http/HttpServlet.java and is critical for this discussion)

-----------------------------------------------
1. HTTP specific or protocol neutral?
-----------------------------------------------

The servlet specification as a whole is not specific to HTTP which is why you have the generic servlet which accepts a generic ServletRequest and ServletResponse objects as parameters to it’s service method. The HttpServlet is a GenericServlet that is specific to HTTP and hence accepts HttpServletRequest and HttpServletResponse as parameters which are in turn sub classes of ServletRequest and ServletResponse respectively. It is clear from the implementation that the GroovyServlet was never intended to be generic to support protocols other than HTTP, so we can get that out of the way for now.

Now coming to what is lost from within the HttpServlet’s service method by overriding it directly.

-----------------------------------------
2. do<<Http Method>> methods
-----------------------------------------
HttpServlet’s service method simply put does the following,

It checks the the HttpServletRequest#getMethod() and conditionally invokes, the methods doGet, doPost, doPut, doDelete, etc.

When we override the service method directly – that conditional logic is lost – and if someone wanted to extend the GroovyServlet to behave a specific way when say the script is invoked as a delete instead of get – that would require reimplementing the logic offered in HttpServlet#service method. But it’s not as bad as it sounds – currently since the service method is overriden directly – irrespective of the HTTP Method used to invoke the groovy script (get, post, put, etc) the script always executes and sends out the script response, besides if someone did want to do something specific in a script based on say post method, the request object is implicitly available in the script. The developer can call the request.getMethod and implement specific logic. This may not be elegant for their use-case but is a viable workaround without working against the internal groovy API.

------------------------------------
3. getLastModified
------------------------------------

Besides the doXYZ methods, the HttpServlet also has a getLastModified method which by default returns -1. The developer is expected to override this method and return an epoch time (since 1970/1/1) . If the servlet is invoked with special headers “If-Modified-Since” and with “GET” method – the HttpServlet#service(HttpServletRequest, HttpServletResponse) checks against the getLastModified. If not changed “If-Modified-Since” - the servlet doesn’t invoke doGet and instead sends a header indicating “Not Modified” (HTTP Response Code 304). There is no good workaround for this since the getLastModified is never called when we directly override the service method in question.

======================
C. Suggested Solution(s)
======================
Suggestion 1: The fix would be to extract out the entire code in the service method in HttpServlet#service(HttpServletRequest, HttpServletResponse) into a separate protected method, say doService. And invoke this doService method from doGet, doPost, doDelete, doPut, doTrace, etc; and do not override the HttpServlet#service method. This is the simplest fix and is unlikely to break existing Groovlets and or developer assumptions. At the same time it will make it easier for people to create a custom implementation of GroovySrevlet to maybe override the getLastModified method if their use-case calls for the same.

Suggestion 2: Accept the behavior as-is and provide a note in the documentation about the behavior. We can add this note in the documentation for the override service method in GroovyServlet class which currently is not very clear about this behavior and simply states “Handle web requests to the GroovyServlet”

(I can help with both or another alternative you have in mind)

> AbstractHttpServlet should extend GenericServlet directly instead of HttpServlet
> --------------------------------------------------------------------------------
>
>                 Key: GROOVY-5227
>                 URL: https://issues.apache.org/jira/browse/GROOVY-5227
>             Project: Groovy
>          Issue Type: Improvement
>          Components: Groovlet / GSP
>    Affects Versions: 1.7.5
>            Reporter: Benjamin Gandon
>            Priority: Trivial
>
> The {{groovy.servlet.AbstractHttpServlet}} class overrides the {{service(HttpServletRequest, HttpServletResponse)}} method.
> Thus all the circuitry implemented in the {{javax.servlet.http.HttpServlet}} superclass is bypassed altogether, except the trivial {{service(ServletRequest, ServletResponse)}} method which only provides a convenient call to {{service(HttpServletRequest, HttpServletResponse)}}, narrowing the request and response types to HTTP-dedicated subclasses.
> None of the doGet, getLastModified, doHead, doPost, doPut, doDelete, doOption, doTrace, is ever called. Are they relevant to any {{AbstractHttpServlet}} subclasses? I doubt so. The programming model of {{GroovyServlet}} currently implies re-implementing in Groovlets the way these HTTP methods should be answered.
> Then it might be a possible optimization to have {{AbstractHttpServlet}} extend {{GenericServlet}} directly, re-implementing in {{AbstractHttpServlet}} the trivial {{service(ServletRequest, ServletResponse)}} method of {{HttpServlet}}. This could prevent the unnecessary loading of the {{HttpServlet}} class in memory.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)