Bug 42558

Created attachment 20301 [details]
Patch for enhancement described below

Renaming Lenya 1.4 to 2.0

Created attachment 20569 [details]
Patch for enhancement described below, second version

Just tried another one. I don't know if in loadPublicationPropertiesIfNotDone

- the avalon stuff is handled correctly (does the publication have to be
released?)

- there could be synchronization issues or if the ThreadSafe implementation of
PropertiesModule takes already care of this

In initialize() the properties of all modules are collected during the Cocoon
components initialization phase regardless of the publications referencing them.
These references are probably unknown at this point, but I think this should be
documented because it can lead to property name conflicts between modules. The
same holds true for the publication properties. The way I implemented them a
publication should not use properties of another publication, is not even
guaranteed to see them and name conflicts could occur as well (not to mention
publication templating). 

I see two solutions:

- modules and publications have to use name prefixes

- properties have to be saved (and may only be read) together with an "owner id"

(In reply to comment #4)
> I see two solutions:
> 
> - modules and publications have to use name prefixes
> 
> - properties have to be saved (and may only be read) together with an "owner id"

i think we should make prefixes mandatory: for modules, it should be
<module-name>.your.property. for publications, it could be
<pub-id>.your.property, but i guess we should leave the choice to the admin.
users may also want to overload global properties...

plus we need to find out and document really well in which order properties are
loaded and how to override them.

(In reply to comment #3)
> Created an attachment (id=20569) [edit]
> Patch for enhancement described below, second version
> 
> Just tried another one. I don't know if in loadPublicationPropertiesIfNotDone
> 
> - the avalon stuff is handled correctly (does the publication have to be
> released?)

iiuc, you're not dealing with avalon components directly (PublicationUtil does
it for you). so you should be fine.
 
> - there could be synchronization issues or if the ThreadSafe implementation of
> PropertiesModule takes already care of this
 
not sure about this. someone else should look at it. anyone?

anyway, i think this is a very useful addition, and i'm +1 for adding it.

(In reply to comment #3)
> Created an attachment (id=20569) [edit]
> Patch for enhancement described below, second version
> 
> Just tried another one. I don't know if in loadPublicationPropertiesIfNotDone
> 
> - the avalon stuff is handled correctly (does the publication have to be
> released?)

No, you need to release all components that you looked up with
manager.lookup(...) AFAIR. So it is good as gold.

> 
> - there could be synchronization issues or if the ThreadSafe implementation of
> PropertiesModule takes already care of this

Why do you think of synchronization issues? > 

(In reply to comment #5)
> (In reply to comment #4)
> > I see two solutions:
> > 
> > - modules and publications have to use name prefixes
> > 
> > - properties have to be saved (and may only be read) together with an "owner id"
> 
> i think we should make prefixes mandatory: for modules, it should be
> <module-name>.your.property. for publications, it could be
> <pub-id>.your.property, but i guess we should leave the choice to the admin.
> users may also want to overload global properties...
> 

I am as well for prefixing, additional a check/record of double named properties.

> plus we need to find out and document really well in which order properties are
> loaded and how to override them.

The case of double properties is very common because we implement a fallback
mechanism aka ant properties files. 

Ant properties work the following order:
1. cli (e.g. -Dmodule=a)
2. build files - normally more then one (local.)build.properties. First in wins.

For us it is (before the patch)
a) SystemProperties
b) local.lenya.properties.xml
c) modules
d) lenya.properties.xml

I thought I wrote some documentation about it but cannot find it. 

One thing that I do not like too much is the 
loadPublicationPropertiesIfNotDone
not been done in initialize().

ATM I am unsure how to do it myself maybe Andreas has an idea. 

If not I will apply the patch within the next 72 hours.

(In reply to comment #7)

>> 
>> - there could be synchronization issues or if the ThreadSafe implementation of
>> PropertiesModule takes already care of this
> 
> Why do you think of synchronization issues?

Because on should always think about synchronization issues :)
But at a first glance the module doesn't change its state (or the state of
another object) after the initialization, so this should be fine.

