collective.recipe.filestorage needs to lowercase "storage" ids

Ah, this was the bug my comment on https://bugs.launchpad.net/zope2/+bug/287550 should have applied to. For a buildout recipe it is a very bad idea trying to change the configured values in any way. It should pass them on, in exactly the way you specified them. If the underlying machinery has a problem it should be fixed there.

If you used this patch the following can happen:

1. You specify the storage in mixed case
2. You use this recipe in version A which lower-cases the value automatically
3. Some month pass by
4. You upgrade to a new minor version of Zope
5. Your database connection doesn't work anymore, as someone fixed the problem in Zope

Step 4 here is bad. With your patch an operation that is perceived to be safe, suddenly has a risk to it, which is not documented in the update notes for Zope itself.

The easiest approach for you is to change your storage names to be all lower-case in the first place.

On October 22, 2008 14:57:11 Hanno Schlichting wrote:
> Ah, this was the bug my comment on
> https://bugs.launchpad.net/zope2/+bug/287550 should have applied to. For
> a buildout recipe it is a very bad idea trying to change the configured
> values in any way. It should pass them on, in exactly the way you
> specified them. If the underlying machinery has a problem it should be
> fixed there.
>
> If you used this patch the following can happen:
>
> 1. You specify the storage in mixed case
> 2. You use this recipe in version A which lower-cases the value
> automatically 3. Some month pass by
> 4. You upgrade to a new minor version of Zope
> 5. Your database connection doesn't work anymore, as someone fixed the
> problem in Zope
>
> Step 4 here is bad. With your patch an operation that is perceived to be
> safe, suddenly has a risk to it, which is not documented in the update
> notes for Zope itself.

I'm not seeing that risk. imo, it actually makes more sense for
collective.recipe.storage to just give the storages incremental numbers - the
way we always have, but if zope was fixed to prevent the error it wouldn't
introduce new problems. As it is, my patch _is_ error prone - if I provided
a name "ABC" and another named "abc", they'd get the same storage name.
Fixing zope so that "ABC" was either illegal or it actually worked, wouldn't
break anything - because my zope.conf file would still say "storage abc".
>
> The easiest approach for you is to change your storage names to be all
> lower-case in the first place.

No, that would be _correct_ but far from easiest. :-) It's a Plone site with
9 different storages at this point,
--
derek

Derek, you can use a lowercase part name with collective.recipe.filestorage and then override each part to specify a camel case Data.fs name and mountpoint. For instance, in one of our buildouts we have:

[filestorage]
recipe = collective.recipe.filestorage
parts =
    mystorage

[filestorage_mystorage]
location = var/filestorage/MyStorage/MyStorage.fs
zodb-mountpoint = /MyStorage

You may have overlooked this ability to override settings for a particular storage part. We had to do this to handle some filestorages that we were migrating from the pre-buildout era.

I'm not likely to change the recipe to automatically lowercase the storage name, for the reason Hanno detailed and because of the collision problem that you brought up. But I will probably add some checking to raise an error if the storage name being generated is mixed case.

I didn't overlook that ability, it just seemed that if I need a new part for every storage, I'm not gaining much over specifying all the storages in "zope-conf-additional". So put it down to laziness :-)

If you don't want to lowercase the names (which I understand), an option to provide sequential numeric storage names (starting at 2, obviously) would be nice, and consistent with typical usage.

fwiw:
[filestorage]
 recipe = collective.recipe.filestorage
 parts =
     MyStorage
[filestorage_MyStorage]
zeo-storage = mystorage

seems more concise :-)