[patch] cleaner PropertiesUtil

[Stephen Haberman <st...@chase3000.com>]

Hello,

Torque uses Texen's PropertiesUtil object and it was giving us problems
in how it guessed where to load files from (e.g. relying on just
templatePath being null to insinuate that useClasspath=false).

So, instead of relying on ad hoc mechanisms for guessing where to load a
properties file from, I updated PropertiesUtil to instead call
RuntimeSingleton.getContent and put the resulting text into a Properties
object.

The patched PropertiesUtil passes all of the unit tests (test-texen and
test-texen-classpath) along with correctly handling for the deprecated
$generator.templatePath being in the propertiesFile parameter. And it
also gets rid of the messy logic on guessing where to load the file from
and instead relies on Velocity's existing resource management
infrastructure.

Does this look okay to commit? I think it works great, and it passes the
tests, but if there are any objections or changes I should make, just
let me know.

Thanks,
Stephen

RE: [patch] cleaner PropertiesUtil

[Stephen Haberman <st...@chase3000.com>]

> > If the properties file was in the classpath, template path, or any
other
> > resource, it would have been found by Velocity, so it'd only have to
try
> > loading with the path as is and not worry about calculating any
template
> > path stuff.
> 
> That's not true - Velocity don't dig around anywhere other than where
the
> configured loaders look, and by default that's the file loader in the
> current directory.

Right, what I meant was, in the current PropertiesUtil, if templatePath
(Texen's templatePath property that maps to the
Velocity.FILE_RESOURCE_LOADER_PATH) was set, it tried to load the
property file off the file system by first prepending the templatePath.
If templatePath was not set, it'd tried to load it out of the classpath.


Given that these were the only two methods tried, the current mechanism
of using ContentResource would find if off the file system as Texen
inherently sets up a FileResourceLoader on the templatePath dir and
would also find it from the classpath as Texen will setup a
ClassPathResourceLoader if it's useClasspath property is set. And I
think it's safe to assume that if users want to load properties via the
classpath, they will already have Texen's useClasspath set (as they are
most likely already loading templates from the classpath).

> Just so you know where I'm coming from, my only concern is that this
doesn't
> break anyone, and second, it's not going to be a bear to maintain in
the
> future.  After that, if Texen users like it, I like it.
>
> Another approach is to try to find the properties file in the
classpath via
> using the classloader, or letting it be set in some system property.
Is
> that better/ more conventional/safer/ more elegant?

That was what the current PropertiesUtil tried to do, but they did some
guessing about whether to try the file system or classpath based on
whether Texen's templatePath was null or not. For Torque, we want to
have one Texen task seamlessly work both from the file system and the
classpath based on whether the user had an Ant ${useClasspath} property
set. So we were setting templatePath regardless of what useClasspath
was, hence PropertiesUtil always thought we wanted properties file from
the file system.

I had made a different patch that instead of assuming templatePath being
set means don't use the classpath, PropertiesUtil would just try the
file system first, then the classpath if that failed..

However, it just didn't sit very well with me, because it was
duplicating a lot of logic that to me the Velocity resource management
system could handle just fine.

After reviewing the current PropertiesUtil code, there are two cases
where the patched PropertiesUtil would break existing code, both of them
dealing with properties files outside of the resource loader's scope:

- a relative path is used, PropertiesUtil forcefully prepends
templatePath, but the relative path backs it way out of the resource
loader's scope via ..\..\..\.

- an abosulte path is used, PropertiesUtil forcesfully prepends
templatePath, but it's empty, so the absolute path would succeed.

Perhaps we could have a more experienced Texen developer, e.g. Jason van
Zyl, Stephane Bailliez, or Leon Messerschmidt (past people who've
committed/worked on PropertiesUtil), and see what they think about
losing the ability to load properties outside of the currently
configured resource loaders? If none of them use such functionality, I
think we could make a strong assumption that the patch is good.

Thanks,
Stephen


--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>

Re: [patch] cleaner PropertiesUtil

["Geir Magnusson Jr." <ge...@adeptra.com>]

On 7/14/02 5:43 PM, "Stephen Haberman" <st...@chase3000.com> wrote:

>> 
>> I didn't look at the patch - but doesn't this mean you'll have to put
> the
>> properties file in path accessible to the resource loader?
>> 
> 
> Yes. I'm not aware of the best practices/current style of where to put
> properties files, but Torque has them mixed in with the templates. Well,
> and the test bed naturally has the properties files mixed in with the
> templates also.
> 
> Hm. Shoot. So I suppose you're saying it's not good to force a resource
> loader to be used in loading a properties file?

Not saying anything.  Just asking.  :)

