groovy-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yazad Khambata (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (GROOVY-5227) AbstractHttpServlet should extend GenericServlet directly instead of HttpServlet
Date Mon, 05 Jun 2017 04:46:04 GMT

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

Yazad Khambata edited comment on GROOVY-5227 at 6/5/17 4:45 AM:
----------------------------------------------------------------

[~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. Should we extend GenericServlet? 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 (the implicit object session is specific to HTTP protocol, also the request
and response implicit objects are HTTP specific) was never intended to be generic to support
protocols other than HTTP, so we can get that out of the way for now.
I do not with this aspect of the ask in the ticket - we should NOT extend from GenericServlet.

-----------------------------------------
2. do<<Http Method>> methods
-----------------------------------------

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

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)


was (Author: yazad3):
[~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 (the implicit object session is specific to HTTP protocol, also the request
and response implicit objects are HTTP specific) was never intended to be generic to support
protocols other than HTTP, so we can get that out of the way for now.

-----------------------------------------
2. do<<Http Method>> methods
-----------------------------------------

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

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)

Mime
View raw message