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

Allows working copy of the Portal #1823

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Allows working copy of the Portal #1823

wants to merge 3 commits into from

Conversation

wesleybl
Copy link
Member

@wesleybl wesleybl commented Oct 8, 2024

This is part of: plone/volto#5284

Allows working copy services to be accessed in the Portal. Also returns working copy data in the Portal serialization.

In Plone 6.0, this PR should be tested together with the following PRs:

plone/plone.app.iterate#128
plone/Products.CMFEditions#113

In Plone 6.1, the plone.app.iterate PR should be replaced with:

plone/plone.app.iterate#130


📚 Documentation preview 📚: https://plonerestapi--1823.org.readthedocs.build/

@mister-roboto
Copy link

@wesleybl thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@wesleybl
Copy link
Member Author

wesleybl commented Oct 8, 2024

I tried to do a checkout test of the Portal in this class:

class TestWorkingCopyEndpoint(unittest.TestCase):

The test is as follows:

    def test_workingcopy_checkout_checkin_portal(self):
        # We create the working copy
        response = self.api_session.post(
            "/@workingcopy",
        )

        self.assertEqual(response.status_code, 201)

But I am getting error 500. I went to debug and the error is not in the restapi code. But in Zope. The error occurs when trying to execute this commit:

https://github.com/zopefoundation/Zope/blob/5.10/src/ZPublisher/WSGIPublisher.py#L187

The error is a POSKeyError. I executed the commit outside the try and the traceback was:

*** transaction.interfaces.TransactionFailedError: An operation previously failed, with traceback:

  File "/usr/lib/python3.12/threading.py", line 1030, in _bootstrap
    self._bootstrap_inner()
  File "/usr/lib/python3.12/threading.py", line 1073, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.12/threading.py", line 1010, in run
    self._target(*self._args, **self._kwargs)
  File "/home/user/.buildout/eggs/cp312/waitress-2.1.2-py3.12.egg/waitress/task.py", line 84, in handler_thread
    task.service()
  File "/home/user/.buildout/eggs/cp312/waitress-2.1.2-py3.12.egg/waitress/channel.py", line 428, in service
    task.service()
  File "/home/user/.buildout/eggs/cp312/waitress-2.1.2-py3.12.egg/waitress/task.py", line 168, in service
    self.execute()
  File "/home/user/.buildout/eggs/cp312/waitress-2.1.2-py3.12.egg/waitress/task.py", line 434, in execute
    app_iter = self.channel.server.application(environ, start_response)
  File "/home/user/.buildout/eggs/cp312/WebTest-3.0.0-py3.12.egg/webtest/http.py", line 82, in wrapper
    return self.test_app(environ, start_response)
  File "/home/user/.buildout/eggs/cp312/Zope-5.10-py3.12.egg/ZPublisher/httpexceptions.py", line 30, in __call__
    return self.application(environ, start_response)
  File "/home/user/.buildout/eggs/cp312/Zope-5.10-py3.12.egg/ZPublisher/WSGIPublisher.py", line 390, in publish_module
    with transaction_pubevents(request, response):
  File "/usr/lib/python3.12/contextlib.py", line 144, in __exit__
    next(self.gen)
  File "/home/user/.buildout/eggs/cp312/Zope-5.10-py3.12.egg/ZPublisher/WSGIPublisher.py", line 187, in transaction_pubevents
    tm.commit()
  File "/home/user/.buildout/eggs/cp312/transaction-4.0-py3.12.egg/transaction/_manager.py", line 254, in commit
    return self.manager.commit()
  File "/home/user/.buildout/eggs/cp312/transaction-4.0-py3.12.egg/transaction/_manager.py", line 133, in commit
    return self.get().commit()
  File "/home/user/.buildout/eggs/cp312/transaction-4.0-py3.12.egg/transaction/_transaction.py", line 280, in commit
    t, v, tb = self._saveAndGetCommitishError()
  File "/home/user/.buildout/eggs/cp312/transaction-4.0-py3.12.egg/transaction/_transaction.py", line 273, in commit
    self._commitResources()
  File "/home/user/.buildout/eggs/cp312/transaction-4.0-py3.12.egg/transaction/_transaction.py", line 456, in _commitResources
    raise v.with_traceback(tb)
  File "/home/user/.buildout/eggs/cp312/transaction-4.0-py3.12.egg/transaction/_transaction.py", line 430, in _commitResources
    rm.commit(self)
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/Connection.py", line 492, in commit
    self._commit_savepoint(transaction)
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/Connection.py", line 1033, in _commit_savepoint
    obj._p_estimated_size = len(data)
    ^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/git/buildout.coredev/src/Products.CMFPlone/Products/CMFPlone/Portal.py", line 68, in __setattr__
    if self._tree is not None and name in self:
       ^^^^^^^^^^
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/Connection.py", line 787, in setstate
    p, serial = self._storage.load(oid)
                ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/mvccadapter.py", line 164, in load
    r = self._storage.loadBefore(oid, self._start)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/DemoStorage.py", line 229, in loadBefore
    return self.base.loadBefore(oid, tid)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/DemoStorage.py", line 229, in loadBefore
    return self.base.loadBefore(oid, tid)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/DemoStorage.py", line 229, in loadBefore
    return self.base.loadBefore(oid, tid)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  [Previous line repeated 2 more times]
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/utils.py", line 281, in __call__
    return func(*args, **kw)
           ^^^^^^^^^^^^^^^^^
  File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/MappingStorage.py", line 171, in loadBefore
    raise ZODB.POSException.POSKeyError(oid)
