Bug 57142 - JSP 2.3 & EL 3.0 - %page import directive & EL ImportHandler
Summary: JSP 2.3 & EL 3.0 - %page import directive & EL ImportHandler
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Jasper (show other bugs)
Version: 8.0.x-trunk
Hardware: PC All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on: 57141
Blocks:
  Show dependency tree
 
Reported: 2014-10-24 21:57 UTC by Arthur Fiedler
Modified: 2014-12-10 21:15 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Fiedler 2014-10-24 21:57:09 UTC
The JSP 2.3 Spec mentions in "Table JSP.1-8" "Page Directive Attributes" that "An import attribute describes the types that are available to
the scripting environment." 

So if you had a JSP page with <%page import="org.test.*" the "org.test" package reference does not make it into the EL context's ImportHandler for resolving classes. This all does depend however, if as by the term "scripting environment" the EL expressions are included.

If JSP 2.3 is to support resolving static fields etc(Bug #57141), like enums then it makes sense that the import attribute would make available those packages/classes to the EL processor. To make custom classes/enums work other than the default java.lang.* for example ${MyCarEnum.PONTIAC == car.make}

The current workaround for importing packages into the EL processor in jsp pages is to use something like the following in the contextInitialized method inside a ServletContextListener...

JspFactory.getDefaultFactory().getJspApplicationContext(sce.getServletContext()).addELContextListener((ELContextEvent e) -> {
    e.getELContext().getImportHandler().importPackage("org.test.util");
    e.getELContext().getImportHandler().importPackage("org.test.enums");
});
Comment 1 Mark Thomas 2014-11-01 23:57:26 UTC
This is one of several areas that could benefit from some clarification in the JSP spec.

Note the approach suggested above will add the imports to every JSP page in the application. On a per page basis you can use:
<%
pageContext.getELContext().getImportHandler().importStatic("java.lang.Integer.MAX_VALUE");
%>

My current thinking os that this behaviour could be optional - i.e. controlled by configuration - until such time as there is some clarification in the JSP spec.
Comment 2 Mark Thomas 2014-11-02 00:41:56 UTC
Marking this as an enahncement request.

Configuration via a servlet context init-param is probably the way I'd look to implement this. Not sure whether to enable or disable by default at this point.
Comment 3 Konstantin Kolinko 2014-11-02 19:21:42 UTC
> The JSP 2.3 Spec mentions in "Table JSP.1-8" "Page Directive Attributes" that
> "An import attribute describes the types that are available to
> the scripting environment." 

Originally the "scripting environment" as mentioned in JSP spec is just the java language. If that covers EL as well then it would better be explicitly explained as a chapter in section JSP.2 that defines interaction between EL and JSP.


In chapter JSP.1.10.1 The page Directive in Table JSP.1-8 it also says that
"Packages java.lang.*, javax.servlet.*, javax.servlet.jsp.*, and javax.servlet.http.* are imported implicitely by the JSP container."

The EL.3.0 chapter 1.22.1 Access Restrictions and Imports says that only "java.lang.*" is imported, for security reasons.

[quote]
For security, the following restrictions are enforced.
... 3. Except for classes with java.lang.* package names, a class has to be explicitly imported before its static fields or methods can be referenced.
[/quote]

So if page imports should affect the EL, then how does it deal with those default imports from servlet packages?  The safer default would be to just import java.lang.*, but it would be odd to require someone to explicitly import those servlet packages with @page directive.
Comment 4 Arthur Fiedler 2014-11-04 00:58:15 UTC
I think it would be fair to say to merge the two statements. JSP spec says as you stated, "Packages java.lang.*, javax.servlet.*, javax.servlet.jsp.*, and javax.servlet.http.* are imported implicitly by the JSP container."

Then the EL spec says "For security, the following restrictions are enforced.
... 3. Except for classes with java.lang.* package names, a class has to be explicitly imported before its static fields or methods can be referenced."

Meaning that "javax.servlet.*, javax.servlet.jsp.* and javax.servlet.http.*" would NOT be included by default into the EL import handler. Unless "explicitly" imported using @page import="".

Because at face value when you make a JSP page that you specify <@page import="org.test.*"... you expect EL to be able to use those objects. After you call ${MyCarEnum.PONTIAC} you start bashing your head as to why X server is bugged as <%= MyCarEnum.PONTIAC %> is working, and the current recommendation is to use EL instead of scriptlets. Not only does this confuse the user on what to do next and how to get it working (they would need to find this page, and use Marks work-around or apply to all of jsp) it also confuses IDE makers as to how to provide auto-complete for EL syntax, the page directive would make the most sense.

I also would provide this functionality by default until the spec is clarified(how long will that take), because a user is just going to think this is a bug and try to work around it with scriptlets most likely. Or if you guys have the contacts to get an official clarification that would be ideal.

These comments are however just my two cents.
Comment 5 Mark Thomas 2014-11-04 09:32:17 UTC
Those are all good points. Please can you add them to https://java.net/jira/browse/JSP-44 ?
Comment 6 Arthur Fiedler 2014-11-04 22:15:31 UTC
Thanks Mark. I'll add them tonight
Comment 7 Arthur Fiedler 2014-11-13 15:30:36 UTC
It seems that the clarification recommendation(per kchung on the JIRA link mentioned in comment#5) is that EL is covered by the "scripting environment" label in relation to the JSP page import attribute. The EL environment should contain all the default packages that are included in JSP, to keep it consistent. Those being java.lang.*, javax.servlet.*, javax.servlet.jsp.*, and javax.servlet.http.*

Also currently the imports should be resolved in the ScopedAttributeELResolver for now, as bug#57141 corrects already
Comment 8 Mark Thomas 2014-11-27 21:17:06 UTC
Fixed in trunk. I'll leave it a few days for folks to comment before back-porting it to 8.0.x.
Comment 9 Konstantin Kolinko 2014-11-28 14:40:53 UTC
Reviewing r1642233 and r1642280.

In general:
a) I wonder whether we can do some work once when JSP page servlet class initializes, instead of doing everything a-new on each request.

b) I wonder whether all ImportHandler.importFoo() methods can be made faster. 

c) I think that pageContext.getELContext() is not fast, as it has to initialize the ELContext, and ELContext is not reused between requests.


For b):
======
1. E.g. is Class.forName(), Package.getPackage() validation needed at import time? Can it store imports as Map<String shortName,String fullName> and defer Class.forName() calls?

From Javadoc, I do not see a need for validation at import time.

http://docs.oracle.com/javaee/7/api/javax/el/ImportHandler.html

The javadoc mentions trivial "class name contains '.'" check. We need to do lastIndexOf('.') to split class name from package name and "no '.'" found is a clear error condition.


2. I think there is a bug in ImportHandler.findClass().

It shall not use Class.forName(), but use TCCL. I expect that Class.forName() does not use WebappClassLoader and that it cannot load webapp classes.

(Not tested)

As a contrast, ImportHandler.importPackage() already uses TCCL when validating a Package.


For c):
=====
3. Move the code that initializes imports from JSP class (the code that Generator adds in r1642233) and move it into PageContextImpl.getELContext() where ELContext is created.

This removes the need for PageInfo.isELUsed() flag added in r1642233 and premature initialization of ELContext that Generator adds to JSP pages.  The PageContextImpl.getELContext() method may initialize it when it is needed for the first time.

Trivial
======
4. in ImportHandler.findClass() r1642280:
The "int modifiers = clazz.getModifiers();" call can be skipped when validate==false.
Comment 10 Arthur Fiedler 2014-12-02 07:52:01 UTC
Per comment #9's mention of #2 I ran into this issue with the class loader before, I believe what I was doing was accessing EL 3.0 classes directly in the context initialize listener and it could not see my classes, I ended up writing my own EL Resolver that resolved my classes, since my resolver was created in the servlets class loader.

I wasn't sure how to fix it or how its truly suppose to function so I didn't report it and made my own work around.

As for "C" is it better to reuse the ELContext instead of creating it each time? Wouldn't the object need to be reset and synchronized for every request to that jsp? I didn't look at the code so I don't know the in's and outs of how jsps are processed, but if you wanted to do that maybe a ThreadModel would work to avoid synchronizing, just a thought.
Comment 11 Mark Thomas 2014-12-09 21:35:11 UTC
(In reply to Konstantin Kolinko from comment #9)
> a)

We could do some pre-processing of the imports. Not sure how much it would speed things up. Needs some testing to see if this part is a bottle neck.

> b) 1.

Yes, I think things could be deferred which should give some performance improvements. Again, testing is required to see how much gain we get from this.

> b) 2.

Agreed. Possible bug. Need to test and confirm.
 
> c)

PageContextImpl doesn't have access to the imports. They'd have to be exposed somehow e.g. via the Servlet.
Comment 12 Mark Thomas 2014-12-10 15:07:22 UTC
Switch this to bug now we have clarification of the required behaviour from the spec lead.
Comment 13 Mark Thomas 2014-12-10 20:20:51 UTC
a) and c) are addressed in r1644516 for trunk.

Looking at b) next.
Comment 14 Mark Thomas 2014-12-10 21:09:22 UTC
b) address in r1644523 for trunk.
Comment 15 Mark Thomas 2014-12-10 21:15:57 UTC
This has been fixed in 8.0.x for 8.0.16 onwards.