-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix catalog inconsistency when creating a dossier from a dossiertemplate. #6952
base: master
Are you sure you want to change the base?
Conversation
91c132d
to
1130676
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this affect bundle import? The bundle import pipeline also uses this context manager.
To be honest, I'm really not a fan of triggering the processQueue()
upon entering / leaving the context manager. That is an operation that could, at least potentially, take several hours (e.g. during an upgrade) that is just triggered when using a benign sounding context manager, probably wrapped around a very specific piece of code.
Did you explore the option of changing the monkey patch order at all? Adding a ZCML dependency to collective.indexing
in the right place (before our patch) should result in the patch order being reversed, and as far as I understand your comment, that should also fix the issue. But again, would need to be investigaged with bundle import because that also might unpatch the collective.indexing
patches.
I tried changing the Monkey patch order, by importing As for the bundle import, I saw it was also using the context manager. As for whether this was a problem there, I am not sure. I did not want to invest too much time, as this was only half part of the actual issue I was supposed to fix. A safer option could be to patch the Queue itself (https://github.com/plone/collective.indexing/blob/master/src/collective/indexing/queue.py#L95-L105). We can discuss that when I get back to work. |
Ok I've given it some more thoughts and I think the current approach with adding an interface in the context manager cannot work:
Problem with solution 2. is that the call to
I'd find solution 1 cleaner. It also has the advantage that we do not have to check for disabled indexing all the time. |
@njohner I agree. After some thought, I also think solution 1 is better (patching the indexing methods instead of using the request layer). |
Because children are added to the main dossier with disabled indexing, we need to reindex the main dossier after the reinabling indexing.
DeactivatedCatalogIndexing was not working properly because collective.indexing was still adding indexing operations to the indexing queue, and the patched methods were really only used when the queue was processed. This led to issues as whether an object would be indexed or not depended on when the queue was processed instead of when the object was added to the queue. To fix that issue we modify the Monkey patch to always skip indexing when active and modify the context-manager to patch and unpatch the indexing methods instead of adding an interface to the request as was done previously.
1130676
to
99822e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concept looks good, i think there is a serious flaw regarding thread safety in the implementation though. Don't hesitate to ☎️ if you want to discuss this!
Apart from that LGTM 👍 .
@@ -58,38 +49,44 @@ class PatchCMFCatalogAware(MonkeyPatch): | |||
and do it manually at the end of your tasks. | |||
""" | |||
|
|||
original_indexing_methods = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a very bad feeling about this class attribute. It will be shared between threads. Please redesign this a bit, so that we use the same instances of PatchCMFCatalogAware
for patching/unpatching withing one request/thread.
- move creation of the mapping into
__init__
or create lazily in__call__
|
||
|
||
class DeactivatedCatalogIndexing(object): | ||
"""Contextmanager: Deactivates catalog-indexing | ||
""" | ||
def __enter__(self): | ||
alsoProvides(getRequest(), IDisableCatalogIndexing) | ||
PatchCMFCatalogAware()() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't create multiple instances of the monkey patch, instead do
self.catalogpatch = PatchCMFCatalogAware()
(and call it separately)
|
||
def __exit__(self, exc_type, exc_val, exc_tb): | ||
noLongerProvides(getRequest(), IDisableCatalogIndexing) | ||
PatchCMFCatalogAware().unpatch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then use the same instance with the thread-safe mapping to unpatch
self.catalogpatch.unpatch()
When creating a dossier from a dossier template, the children of the main dossier get created with
DeactivatedCatalogIndexing
. This does not work as expected as we Monkey patch the same indexing methods thancollective.indexing
. Because our patch gets applied first, the patch fromcollective.indexing
will still be adding indexing operations to the indexing queue, and our patched methods will only be used when the queue is processed. This is why we need to make sure to process the queue when entering this context manager so that items already in the queue get processed correctly, and before exiting the context manager so that indexing of queued items really gets skipped.Moreover we need to make sure to reindex the main dossier after creation of the child objects (because adding children modifies the container).
This issue was discovered while looking at https://4teamwork.atlassian.net/browse/CA-1361
Checklist
Everything has to be done/checked. Checked but not present means the author deemed it unnecessary.