(In reply to comment #10)

> Because on should

That should read "one", sorry.

(In reply to comment #10)
> (In reply to comment #7)
> >> - there could be synchronization issues or if the ThreadSafe implementation of
> >> PropertiesModule takes already care of this
> > 
> > Why do you think of synchronization issues?
> 
> Because on should always think about synchronization issues :)
> But at a first glance the module doesn't change its state (or the state of
> another object) after the initialization, so this should be fine.
> 

(see comment #9)
> One thing that I do not like too much is the 
> loadPublicationPropertiesIfNotDone
> not been done in initialize().

Now I understand the synchronization issues. The module changes state with the
getAtribute!

thorsten, markus: what's the state of this issue? i think it would be nice to
have it in 2.0, since it seems to address a real user need.
markus, if you've got some time: can you describe what you're using this for?
then we can add some documentation once it's in.

Just realized that I made a really stupid mistake: I replied to some of the
above posts on the dev mailing list instead of bugzilla. Really silly. I
apologize for this.

The state of this issue is as follows:

1.0

The publication properties cannot be read in the initialization phase. At least
I don't know how. The approach with

Request request = contextUtility.getRequest();
DocumentFactory factory = DocumentUtil.getDocumentFactory(serviceManager, request);
Publication[] pubs = publicationManager.getPublications(factory);

cannot be used in the initialization phase. You get "Unable to get the request
object from the context" when you try it. Being able to call
publicationManager.getPublications() without the factory parameter could maybe
help, but this doesn't seem to be possible unless some of the recent changes of
Andreas have changed this.

1.1

This leads to the workaround with loadPublicationPropertiesIfNotDone, called
from all get* methods which circumvents the problem by lazy loading the
publication properties.

1.2

This in turn led to the question if this could cause synchronization problems. I
don't think calling loadPublicationPropertiesIfNotDone twice in parallel would
be very dangerous but I am not fit enough to judge this.

2.0

Jörn suggested to (re-)use aggregating fallback for properties of templated
publications.

2.1

I found AggregatingFallbackSourceFactory which provides the protocol called
"aggregate-fallback://". I think to be able to reuse it some of the code of
getSource would have to be refactored. I am not able to do this myself without
help. If anybody with a better understanding of the big picture could have a
look at it and tell me if this is the right way to go i would give it a try.
(There are some similarities in code with FallbackSourceFactory.getSource, so
maybe this would have to be taken into account as well.)

Committed revision 568857.

I as well added a sample to the default pub in revision 568859.

There are still some issues to solve regarding the documentation (we do not have
any).

(In reply to comment #14)
> ...
> 2.0
> 
> Jörn suggested to (re-)use aggregating fallback for properties of templated
> publications.
> 
> 2.1
> 
> I found AggregatingFallbackSourceFactory which provides the protocol called
> "aggregate-fallback://". I think to be able to reuse it some of the code of
> getSource would have to be refactored. I am not able to do this myself without
> help. If anybody with a better understanding of the big picture could have a
> look at it and tell me if this is the right way to go i would give it a try.
> (There are some similarities in code with FallbackSourceFactory.getSource, so
> maybe this would have to be taken into account as well.)

Actually in loadXMLPropertiesFromURI we use a Source to resolve the uri meaning 
you can use loadXMLPropertiesFromURI(filteringProperties,
"aggregate-fallback://"+PROPERTY_NAME, true);

> Actually in loadXMLPropertiesFromURI we use a Source to resolve the uri meaning 
> you can use loadXMLPropertiesFromURI(filteringProperties,
> "aggregate-fallback://"+PROPERTY_NAME, true);

I made an attempt to do that. It works for me so far, but there is the function
PropertiesModule.getAttributeValues which i don't fully understand. I put some
code there but commented it out. Maybe somebody can review it and enlighten
me... Thanks!

Will draft a short doc and put it in the wiki.

Created attachment 21044 [details]
Patch for aggregate-fallback, requires older patch (which is already applied).

I put a documentation draft on the wiki:
http://wiki.apache.org/lenya/LenyaProperties?action=show. It is open for
discussion. Comments and edits welcome.

Any news here ? 
Still required ?