Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for a callable title on objects in ZODB. #108

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

sallner
Copy link
Contributor

@sallner sallner commented Nov 28, 2022

During the serialization of forms from Products.Formulator an error occurred as in some cases the title of an object was a callable.

@sallner sallner marked this pull request as ready for review November 28, 2022 16:15
@viktordick
Copy link
Collaborator

viktordick commented Nov 28, 2022

I did not know that objects with non-plain titles could exist. I guess if such objects need to be serialized and also played back into the ZODB, I would rather try to access the data that determines the output of the title function instead of calling it - if it is a function and could depend on different data behind the scenes, simply recording the result of the function will not give enough information to restore the object later on.
That being said, recording such objects should of course not fail and at least give an unsupported dummy. But I think I would prefer to simply omit the key meta['title'] in that case rather than setting it to the result of the callable.

What do you think @sallner? Would it be sufficient for your use case if the resulting serialization simply had no title field?

On another note: We try to still support Python 2 with this package (still have some systems with Zope 2 in the field and zodbsync is a tool we use when upgrading old systems containing some custom code to a current code base). It seems that mock is not part of the standard library in Python 2 (at least on my archlinux, running the tests for Python 2 complained that mock could not be imported), so it should be added to .toxfiles/reqs-py2.txt.

EDIT: Just saw that you have already added the dependency. I guess my tox call used the cached environment and did not update to fetch this dependency.

@sallner
Copy link
Contributor Author

sallner commented Dec 2, 2022

I think that it would be an option to have the meta['title'] omitted. I can imagine, that a meta['custom_title'] in read() and write() could be an option for the current problem in my situation.

For the reference: The problem occurred while serializing Fields in Forms of Products.Formulator.

Would you like to implement a solution for the omission of meta['title']?

@viktordick
Copy link
Collaborator

If following my suggestion, you could simply change the code snippet to

    title = getattr(obj, 'title', None)
    # see comment in helpers.py:str_repr for why we convert to string
    if isinstance(title, (six.binary_type, six.text_type)):
        meta['title'] = to_string(title)

The tests also needs to be adjusted, of course.

I am not sure what you are trying to achieve. zodbsync is designed to serialize objects as fully as possible to allow a ZODB that contains page templates, python scripts, document templates and similar object types to be stored on the file system for git workflows and transfering between systems. In particular, as far as I understand, our use case is somewhat special in that we (at PerFact) use the ZODB primarily as code storage and not for holding transactional data (we use a PostgreSQL database for that).

This means that zodbsync has a specific list of object types that it supports and is also able to play back (see object_types.py). I have no problem in accepting additional supported types and there is probably only partial support for some of the types that we do support, because we might not use some feature (compare #87, which documents an incomplete support for access rules, but I never came around to fix this because we do not actually use them and there is no demand for it that I know of).

Currently, any object that we do now support is recorded with a dummy meta file that contains the name of the meta type and an unsupported key. It also until now always contained the title because I never encountered an object type that had no POD title until now, but if this assumption was wrong all this time, I have no problem with omitting the title.

I am a little bit opposed to simply evaluating the title if it is a callable. If you need full support for Fields from Formulator, you would need to add a corresponding handler in object_types.py, which parses obj.tales or whatever actually determines the output of obj.title() - that way a playback of the serialized object back into the ZODB could also be achieved. As of now, only a bare meta file with unsupported marker is created and my guess is that it does not make much of a difference for you if this file contains a title key or not.

My reason for favoring the omission of the title key altogether in such cases is historical. There have been multiple instances where we used a convenience function at first, but then switched to reading out the underlying data directly due to inconsistencies between playback and record or between what the watcher recorded without having the full acquisition path to the object available and what a record returned (for example, this or this).

@sallner
Copy link
Contributor Author

sallner commented Dec 14, 2022

I am happy with your proposed solution and will implement it in this PR and adapt the tests.

The general structure of my current project reflects yours in the sense that code is stored in the ZODB. The forms form the formulator are also treated as code in the sense, that they are archived and versioned with git.

I am aware of object_types.py and have in fact already created some classes for our use cases. I changed also all of the other fields in the __meta__ file so that we can dump and load them in completely. The title was just the only one I could not modify with the help of a ModObj subclass.

@icemac
Copy link
Contributor

icemac commented Sep 28, 2023

@viktordick I implemented your suggestions, could you please review?

Copy link
Collaborator

@viktordick viktordick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except that this has a small conflict with #116, but I'll rebase that one afterwards.

@viktordick viktordick merged commit 3bae333 into perfact:master Sep 29, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants