Bugzilla – Full Text Bug Listing
| Summary: | As a user, I would like to configure Context elements for all web apps | ||
|---|---|---|---|
| Product: | [RT] Gemini.Web | Reporter: | Glyn Normington <glyn.normington> |
| Component: | unknown | Assignee: | Glyn Normington <glyn.normington> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | milesg78, rob.harrop |
| Version: | unspecified | Flags: | glyn.normington:
documentation+
glyn.normington: iplog+ |
| Target Milestone: | 1.1.0.M03-incubation | ||
| Hardware: | PC | ||
| OS: | Mac OS X - Carbon (unsup.) | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Glyn Normington
In 1.0, *some* of the options worked, in particular, the crossContext and contextPath options. In 2.0 these should be supported as well. This should be quite easy to accomplish, you'll just need to read in the context.xml file using the Tomcat digester code. This will populate a Tomcat context object for you. In general though, supporting all the options for the Context tag will not work. One that comes to mind is support for a custom classloader, which is obviously not an option in OSGi Standard Tomcat handles conf/context.xml in ContextConfig.contextConfig() which performs the following steps: * loads conf/context.xml * loads conf/<engine>/<host>/context.xml.default * loads webapp's context.xml Note that ContextConfig.contextConfig obtains conf/context.xml relative to Tomcat's base directory (see ContextConfig.getBaseDir), so ContextConfig.contextConfig is not directly applicable in Virgo (assuming Tomcat configuration files in Virgo are to reside in the config directory). Hi, I am also interested in this functionality and I implemented the web application part i.e. - conf/<engine>/<host>/<web-app>.xml - META-INF/context.xml I will be happy to contribute the implementation if you are interested. Probably I will be able to implement also and the global part (for all web apps) - conf/context.xml - conf/<engine>/<host>/context.xml.default Thanks Violeta Hi Violeta (In reply to comment #3) > Hi, > > I am also interested in this functionality and I implemented the web > application part i.e. > - conf/<engine>/<host>/<web-app>.xml > - META-INF/context.xml > > I will be happy to contribute the implementation if you are interested. Yes, that would be very helpful. Please would you attach a patch to this bug. > > Probably I will be able to implement also and the global part (for all web > apps) > - conf/context.xml > - conf/<engine>/<host>/context.xml.default That would be excellent. > > Thanks > Violeta Regards, Glyn (In reply to comment #4) Please read http://wiki.eclipse.org/Virgo/Community#Contributions for help with making a contribution. Created attachment 172488 [details]
The archive contains the patch that I want to propose.
Hi,
Here is the patch that I want to propose.
Regards
Violeta
Thanks for those files! Some initial comments: 1. I'm concerned by the file copying. Doesn't this pollute the configuration directory which should really be under control of the user? Why is the copy necessary at all? 2. It would be great to have an integration test which validates the function and gives an example of its use. 3. It would also be great to have some unit tests since the code in Tomcat.java has expanded considerably and I suspect not many paths will be exercised by the current test suite. It would probably be necessary to break out one or more helper classes along logical boundaries and mock out any dependencies on Tomcat. 4. There were extraneous carriage returns in the files which made diffing difficult. I removed these using the shell command: tr -d '\r' <inputfile >outputfile It would be really helpful if you could avoid adding carriage returns in future. :-) Some Windows editors support this - not sure what the default behaviour of Eclipse is. Created bug312752 branch to track this work. Captured the initial patch in commit 4feac292ffbb339d33f585f7b86a49e1997e0d8b. Hi Glyn, (In reply to comment #7) > Thanks for those files! Some initial comments: > 1. I'm concerned by the file copying. Doesn't this pollute the configuration > directory which should really be under control of the user? Why is the copy > necessary at all? There are two reasons behind this implementation: - I followed the algorithm in Tomcat's documentation: "Only if a context file does not exist for the application in the $CATALINA_BASE/conf/[enginename]/[hostname]/, in an individual file at /META-INF/context.xml inside the application files. If the web application is packaged as a WAR then /META-INF/context.xml will be copied to $CATALINA_BASE/conf/[enginename]/[hostname]/ and renamed to match the application's context path. Once this file exists, it will not be replaced if a new WAR with a newer /META-INF/context.xml is placed in the host's appBase." (http://tomcat.apache.org/tomcat-6.0-doc/config/context.html) - I want to set the config file to the StandardContext and the method parameter is path to the file (context.setConfigFile(contextXml.getAbsolutePath());). When we installing an archive I cannot set this config file if it is not copied outside of the archive. > 2. It would be great to have an integration test which validates the function > and gives an example of its use. > 3. It would also be great to have some unit tests since the code in Tomcat.java > has expanded considerably and I suspect not many paths will be exercised by the > current test suite. It would probably be necessary to break out one or more > helper classes along logical boundaries and mock out any dependencies on > Tomcat. Sure I will prepare the tests. > 4. There were extraneous carriage returns in the files which made diffing > difficult. I removed these using the shell command: > tr -d '\r' <inputfile >outputfile > It would be really helpful if you could avoid adding carriage returns in > future. :-) Some Windows editors support this - not sure what the default > behaviour of Eclipse is. I'm using Windows and Eclipse and I'll take care in the future :) Regards Violeta Thanks for the explanation. I'll look forward to the tests. Created attachment 173496 [details] The archive contains the patch that I want to propose. Hi Glyn, Here is the new patch that I prepared. I changed a little bit the implementation and also added the missing global context.xml files functionality - conf/context.xml - conf/Catalina/localhost/context.xml.default I removed copying of the web application's context.xml and now the copy is done only in case the doc base is a jar file. I want to mention also one point in the implementation - you will see that I'm using a relative path for the global context.xml. This is because in the current Tomcat implementation it is not possible to use absolute paths regardless of the fact that in the javadoc of the setter method it is stated that the absolute paths are acceptable (ContextConfig.setDefaultContextXml(String path)). I reported a but to Tomcat and they replied that this is fixed in 7.0.x and will be proposed for fixing in 6.0.x (https://issues.apache.org/bugzilla/show_bug.cgi?id=49551) I also added JUnit tests and an integration test. I hope that I removed correctly the carriage returns this time. I'm looking forward to your comments. Regards Violeta Created attachment 173508 [details]
The archive contains the patch that I want to propose.
Resources created during integration test are cleared before exit.
Excuse me for missing this.
Regards
Violeta
(In reply to comment #12) Thanks for adding the tests - this patch is looking really good. I've committed and pushed your changes on the branch with just one change in a comment (incorrect apostrophe - a mistake even native English speakers make fairly frequently :-) ). The next step is to tweak the tests to avoid test files (specifically a xml file in config/Catalina/localhost) going directly into org.eclipse.gemini.web.test rather than org.eclipse.gemini.web.test/target which is our usual convention. I would also like to examine the code more closely. I am wondering, for example, whether some of the static methods should be instance methods with longer-lived parameters passed to a constructor. Also we have a coding convention of minimising scope, so it may be possible to change a public class to be package visibility. I will look at this in the next few days and get back to you if there are any questions. Please don't make any of the above changes for now as I'd like to avoid working on a moving base. Before long we should be able to merge the branch into master. (In reply to comment #13) > The next step is to tweak the tests to avoid test files (specifically a xml > file in config/Catalina/localhost) going directly into > org.eclipse.gemini.web.test rather than org.eclipse.gemini.web.test/target > which is our usual convention. I moved the generated config into target and set a system property appropriately. I had to modify TomcatConfigLocator to work around what may be an Equinox bug (see bug 319679). See commit d54f545f7e9accd456b2b94ec279217ee91bdf5c for details. However TomcatServletContainerTests.testWarWithContextXml now fails with a HTTP 500 and I haven't had a chance to investigate further. (In reply to comment #14) > I moved the generated config into target and set a system property > appropriately. I had to modify TomcatConfigLocator to work around what may be > an Equinox bug (see bug 319679). Thanks > See commit d54f545f7e9accd456b2b94ec279217ee91bdf5c for details. > However TomcatServletContainerTests.testWarWithContextXml now fails with a > HTTP 500 and I haven't had a chance to investigate further. Could you please change in the TomcatServletContainerTests.beforeClass() method, the value of the property: + @BeforeClass + public static void beforeClass() throws Exception { + System.setProperty("org.eclipse.gemini.web.tomcat.config.path", "target/config"); + } instead of "target/config", we need "target/config/tomcat-server.xml" with this change the build on my system is successful. Thanks Violeta In addition to my previous comment. I think that we do not need this file: org.eclipse.gemini.web.test/config/Catalina/localhost/war-with-context-xml-resources.xml it will be generated automatically Thanks Violeta Hi Violeta Thanks. I've corrected the property and deleted the file (checked in accidentally - apologies). The next problem is that ant precommit shows a findbug error which needs fixing. WebappConfigLocator.copyFile does not test the return value of mkdirs. Please could you take a look and advise? Regards, Glyn Hi Glyn,
Let's do it as follows:
OLD
private static void copyFile(InputStream source, File destination) throws IOException {
159 destination.getParentFile().mkdirs();
...
}
NEW
private static void copyFile(InputStream source, File destination) throws IOException {
159 if (!destination.getParentFile().mkdirs()) {
160 throw new IOException("Cannot create parent directories for file " + destination.getAbsolutePath());
161 }
...
}
Excuse me for missing this. I'll check the findbugs report in the future.
Thanks
Violeta
(In reply to comment #18) That change made a test fail: [junit] Testsuite: org.eclipse.gemini.web.tomcat.internal.WebappConfigLocatorTest [junit] Tests run: 3, Failures: 0, Errors: 1, Time elapsed: 0.067 sec [junit] [junit] Testcase: testResolveWebappContextXml(org.eclipse.gemini.web.tomcat.internal.WebappConfigLocatorTest): Caused an ERROR [junit] Cannot copy /Users/glynnormington/eclipse/gemini/org.eclipse.gemini.web.gemini-web-container/org.eclipse.gemini.web.tomcat/target/test-classes/test1.xml to /Users/glynnormington/eclipse/gemini/org.eclipse.gemini.web.gemini-web-container/org.eclipse.gemini.web.tomcat/target/test-classes/test1.xml [junit] org.eclipse.gemini.web.core.spi.ServletContainerException: Cannot copy /Users/glynnormington/eclipse/gemini/org.eclipse.gemini.web.gemini-web-container/org.eclipse.gemini.web.tomcat/target/test-classes/test1.xml to /Users/glynnormington/eclipse/gemini/org.eclipse.gemini.web.gemini-web-container/org.eclipse.gemini.web.tomcat/target/test-classes/test1.xml [junit] at org.eclipse.gemini.web.tomcat.internal.WebappConfigLocator.resolveWebappContextXml(WebappConfigLocator.java:120) [junit] at org.eclipse.gemini.web.tomcat.internal.WebappConfigLocatorTest.testResolveWebappContextXml(WebappConfigLocatorTest.java:147) [junit] Caused by: java.io.IOException: Cannot create parent directories for file '/Users/glynnormington/eclipse/gemini/org.eclipse.gemini.web.gemini-web-container/org.eclipse.gemini.web.tomcat/target/test-classes/test1.xml' [junit] at org.eclipse.gemini.web.tomcat.internal.WebappConfigLocator.copyFile(WebappConfigLocator.java:160) [junit] at org.eclipse.gemini.web.tomcat.internal.WebappConfigLocator.resolveWebappContextXml(WebappConfigLocator.java:118) I wonder if mkdirs returns false if the directory to be created already exists? If so, it may be necessary to avoid calling mkdirs in that case so that the return code can be interpreted as failure. I'll let you decide... I should have pointed out that we use org.eclipse.virgo.util.io.PathReference elsewhere for such tasks. Please take a look at it and see if you would like to use something like the following sequence: PathReference dest = new PathReference(destination); dest.getParent().createDirectory(); This will throw an unchecked exception if and only if there is a failure to create the parent directory. (In reply to comment #20) > I should have pointed out that we use org.eclipse.virgo.util.io.PathReference > elsewhere for such tasks. Please take a look at it and see if you would like to > use something like the following sequence: > PathReference dest = new PathReference(destination); > dest.getParent().createDirectory(); > This will throw an unchecked exception if and only if there is a failure to > create the parent directory. Hi Glyn, I look at the PathReference and I absolutely agree with your proposal. Let's use it. Regards Violeta (In reply to comment #21) > Hi Glyn, > > I look at the PathReference and I absolutely agree with your proposal. > Let's use it. > > Regards > Violeta That fixed the problem and precommit now passes. I'm now "reverse" merging master into our branch and re-testing in preparation for merging our branch into master. Hi Violeta It occurs to me that the only documentation of the new function is in this bug. Would you be able to take a look at the existing Virgo configuration documentation [1] and propose a paragraph or two to explain the new function from a user's perspective? (If you can't do this, it will need to wait for me to get around to it.) Thanks, Glyn [1] http://www.eclipse.org/virgo/documentation/virgo-documentation-2.1.0.M02-incubation/docs/virgo-user-guide/htmlsingle/virgo-user-guide.html#configuring-tomcat Hi Violeta In preparation for merging this branch and in view of the due diligence process [1], please could you confirm that you wrote 100% of the code you contributed in this bug and that you have the right to contribute the code to Eclipse. I note that the file headers now contain the appropriate License header. I should have confirmed this before pushing the code to Eclipse. My mistake, for which I apologise. Thanks, Glyn [1] http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf Hi Glyn, I confirm that I wrote 100% of the patch and I have the right to contribute it to Eclipse. Regards Violeta Created attachment 174503 [details]
User documentation
Hi,
Here is the user docu that I want to propose for this functionality.
Regards
Violeta
(In reply to comment #26) Hi Violeta Thanks for that documentation which I will include in the Virgo user guide as we don't currently provide any documentation for Gemini Web other than the OSGi specification. I made some editorial and stylistic changes which I hope you won't mind, but I wanted to check something with you. You refer to the directory: $SERVER_HOME/config/[enginename]/[hostname] I think it's true that the web container uses a single default engine and so [enginename]=Catalina. Do you agree? Regards, Glyn (In reply to comment #27) Hi Glyn, > but I > wanted to check something with you. You refer to the directory: > $SERVER_HOME/config/[enginename]/[hostname] > I think it's true that the web container uses a single default engine and so > [enginename]=Catalina. Do you agree? Yes the default name is Catalina, but you can change the name of the engine in the server.xml instead of <Engine name="Catalina" defaultHost="localhost"> you can rename the engine to something else <Engine name="MyServer" defaultHost="localhost"> because of this I did not specify it as "Catalina" in the documentation. Regards Violeta Created attachment 174832 [details]
Screenshot of documentation update
(In reply to comment #28) Thanks Violeta - I had missed that. Please would you review the attached screenshot [1] of the updated documentation and see if you are happy with it. (I have added you to the list of authors and I am investigating if the copyright needs tweaking.) Regards, Glyn [1] https://bugs.eclipse.org/bugs/attachment.cgi?id=174832 > screenshot [1] of the updated documentation and see if you are happy with it.
I'm happy with it ;)
Regards
Violeta
Code merged into master by 002e9724e9fba7cd4484cb5c260d2fde6ea8ad10 and pushed. Documentation added in Virgo documentation commit 7b582d3d21d5255485d083a95e68db6d11c68793. |