ZODB.POSException.POSKeyError: 0x2a33e463be38bb08

When Plone is running normally this error does not occur, only in testing. Could there be some missing test configuration?

@mauritsvanrees @davisagli @d-maurer @dataflake

@d-maurer
Copy link

d-maurer commented Oct 8, 2024 via email

@wesleybl
Copy link
Member Author

wesleybl commented Oct 8, 2024

The oid 0x2a33e463be38bb08 is huge for such a setup.

@d-maurer the functionality I'm implementing clones the portal with pickle. See:

https://github.com/plone/Products.CMFEditions/blob/c80bc31af46bff45fee2908878b1f01190fda8d8/Products/CMFEditions/ArchivistTool.py#L210-L229

Usually the Portal is a large object. Could it be that the test configuration is not supporting the clone size?

In real life, would I have to optimize the portal clone, to clone only the attributes that are really needed?

@d-maurer
Copy link

d-maurer commented Oct 8, 2024 via email

@wesleybl
Copy link
Member Author

wesleybl commented Oct 8, 2024

@d-maurer there is already a treatment to prevent child objects of the Portal from being placed in the clone:

https://github.com/plone/Products.CMFEditions/blob/13fe34186f7294113f5c14bac6e8f78170868569/Products/CMFEditions/StandardModifiers.py#L322-L347

The child objects are replaced by an object of the VersionAwareReference class. So my theory that the clone was getting big because of the child objects is wrong.

Checking the traceback, I noticed that the error occurs in the Portal class, in this line:

https://github.com/plone/Products.CMFPlone/blob/6.0.13/Products/CMFPlone/Portal.py#L68

So I printed self before that line. The result was:

...
<PloneSite at plone>
<PloneSite at plone>
<Products.CMFPlone.Portal.PloneSite object at 0x7f697d4581a0 oid 0x1d86b25a16bc20b5 in <ZODB.Connection.Connection object at 0x7f697d4ddb80>>

In other words, the object that caused the error was this:

<Products.CMFPlone.Portal.PloneSite object at 0x7f697d4581a0 oid 0x1d86b25a16bc20b5 in <ZODB.Connection.Connection object at 0x7f697d4ddb80>>

When we had the object:

<PloneSite at plone>

the error did not occur. Does this give any clue as to what might be causing the error?

@d-maurer
Copy link

d-maurer commented Oct 8, 2024 via email

@d-maurer
Copy link

d-maurer commented Oct 9, 2024

