Clarify methods under various dispatch types.

[gregw]

While considering #308 and some jetty TCK failures with getHttpServletMapping, it became apparent that there are some inconsistent and undefined behaviours under various dispatch types. In the summary below I have indicated if under a given dispatch type if a method should return values for:

• target The target of the current dispatch, ie the Servlet that a dispatch is to.
• source The source of the current dispatch, ie the Servlet that was the target when the dispatch was done or when the async cycle was started.
• target^source Merged values of both the source and target, with target having precedence.
• target||source Target value if there is one, else the source value.

	REQUEST	FORWARD	NAMED FORWARD	INCLUDE	NAMED INCLUDE	ASYNC	ERROR
getRequetURI	target	target	source[9]	source	source[9]	target	target
get(Context|Servlet)Path(Info)?	target	target	source[9]	source	source[9]	target	target
getParameter	target	target^source	source[9]	target^source	source[9]	target^source	target[7]
getQueryString	target	target||source[1]	source[9]	source	source[9]	target||source	target[8]
getHttpServletMapping	target	target	source[9]	source	source[9]	source[2]	target
isUserInRole(String)	target	target	source[9]	target[4]	source[9]	target	target
getAttribute("javax.servlet.forward.*")		source
getAttribute("javax.servlet.include.*")				target
getAttribute("javax.servlet.async.*)						source
getAttribute("javax.servlet.async.mapping")						source[3]
sendError(); getAttribute("javax.servlet.error.*")	target	target	source[9]	target[5]	source[9]	target	target
getSession	target	target	source[9]	target	source[9]	target	target
getPathTranslated	target	target	source[9]	target[6]	source[9]	target	target

Mostly, (other than the bizarre behaviour of include that I never understood and probably lost in ancient history) this table looks pretty sensible. However there are a number of cases that I think need to be clarified:

[1] As per the discussion in #308, the spec doesn't really say what the query string should be during a FORWARD, only what the parameters should be. Jetty currently merges the query string of source and target, while tomcat and undertow use the target if there is one else the source.

[2] Unlike the path methods which return the target values during an ASYNC dispatch, the getHttpServletMapping method is explicitly documented as returning the source mapping (actually I'm a bit dubious about the language in that javadoc, but this interpretation is backed up by a TCK test). Is this really the intended behaviour? Why is it just this method that acts like an INCLUDE during ASYNC?

[3] Given the current interpretation of [2], this means that the attribute javax.servlet.async.mapping also returns the source mapping and thus the target mapping is unavailable during an ASYNC dispatch. Should this attribute be set to the target value?

[4] I can't find anything to say if isUserInRole should be affected by the target Servlet role mappings during an include. During an include, almost all other methods return values of the source, so perhaps the also the sources role mappings should apply? But that is also kind of counter intuitive as a included resource might need it's role mappings and runAs role to generate its content? Perhaps the role mappings should be merged???

[5] If sendError is called during an INCLUDE, it is unclear what the javax.servlet.error.* attributes should be during any subsequent ERROR dispatch. It feels reasonable that it should be the target... but then I could say that for all methods during an include?

[6] If getPathTranslated is called during an INCLUDE, it is unclear what the path should be relative to the source or target context. It feels reasonable that it should be the target... but then I could say that for all methods during an include?

[7] Probably should be target^source

[8] Maybe should be target||source

[9] Named dispatchers are not well defined

Edit: reworded [4] Edit: added [7] Edit: added [8] Edit: added [9]

[gregw]

We just did some testing on the [2] issue with getHttpServletMapping. It appears the currently undertow is implementing it according to the spec, but that tomcat and jetty are implementing according to common sense (dripping with value judgement there :).

@markt-asf is this a known issue for tomcat? Are you intending on fixing it to match the spec or advocating to fix the spec/tck instead? @stuartwdouglas is undertow wedded to it's implementation or is it possible to get it to change?

[markt-asf]

The spec and the Javadoc are not consistent. From the spec

...the HttpServletMapping is not available for servlets that have been obtained with a call to ServletContext.getNamedDispatcher().

and the Javadoc:

If the currently active Servlet invocation was obtained by a call to ServletContext.getNamedDispatcher(java.lang.String), the returned HttpServletMapping is the one corresponding to the path for the mapping last applied to this request.

In the getNamedDispatcher() instance, I think the Javadoc is correct.

I'm not convinced about that TCK test. There wasn't an associated failure when I ran the Jakarta EE 8.0.1 nightly TCK against Tomcat 9.0.x (notes).

I'd advocate to change the text in the Javadoc. I've just been through the archives where the feature was discussed and the intention all along was that the behaviour of getHttpServletMapping() with dispatchers should be consistent with getServletPath() and friends. When Ed proposed the Javadoc text it didn't reflect that for async and none of us spotted it.

[stuartwdouglas]

I am not tied to the current behaviour in Undertow, it can be changed if required, I just implemented what the spec said.

[arjantijms]

I'm also now implementing what the spec says, and would love some clarifications. As I'm also much involved in security, I particularly wonder about isUserInRole().

Even in a single REQUEST dispatch with filters, there's also the question when the Servlet role mappings should be taken into account. Not until the target Servlet is actually invoked, or as soon as the filter chain is entered?

[gregw]

@markt-asf @stuartwdouglas I'd like to make progress on this one and have opened https://github.com/eclipse-ee4j/jakartaee-tck/issues/585 as I think the TCK is wrong. If you are in agreement, the I'll create PR to fix the test.

[arjantijms]

For Piranha I also mostly implemented what the spec said and what the TCK expected. I'll check next what Piranha exactly does, but from the top of my head we pass the Servlet 4.0 mapping dispatch test.

[gregw]

I've opened https://github.com/eclipse-ee4j/servlet-api/issues/385 to get the TCK updated

[markt-asf]

Hmm. Something doesn't add up here and I need some time to dig into it. I believe that Tomcat implements the behaviour described in the updated Javadoc but Tomcat also passes the TCK test in question. That doesn't seem possible so I want to figure out where I have things wrong.

[markt-asf]

Figured it out. Tomcat's behaviour is configurable. The default behaviour is consistent with the updated text agreed here but when running the TCK Tomcat is put into "strict spec compliance mode" and does what the TCK expects.

[patric-r]

Regarding [1]:

I would find it inconsistent if getQueryString() would not get merged while getParameterMap() gets merged. For GET requests this means the forward targets has an inconsistent view when comparing query string and parameter map, For POST requests this means that the old parameters in the original query string are lost.

IMHO, merging getQueryString() is the only valid option.

[gregw]

I've updated the table in the original post with some questions. It would be good to close this one off and get our consensus for it captured in documentation and/or javadoc.

I find the source||target behaviour for query strings really strange. It should always be one or the other or the merge of them.

[markt-asf]

[1] Given we have RequestDispatcher.INCLUDE_QUERY_STRING and RequestDispatcher.FORWARD_QUERY_STRING I think this should be target for consistency. Same for ASYNC.

[2] has been fixed to be target

[3] is not an issue now [2] has been fixed

[4] Given a resoure may need the role mappings to work, I think this should be target for all columns

[5] I tend to think of an ERROR dispatch as being similar to a FORWARD so target makes sense to me here

[6] For consistency with the other methods, I'd expect getPathTranslated() to use source

[7]&[8] I tend to think of an ERROR dispatch as being similar to a FORWARD so I'd expect this to be the same as FORWARD

[9] Named Include - source for all apart from getSession() and isUserInRole() Named Forward - target for all apart from anything path related which is source (since there is no new path)

[10] I think getSession should also be target for all columns

[gregw]

@markt-asf any chance you could update the table with your feedback. I tried, but there were just a few too many cells that didn't seam right (e.g. "Named Forward - target for all apart from anything path related" doesn't make sense for getParameter)

[markt-asf]

Sure. I'll add a copy of the table with what I think it should be then we can iterate on the differences and hopefully reach consensus.

[markt-asf]

	REQUEST	FORWARD	NAMED FORWARD	INCLUDE	NAMED INCLUDE	ASYNC	ERROR
getRequetURI	target	target	source	source	source	target	target
get(Context|Servlet)Path(Info)?	target	target	source	source	source	target	target
getParameter	target	target^source	source	target^source	source	target^source	target^source
getQueryString	target	target	source	source	source	target	target
getHttpServletMapping	target	target	source	source	source	target	target
isUserInRole(String)	target	target	target	target	target	target	target
getAttribute("javax.servlet.forward.*")		source
getAttribute("javax.servlet.include.*")				target
getAttribute("javax.servlet.async.*)						source
getAttribute("javax.servlet.async.mapping")						source
sendError(); getAttribute("javax.servlet.error.*")	target	target	target	target	target	target	target
getSession	target	target	target	target	target	target	target
getPathTranslated	target	target	source	source	source	target	target

I have used italics for cells where I think we have agreed a change. I have used bold for cells where I think a change is required but there isn't agreement (or I'm not sure there is agreement).