Fix catalog inconsistency when creating a dossier from a dossiertemplate. #6952

njohner · 2021-03-24T16:48:28Z

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 than collective.indexing. Because our patch gets applied first, the patch from collective.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.

Changelog entry
Link to issue (Jira or GitHub) and backlink in issue (Jira)

lukasgraf

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.

njohner · 2021-03-29T20:02:17Z

I tried changing the Monkey patch order, by importing collective.indexing in our patch. This did change the order, but somehow probably led to an infinite loop when creating a dossier from a template (I did not check what was really happening, just that Gever was hanging for ever when creating a dossier from a template). That is why I chose that approach. We can of course try to find a better solution.

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.

njohner · 2021-04-09T10:12:58Z

Ok I've given it some more thoughts and I think the current approach with adding an interface in the context manager cannot work:

If our patch is applied first, the collective.indexing patch second, then indexing an object will still add to the queue, and our patch's effect will be applied when the queue is processed, so that cannot work
If collective.indexing patch is applied first, then ours: in that case things will work as expected. If IDisableCatalogIndexing is provided by the request, indexing operations will do nothing (objects will not land in the queue), but if the queue is processed, the original indexing methods will be applied so the objects in the queue will get correctly indexed.

Problem with solution 2. is that the call to unpatch_collective_indexing will remove both patches, so that our DeactivatedCatalogIndexing context manager will not work anymore. So I see two solutions:

Modify the context manager so that it patches and unpatches the indexing methods instead of adding the IDisableCatalogIndexing to the request.
write our own unpatch method to correctly remove the collective.indexing patches, or reapply our own patch after the unpatch_collective_indexing call.

I'd find solution 1 cleaner. It also has the advantage that we do not have to check for disabled indexing all the time.

lukasgraf · 2021-04-15T14:27:47Z

@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.

deiferni

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 👍 .

deiferni · 2021-04-26T10:05:01Z

opengever/base/monkey/patches/cmf_catalog_aware.py

@@ -58,38 +49,44 @@ class PatchCMFCatalogAware(MonkeyPatch):
    and do it manually at the end of your tasks.
    """

+    original_indexing_methods = {}


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__

deiferni · 2021-04-26T10:06:18Z

opengever/base/monkey/patches/cmf_catalog_aware.py



 class DeactivatedCatalogIndexing(object):
    """Contextmanager: Deactivates catalog-indexing
    """
    def __enter__(self):
-        alsoProvides(getRequest(), IDisableCatalogIndexing)
+        PatchCMFCatalogAware()()


don't create multiple instances of the monkey patch, instead do

self.catalogpatch = PatchCMFCatalogAware() (and call it separately)

deiferni · 2021-04-26T10:06:29Z

opengever/base/monkey/patches/cmf_catalog_aware.py


    def __exit__(self, exc_type, exc_val, exc_tb):
-        noLongerProvides(getRequest(), IDisableCatalogIndexing)
+        PatchCMFCatalogAware().unpatch()


then use the same instance with the thread-safe mapping to unpatch

self.catalogpatch.unpatch()

njohner force-pushed the nj/CA-1361/fix_disabled_indexing branch from 91c132d to 1130676 Compare March 24, 2021 16:51

njohner requested a review from a team March 24, 2021 16:51

lukasgraf reviewed Mar 25, 2021

View reviewed changes

Niklaus Johner added 2 commits April 22, 2021 12:23

Fix indexing of main dossier created from dossiertemplate.

61d0ab1

Because children are added to the main dossier with disabled indexing, we need to reindex the main dossier after the reinabling indexing.

njohner force-pushed the nj/CA-1361/fix_disabled_indexing branch from 1130676 to 99822e8 Compare April 22, 2021 10:35

njohner requested a review from a team April 22, 2021 12:37

deiferni suggested changes Apr 26, 2021

View reviewed changes

Make disable indexing thread-safe.

52841e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix catalog inconsistency when creating a dossier from a dossiertemplate. #6952

Fix catalog inconsistency when creating a dossier from a dossiertemplate. #6952

njohner commented Mar 24, 2021 •

edited

lukasgraf left a comment

njohner commented Mar 29, 2021

njohner commented Apr 9, 2021

lukasgraf commented Apr 15, 2021

deiferni left a comment

deiferni Apr 26, 2021

deiferni Apr 26, 2021

deiferni Apr 26, 2021

Fix catalog inconsistency when creating a dossier from a dossiertemplate. #6952

Are you sure you want to change the base?

Fix catalog inconsistency when creating a dossier from a dossiertemplate. #6952

Conversation

njohner commented Mar 24, 2021 • edited

Checklist

lukasgraf left a comment

Choose a reason for hiding this comment

njohner commented Mar 29, 2021

njohner commented Apr 9, 2021

lukasgraf commented Apr 15, 2021

deiferni left a comment

Choose a reason for hiding this comment

deiferni Apr 26, 2021

Choose a reason for hiding this comment

deiferni Apr 26, 2021

Choose a reason for hiding this comment

deiferni Apr 26, 2021

Choose a reason for hiding this comment

njohner commented Mar 24, 2021 •

edited