Your use of a DemoStorage (i.e. a RAM based storage) suggests a test setup with few objects. The oid 0x2a33e463be38bb08 is huge for such a setup.

I have been partially wrong: DemoStorage is not a (pure) RAM based storage. Instead it allows to put a RAM based storage (more precisely a MappingStorage) on top of any storage (in particular a non RAM based storage).

Looking more closely at your traceback:
The recursive call DemoStorage.py", line 229, in loadBefore suggests that you have several DemoStorages layered on top of one another. The final traceback line from MappingStorage suggests that you have a MappingStorage at the base of your DemoStorage stack. This would mean that your storage is indeed fully RAM based.

Checking the code, I see that DemoStorage chooses its initial oid randomly: self._next_oid = random.randint(1, 1 << 62). This explains the huge oid value. Using randomness at this point may lead to non-deterministic failures, however, e.g. that the same oid is used for different objects. I suggest you replace line 135 of ZODB.DemoStorage (self._next_oid = random.randint(1, 1 << 62)) by self._next_oid = self.base._next_oid + 0x100000 and check whether this lets your test succeed.

@d-maurer
Copy link

d-maurer commented Oct 9, 2024

I have created zopefoundation/ZODB#401

@wesleybl
Copy link
Member Author

wesleybl commented Oct 9, 2024

I suggest you replace line 135 of ZODB.DemoStorage (self._next_oid = random.randint(1, 1 << 62)) by self._next_oid = self.base._next_oid + 0x100000 and check whether this lets your test succeed.

After changing the line, I get the error:

AttributeError: 'MappingStorage' object has no attribute '_next_oid'. Did you mean: 'new_oid'?

I did a dir on self.base:

['__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getstate__', '__gt__', '__hash__', '__implemented__', '__init__', '__init_subclass__', '__le__', '__len__', '__lt__', '__module__', '__name__', '__ne__', '__new__', '__providedBy__', '__provides__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_commit_lock', '_data', '_last_pack', '_lock', '_ltid', '_oid', '_opened', '_transaction', '_transactions', 'checkCurrentSerialInTransaction', 'cleanup', 'close', 'getName', 'getSize', 'getTid', 'history', 'isReadOnly', 'iterator', 'lastTransaction', 'load', 'loadBefore', 'loadSerial', 'new_oid', 'not_in_transaction', 'opened', 'pack', 'registerDB', 'sortKey', 'store', 'tpc_abort', 'tpc_begin', 'tpc_finish', 'tpc_transaction', 'tpc_vote']

@d-maurer
Copy link

d-maurer commented Oct 9, 2024 via email

@wesleybl
Copy link
Member Author

wesleybl commented Oct 9, 2024

@d-maurer I debugged a bit and found out that this error occurs when we have a savepoint and a Plone Site object is involved. It occurs when the code enters here:

https://github.com/zopefoundation/ZODB/blob/5.8.1/src/ZODB/Connection.py#L486-L494

However, the error only occurs in the test and when there is a portal object involved. Even in the test, this code works, if there is no modified Plone Site object.

In the normal execution of an instance, the code does not enter the if, because self._savepoint_storage is None. self._savepoint_storage is not filled, because we have no resources here:

https://github.com/zopefoundation/transaction/blob/4.0/src/transaction/_transaction.py#L618

So the savepoint is not actually created. But in the test we have resources and the savepoint code is executed.

In fact, if I return after this line, the test works.

Do we have a limitation of DemoStorage running savepoint code when we have a Plone Site involved?

@d-maurer
Copy link

d-maurer commented Oct 9, 2024

File "/home/user/.buildout/eggs/cp312/ZODB-5.8.1-py3.12.egg/ZODB/Connection.py", line 1033, in _commit_savepoint
    obj._p_estimated_size = len(data)
    ^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/git/buildout.coredev/src/Products.CMFPlone/Products/CMFPlone/Portal.py", line 68, in __setattr__
    if self._tree is not None and name in self:
       ^^^^^^^^^^