If Torque / Texen practice is to mix the properties there, then this seems a
perfectly reasonable approach, as 'getContent()' while intended to return
content to be presented in the rendered output, is just text-ish stuff from
the resource path that won't be parsed.

>I thought it made things
> a lot cleaner, but I was under the assumption that properties files
> wouldn't be coming in from places not already available to the resource
> loader(s).

Again, that makes sense to me too if that's what people normally do with
Texen/Torque.
 
> Perhaps I'm missing some, but the only location properties could be that
> perhaps works in the current PropertiesUtil but not the patch is if
> they're referred to with a full path (e.g. c:\offinanotherdir) or
> relative paths that back out of the dir the file resource loader is
> pointed at (e.g. ..\..\..\clearbackhere).

And that stinks :)
 
> Yeah, and I just reviewed FileResourceLoader and it looks like your
> specifically trying to prevent that with the StringUtils.normalizePath
> stuff.
> 
> So, assuming this will break lots of Texen code (?), what if after
> trying Velocity's resource loader, if a ResourceNotFoundException occurs
> from the RuntimeSingleton.getContent, I just in some code that tries
> loading the properties file off the hard drive?
> 
> If the properties file was in the classpath, template path, or any other
> resource, it would have been found by Velocity, so it'd only have to try
> loading with the path as is and not worry about calculating any template
> path stuff.

That's not true - Velocity don't dig around anywhere other than where the
configured loaders look, and by default that's the file loader in the
current directory.

Just so you know where I'm coming from, my only concern is that this doesn't
break anyone, and second, it's not going to be a bear to maintain in the
future.  After that, if Texen users like it, I like it.

Another approach is to try to find the properties file in the classpath via
using the classloader, or letting it be set in some system property.  Is
that better/ more conventional/safer/ more elegant?


-- 
Geir Magnusson Jr. 
Research & Development, Adeptra Inc.
geirm@adeptra.com
+1-203-247-1713



--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>

RE: [patch] cleaner PropertiesUtil

[Stephen Haberman <st...@chase3000.com>]

> 
> I didn't look at the patch - but doesn't this mean you'll have to put
the
> properties file in path accessible to the resource loader?
> 

Yes. I'm not aware of the best practices/current style of where to put
properties files, but Torque has them mixed in with the templates. Well,
and the test bed naturally has the properties files mixed in with the
templates also.

Hm. Shoot. So I suppose you're saying it's not good to force a resource
loader to be used in loading a properties file? I thought it made things
a lot cleaner, but I was under the assumption that properties files
wouldn't be coming in from places not already available to the resource
loader(s).

Perhaps I'm missing some, but the only location properties could be that
perhaps works in the current PropertiesUtil but not the patch is if
they're referred to with a full path (e.g. c:\offinanotherdir) or
relative paths that back out of the dir the file resource loader is
pointed at (e.g. ..\..\..\clearbackhere). 

Yeah, and I just reviewed FileResourceLoader and it looks like your
specifically trying to prevent that with the StringUtils.normalizePath
stuff.

So, assuming this will break lots of Texen code (?), what if after
trying Velocity's resource loader, if a ResourceNotFoundException occurs
from the RuntimeSingleton.getContent, I just in some code that tries
loading the properties file off the hard drive?

If the properties file was in the classpath, template path, or any other
resource, it would have been found by Velocity, so it'd only have to try
loading with the path as is and not worry about calculating any template
path stuff.

Thanks,
Stephen


--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>

Re: [patch] cleaner PropertiesUtil

["Geir Magnusson Jr." <ge...@adeptra.com>]

On 7/13/02 5:20 PM, "Stephen Haberman" <st...@chase3000.com> wrote:

> Hello,
> 
> Torque uses Texen's PropertiesUtil object and it was giving us problems
> in how it guessed where to load files from (e.g. relying on just
> templatePath being null to insinuate that useClasspath=false).
> 
> So, instead of relying on ad hoc mechanisms for guessing where to load a
> properties file from, I updated PropertiesUtil to instead call
> RuntimeSingleton.getContent and put the resulting text into a Properties
> object.
> 
> The patched PropertiesUtil passes all of the unit tests (test-texen and
> test-texen-classpath) along with correctly handling for the deprecated
> $generator.templatePath being in the propertiesFile parameter. And it
> also gets rid of the messy logic on guessing where to load the file from
> and instead relies on Velocity's existing resource management
> infrastructure.
> 
> Does this look okay to commit? I think it works great, and it passes the
> tests, but if there are any objections or changes I should make, just
> let me know.

I didn't look at the patch - but doesn't this mean you'll have to put the
properties file in path accessible to the resource loader?


-- 
Geir Magnusson Jr. 
Research & Development, Adeptra Inc.
geirm@adeptra.com
+1-203-247-1713



--
To unsubscribe, e-mail:   <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>