Add class_factory parameter to DB and ZConfig class-factory option - #421
Conversation
b41f86c to
38e9f83
Compare
|
Hello @dataflake and @icemac , I used "ask for review" on this pull request. The explanation of the problem is on the linked issue. If something is not clear I can explain more. Thanks ! |
|
I have not looked at this because I simply do not have in-depth knowledge of this code or a use case where I would need it. |
|
It would make more sense if you asked people who actually know this code better. |
There was a problem hiding this comment.
Pull request overview
This PR makes the database class factory configurable at DB construction time and via ZConfig, so pooled connections created during DB.__init__() consistently use the intended class resolver (important for Zope’s broken/uninstalled object handling).
Changes:
- Add
class_factoryparameter toZODB.DB.DB.__init__()and apply it before opening any initial connection. - Add
class-factoryoption to the ZConfig<zodb>schema and wire it through toZODB.DB(...). - Add doctests validating the constructor parameter and ZConfig option plumbing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/ZODB/DB.py |
Adds class_factory parameter and sets classFactory before initial connection/root verification. |
src/ZODB/config.py |
Adds a ZConfig datatype (importable_name) and passes class_factory through when opening DB from config. |
src/ZODB/component.xml |
Extends the <zodb> schema with a class-factory key. |
src/ZODB/tests/testDB.py |
Adds doctest coverage for class_factory being applied to the pooled init connection. |
src/ZODB/tests/testConfig.py |
Adds doctest coverage for the new class-factory ZConfig option. |
CHANGES.rst |
Documents the new constructor parameter and ZConfig option for the upcoming release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
That's an excellent use case for Copilot review. I didn't even think about it because I never use AI. |
461388e to
6ab9bee
Compare
DB.__init__() opens a connection to verify the root object exists, and that connection is returned to the pool for future reuse. Connections capture the classFactory reference at creation time (in Connection.__init__ via ObjectReader), so setting db.classFactory after DB.__init__() does not apply to connections already in the pool. This is a problem for Zope, which overrides DB.classFactory after construction to provide graceful handling of broken/uninstalled objects. The first connection handed out after startup may silently use the wrong class factory. Add a class_factory parameter to DB.__init__() that is set before any connection is opened, and a corresponding class-factory option to the ZConfig database configuration schema. This allows frameworks like Zope to pass the class factory at construction time.
eea614c to
2e5c09d
Compare
|
Thanks for the feedback and thanks for running Copilot. The review suggestions were OK, I think I have addressed them all. Note that at a very high level, this pull request, together with zopefoundation/Zope#1290 just move one configuration from Zope to ZODB, because Zope was configuring it by a monkey patching that was not working anymore. I think that it's a "small, easy" change :) although it was a bit hard to figure out the bug that this was causing. |
dataflake
left a comment
There was a problem hiding this comment.
I'll just say the changes look plausible to me.
|
Thanks @dataflake I just realized there is a potential compatibility issue: a new ZODB version including this change will be incompatible with the current Zope version: Zope would refuse to start with a ZConfig error, because Zope still registers the class_factory key, which is now also registered by ZODB. I think that's not really an issue, because the only supported versions for Zope are the versions listed in https://github.com/zopefoundation/Zope/blob/master/versions-prod.cfg , so we can have incompatible version during development. If that's actually a problem, please let me know, otherwise I'm ready to click merge, but I would need a new ZODB release to continue with zopefoundation/Zope#1290 |
|
Go ahead and merge, I'll do the release. |
|
Thanks a lot |
|
ZODB 6.3 is now published. |
DB.__init__()opens a connection to verify the root object exists, and that connection is returned to the pool for future reuse. Connections capture theclassFactoryreference at creation time (inConnection.__init__viaObjectReader), so settingdb.classFactoryafterDB.__init__()does not apply to connections already in the pool.This is a problem for Zope, which overrides
DB.classFactoryafter construction to provide graceful handling of broken/uninstalled objects. The first connection handed out after startup may silently use the wrong class factory.Add a class_factory parameter to
DB.__init__()that is set before any connection is opened, and a corresponding class-factory option to the ZConfig database configuration schema. This allows frameworks like Zope to pass the class factory at construction time.If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.
It does not fully close but is related to #420