New hypothesis: The code cited above tries to update the object's size estimation (obj._p_estimated_size = ...). This is an assignment to one of the special persistency attributes (identified by an _p_ name prefix); it should happen without access to any other attributes of obj. But, apparently obj._tree is accessed and results in a ZODB load which apparently fails.

I suggest you try the following:
in Products.CMFPlone.Portal.__setattr__ you replace

        if self._tree is not None and name in self:

by

      if not name.startswith("_p_") and self._tree is not None and name in self:

Apparently the special __setattr__ is there to allow (content) items to be stored in a tree structure (providing support for a huge number of child items). Because the id of content items must not start with _, the startswith("_p_") can be replaced by startswith("_").

@wesleybl
Copy link
Member Author

wesleybl commented Oct 9, 2024

I suggest you try the following:
in Products.CMFPlone.Portal.__setattr__ you replace

   if self._tree is not None and name in self:

by

 if not name.startswith("_p_") and self._tree is not None and name in self:

@d-maurer This worked! Thanks!

I'll try to create a PR to fix this.

wesleybl added a commit to plone/Products.CMFPlone that referenced this pull request Oct 9, 2024
Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)
wesleybl added a commit to plone/Products.CMFPlone that referenced this pull request Oct 9, 2024
Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)
davisagli pushed a commit to plone/Products.CMFPlone that referenced this pull request Oct 16, 2024
…ves (#4026)

Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)

Co-authored-by: Rohan Shaw <[email protected]>
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Oct 16, 2024
Branch: refs/heads/master
Date: 2024-10-15T22:21:08-07:00
Author: Wesley Barroso Lopes (wesleybl) <[email protected]>
Commit: plone/Products.CMFPlone@9a1a62b

Avoid POSKeyError when commit occurs and we have savepoint that involves (#4026)

Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)

Co-authored-by: Rohan Shaw &lt;[email protected]&gt;

Files changed:
A news/4026.bugfix
M Products/CMFPlone/Portal.py
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@wesleybl Could you add a test?

@d-maurer
Copy link

d-maurer commented Oct 30, 2024 via email

@davisagli
Copy link
Member

I meant a functional test that the new REST API endpoints in this PR are working as expected.

@wesleybl
Copy link
Member Author

@davisagli for the test to work, we first need a Plone release with: plone/Products.CMFPlone#4026

I need to backport this to Plone 6.0.

@d-maurer
Copy link

d-maurer commented Oct 30, 2024 via email

@d-maurer
Copy link

d-maurer commented Oct 30, 2024 via email

wesleybl added a commit to plone/Products.CMFPlone that referenced this pull request Nov 4, 2024
Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)
wesleybl added a commit to plone/Products.CMFPlone that referenced this pull request Nov 4, 2024
Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)
@wesleybl
Copy link
Member Author

wesleybl commented Nov 4, 2024

@davisagli Should the github actions tests pass even if we don't have the package versions with the other PRs? Should we expect a new Plone version with the other PRs?

Allows working copy services to be accessed in the Portal. Also returns
working copy data in the Portal serialization.
davisagli pushed a commit to plone/Products.CMFPlone that referenced this pull request Nov 5, 2024
…ves (#4043)

Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Nov 5, 2024
Branch: refs/heads/6.0.x
Date: 2024-11-05T14:27:57-08:00
Author: Wesley Barroso Lopes (wesleybl) <[email protected]>
Commit: plone/Products.CMFPlone@c2b14e3

Avoid POSKeyError when commit occurs and we have savepoint that involves (#4043)

Plone Site

When ZODB handles savepoint and we have changes in Plone Site at that
savepoint, it changes the `_p_estimated_size` attribute of Plone Site.
This is an assignment to one of the special persistency attributes
(identified by an _p_ name prefix); it should happen without access to
any other attributes of obj. But obj._tree is accessed in __setattr__ of
PloneSite class and results in a ZODB load which apparently fails.

See: plone/plone.restapi#1823 (comment)

Files changed:
A news/4043.bugfix
M Products/CMFPlone/Portal.py
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.

4 participants