Collective zc.buildout recipes

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

Reported by auspex on 2008-10-22
4
Affects Status Importance Assigned to Milestone
collective.buildout
Undecided
Unassigned

Bug Description

When collective.recipe.filestorage builds the "storage" parameter for zope.conf, it defaults the id to %(fs_part_name)s, but Zope doesn't understand storages with ids in mixed case. Strictly, this would be a Zope problem, I'm sure, but lowercasing the name ensures that it works, so I found the attached patch to the recipe to be the simplest workaround for me.

The error for mixed case storage names looks like:
2008-10-20T21:48:45 INFO ZEO.ClientStorage (9697) ClientStorage (pid=9697) created RW/normal for storage: 'CHONe'
------
2008-10-20T21:48:45 INFO ZEO.cache created temporary cache file '<fdopen>'
------
2008-10-20T21:48:45 INFO ZEO.ClientStorage (9697) Testing connection <ManagedClientConnection ('127.0.0.1', 11100)>
------
2008-10-20T21:48:45 INFO ZEO.zrpc.Connection(C) (127.0.0.1:11100) received handshake 'Z303'
------
2008-10-20T21:48:45 INFO ZEO.ClientStorage (9697) Server authentication protocol None
------
2008-10-20T21:48:45 ERROR ZEO.zrpc (9697) CW: error in testConnection (('127.0.0.1', 11100))
Traceback (most recent call last):
  File "/home/Plone-buildout/parts/zope2/lib/python/ZEO/zrpc/client.py", line 454, in test_connection
    self.preferred = self.client.testConnection(self.conn)
  File "/home/Plone-buildout/parts/zope2/lib/python/ZEO/ClientStorage.py", line 442, in testConnection
    stub.register(str(self._storage), self._is_read_only)
  File "/home/Plone-buildout/parts/zope2/lib/python/ZEO/ServerStub.py", line 74, in register
    self.rpc.call('register', storage_name, read_only)
  File "/home/Plone-buildout/parts/zope2/lib/python/ZEO/zrpc/connection.py", line 646, in call
    raise inst # error raised by server
ValueError: unknown storage: CHONe

auspex (auspex) wrote :
Hanno Schlichting (hannosch) 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.

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

David Glick (davisagli) wrote :

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.

auspex (auspex) wrote :

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 :-)

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers