Re: [Patch] #13 resubmitted

[Berin Loritsch <bl...@apache.org>]

Anton Tagunov wrote:
> Hello Berin!
> 
> AT> +    public void setContextDirectory( final URL url )
> 
> Further investigation has shown that this patch _is_ needed.
> In fact, in a servlet I do
> 
> URL urlRoot = servletContext.getResource( "/WEB-INF/avalon" );
> String stringRoot = urlRoot.toExternalFort();
> FortressConfig.setContextDirectory( stringRoot );

You can bind a File object directly like this:

FortressConfig.setContextDirectory(
     servletContext.getRealPath( "/WEB-INF/avalon" ) );

> and I get something like (writing by memory)
>     file:/C:/file/D:\goj\t331ah\webapps\tst\WEB-INF\avalon
> on Windows.
> 
> When however I first create a DefaultContext() and bind in
> urlRoot as "context-root" to it and create FortressContext
> on it then all is ok
>     file:D:\goj\t331ah\webapps\tst\WEB-INF\avalon
> 
> So, I'm resending the patch.


The problem here is a bug in the toExternalForm() method.
This is most likely from your J2EE servlet container vendor.
Notice that there is no colon after the second file?  There
is also no protocol named "file/D" so that is why it fails.
The thing is I believe Fortress expects a File root, although
it is primarily for the SourceResolver--which can handle both
URL and File.

We need to make it simple to work with--and either work with
Files or URLs.  The thing is that some vendors like IBM
WebSphere relocate everything and use a "classloader:" protocol.
If you use the context root as a relative location for the
log file (as is the default case), then you need a File, and
not a URL.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@avalon.apache.org
For additional commands, e-mail: dev-help@avalon.apache.org

Re[2]: [Patch] #13 resubmitted

[Anton Tagunov <at...@mail.cnt.ru>]

Hello Berin!


AT> +    public void setContextDirectory( final URL url )
AT> Further investigation has shown that this patch _is_ needed.

BL> You can bind a File object directly like this:

BL> FortressConfig.setContextDirectory(
BL>      servletContext.getRealPath( "/WEB-INF/avalon" ) );

Yup, but as you say

BL> The thing is that some vendors like IBM
BL> WebSphere relocate everything and use a "classloader:" protocol.

I was speaking about exactly that!

In fact Servlet spec allowes everything to be run from an unpacked
.war file. It's a perfectly legitimate approach.

BL> We need to make it simple to work with--and either work with
BL> Files or URLs.

Justification for having both:

BL> SourceResolver--which can handle both URL and File.

:-)

BL> If you use the context root as a relative location for the
BL> log file (as is the default case), then you need a File, and
BL> not a URL.

A direct hit! That's why I would never use it for anything
not read-only.

We have a
    (File) servletContext.getAttribute( "javax.servlet.context.tempdir" )
for read-write access (and it's a guarateed File!) but it's not good for
logs either since the servlet container is allowed to wipe tempdir
on its restart.

So, I beleive that the location of the logs should be configured
some other way. Probably directly in a .xlog file or somewhere
in web.xml via some init parameter.

BL> The thing is I believe Fortress expects a File root, although
BL> it is primarily for the
BL> SourceResolver--which can handle both
BL> URL and File.

Well, I beleive that if a developer creates a web application
he should be aware that the applicatoin tree may probably be
read-only for him at runtime. (Or his webapp is not portable.).
If he makes a URL that may be readonly at runtime the context-root,
well, he's asked for it himself. He should know what he is doing.
Shouldn't he?

AT> and I get something like (writing by memory)
AT>     file:/C:/file/D:\goj\t331ah\webapps\tst\WEB-INF\avalon

BL> The problem here is a bug in the toExternalForm() method.
BL> This is most likely from your J2EE servlet container vendor.
BL> Notice that there is no colon after the second file?  There
BL> is also no protocol named "file/D" so that is why it fails.

Sorry about misleading you, Berin :-(
I beleive that it rather the following effect:

import java.io.File;

public class A{
    public static void main(String[] args) throws Exception
    {
        final File inner = new File( "d:/foo.txt" );
        final String innerURL = inner.toURL().toExternalForm();
        final File doubleFile = new File( innerURL );
        System.out.println( doubleFile.toURL() );
    }
}

this outputs
    file:/d:/temp/file:/d:/foo.txt

I have misleaded you by missing one colon in the offending output
(indeed I was in too a hurry and wrote that string by memory).

So it's all about
* Tomcat 3.1.1 (that's what my employer requires)
  returning file:D:<some-path> and FortressConfig
* me casting it to a string via .toExternalForm()
* FortressConfig creating new File( string )

============================

Conclusion:

    public void setContextDirectory( final String directory )
    {
        m_context.put( ContextManagerConstants.CONTEXT_DIRECTORY, new File( directory ) );
    }

will probably do something wrong with "classloader:...",
so I propose to have a

    public void setContextDirectory( final URL url )
    {
        m_context.put( ContextManagerConstants.CONTEXT_DIRECTORY, url );
    }

:-)


Maybe setContextDirectory() is not the best name as proposed in this
patch, maybe we should better to have

setContextDirectory( File )
setContextDirectory( String s ) { .. new File( s ) .. }
setContextURL( URL )
setContextURL( String u ){ .. new URL( u ) .. }

?

or maybe it would be better to stress we it's a context _root_

setContextRootURL( URL )
setContextRootURL( String url )

?

- Anton

P.S.
Currently I do

        final Context defaultContext = FortressConfig.createDefaultConfig();
        
        m_initialFortressContext = new DefaultContext( defaultContext );
        
        final URL contextRoot = getOurContextRoot();
        m_initialFortressContext.put( ContainerConstants.CONTEXT_DIRECTORY, contextRoot );
        
        m_initialFortressContext.put( SERVLET_CONTEXT_CONTEXT_KEY, servletContext );
        
        m_initialFortressContext.makeReadOnly();
        
        final FortressConfig fortressConfig = new FortressConfig( m_initialFortressContext );

so a workaround exists, moreover, it is necessary to do this to get
the servletContext in ( servlet logger factory from logkit will need
it), but still I beleive that it will be cleaner to add two
setContextURL() to FortressConfig

P.P.S.
If approved will send a new patch - with two methods instead of one
and named setContextContextRoot( URL )

P.P.P.S.
No compelling reason to do that before release,
(although servlet-writers like are likely to spend some time guessing
what they really should do :-)
just as the discussion continues
I'm sending in my replies :-)

-- 
Best regards,
 Anton                            mailto:atagunov@mail.cnt.ru


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@avalon.apache.org
For additional commands, e-mail: dev-help@avalon.apache.org