getVersionByLabel() is not correct when called in the same request as addVersionLabel()

[UFTimmy]

If you follow this sequence, you do not get the correct Version. Instead, you get the previous version that held the label:

addVersionLabel
checkin (or by calling notifyHistoryChanged directly)
getVersionByLabel

Performing acts in that sequence will get you the Version that had the label before you moved it. The reason is due to the ObjectManager's in memory caching of the jcr:versionLabels node.

addVersionLabels updates its PHP internal property for where the label is, here: https://github.com/jackalope/jackalope/blob/master/src/Jackalope/Version/VersionHistory.php#L197

At this point, if you call getVersionByLabel, everything is fine. However, if within the same HTTP request, you perform a checkin or otherwise call notifyHistoryChanged, then that PHP internal property for versionLabels is emptied: https://github.com/jackalope/jackalope/blob/master/src/Jackalope/Version/VersionHistory.php#L319

Now the next time you call getVersionByLabel, it needs to now call initVersionLabels again, since the Version state was cleared via notifyHistoryChanged.

The problem is then here, when VersionHistory calls getNode: https://github.com/jackalope/jackalope/blob/master/src/Jackalope/Version/VersionHistory.php#L279

That calls Node:;getNode, which calls ObjectManager's getNodeByPath: https://github.com/jackalope/jackalope/blob/master/src/Jackalope/Node.php#L666

And at this point, the Node in question has already been fetched once, so it's in memory, and thus returned without fetching it fresh: https://github.com/jackalope/jackalope/blob/master/src/Jackalope/ObjectManager.php#L205

The in memory node returned here contains where the label was prior to moving it.

I believe I have found a bug, but I'm not sure of any solutions.

Possible ideas:

• Have the notifyHistoryChanged clear out the in memory cache of ObjectManager for just that one node. But I don't see any methods on ObjectManager to facilitate that.
• Update the node's properties when addVersionLabel is called, so that way the in memory version of it will be correct. Something like this: $node = $this->getNode('jcr:versionLabels'); $node->setProperty($label, $version);
  However, the property is protected, and thus errors, when setProperty is called.

As a work around for me, I am calling clear on the ObjectManager after a checkin, which resets the in memory map of ObjectManager so that the Version history node is fetched fresh next time.

        /** @var Session $session */
        $session = $this->dm->getPhpcrSession();
        $session->getObjectManager()->clear();

But clear is deprecated, so I don't believe this is a durable workaround.

[dbu]

thanks for the detailed analysis!

hm, this is indeed not good. i think we must not remove the node cache from the ObjectManager, as there might still be references to that node. rather, we should update the node internal state.

i did not try it out, but i think we should update the state in the node. there is the Property::_setValue method to change a property from within the implementation that does not do any checks. ah, but the label is adding a new property. there is Node::_setProperty but that one is protected.

i haven't touched any of this in ages. do you see a way where we could internally set that? is the labels node a regular node or a special class like Version? worst case we might need to make Node::_setProperty public or create a new method on Node that is marked as @internal to allow to change the state when we know its right to change it and we just make the node representing the labels reflect whats already happened.

[UFTimmy]

Thanks for the guidance. This bit of code added to the bottom of addVersionLabel takes care of updating any existing labels to their new version and works:

        $node = $this->getNode('jcr:versionLabels');
        foreach ($node->getProperties() as $property) {
            if ($property->getName() === $label) {
                $property->_setValue($version);
                return;
            }
        }

Since the _setValue just updates the new internal state. However, as you said, if it's a new label, then _setProperty is protected, and thus not accessible. Calling $node->setProperty() seems to work as far as updating the internal state correctly, but errors with a 409 "HTTP 409: Unable to perform operation. Node is protected." as soon as you flush.

So I do think the only way to fix this would be expose a method on the Node that just updates the internal state, as you suggested.

Though I think the code snippet above could be added and would not damage anything, and would cover the case of moving labels, making a niche issue even more niche.

[dbu]

but your snippet only works if the label already exists, right? i think if we fix this, we better fix it completely. i wonder if we should add a separate method like setPropertyInternal or make _setProperty public. probably the later, as having two methods that do almost the same will be even more confusing. can you try to do a pull request? ideally with a test in phpcr-api-tests for the scenario?

[UFTimmy]

Sorry for the delay. Yes, my snippet only works for the one case, so I understand not wanting to incorporate it.

I'm afraid that what needs to be done is beyond my limits given the time I have to work on it. I appreciate the time you have spent helping me so far.