Optimization to only reindex cmf_uid after a UID is set

[davisagli]

In 2008 I made a change to make sure that new UIDs are indexed by calling the reindexObject method on objects that have one. But I didn't specify that only the cmf_uid index should be reindexed, so this can end up doing unnecessary work.

(In practice these days it often doesn't matter thanks to CMF's catalog indexing queue optimization. If the same object is also indexed elsewhere in the same transaction before a query is performed, those operations are collapsed. But there are some cases where a UID is added in a transaction that doesn't include other indexing, or where the indexing queue is processed in between adding a UID and other reindexing. In those cases unnecessary work was performed.)

To fix this I'm now specifying to only reindex the cmf_uid index.

This is a slightly backwards incompatible change in case someone implemented a reindexObject method without an idxs keyword argument. I searched the zopefoundation, plone, and collective github organizations and only found one implementation like that, in plone.app.discussion, which I can adjust. ICatalogAware defines reindexObject with the idxs kwarg, so implementations without it do not comply with the interface.

@mauritsvanrees What is the best way to test this change against Plone?

[jensens]

LGTM

[mauritsvanrees]

@mauritsvanrees What is the best way to test this change against Plone?

In the future, if you really want to test it against Plone before merging, you can login on Jenkins and start PR jobs, with the url of this PR in the parameters textarea. That should work for any repository that somehow used by buildout.coredev, I guess even if no tests are run for a particular package.

[davisagli]

@mauritsvanrees I actually tried that, but it broke because jenkins does not have permission to update PRs in this repository:

Could not update Pull Request https://github.com/zopefoundation/Products.CMFUid/pull/21
Please contact the testing team: https://github.com/orgs/plone/teams/testing-team
Fill an issue as well: https://github.com/plone/jenkins.plone.org/issues/new

[dataflake]

Out of curiosity, I don't know much about the Plone build and test mechanisms and I don't advocate for Jenkins to make changes to anything in the zopefoundation organization, but what is it trying to do?

[davisagli]

@dataflake I'm not 100% sure but I think it is probably either trying to add a comment to the pull request with information about the build, or updating commit status. I agree it does not make sense to give Plone Jenkins write permission to zopefoundation repositories. More likely the jenkins build configuration needs to be adjusted so that it ignores failures to write to repositories outside the plone organization.

[dataflake]

Maybe it's possible to enable this on a per-repository basis.

[mauritsvanrees]

The code that throws the error is in this script. It tries to add a "pending" status to the checks of this PR, which is then not allowed.

David started this job. The job indeed stops executing, throwing that error. See the console output. (Currently very hard to read due to some styling issue. The 'Reader View' in Safari or other browsers may help.)

I will open an issue.