id matching in DocumentHandler

[leotiger]

Hi.

I'm working with this a bit and from my understanding the id check inside the save function of documentHandler.php does not conform to the definition of what an _id is internally for arangoDb: a document path based on the collection plus path separator plus the _key.

Starting with line 366 of the current version we find this:

if ($id !== $document->getId()) {
            throw new ClientException('Got an invalid response from the server');
}

A few lines before we find this to generate the value for the $id variable:

$id = UrlHelper::getDocumentIdFromLocation($location);

At least in my environment this leads to problems as $id delivers the _key and $document->getId() includes the the collection name so we have a comparison of

     if (some_key !==  collection/some_key) {
         throw an error;
     }

I would propose to make this fail-safe or by doing something like this:

if ($id !== $document->getId()) {
            if ($id !== $document->getKey()) {
                 throw new ClientException('Got an invalid response from the server');
           }
}

or better to change the whole flow:

In document.php

    /**
     * Get the document id (if already known)
     *
     * Document ids are not!!!! generated on the server only but they follow
     * a fixed pattern combining collection id with document key. 
     * Document ids use _keys that are numeric and might be
     * bigger than PHP_INT_MAX. To reliably store a document id elsewhere,
     * a PHP string should be used
     *
     * @return mixed - document id, might be NULL if document does not yet have an id
     */
    public function getId()
    {
        return $this->_id;
    } 

in documentHandler.php

        // $id will be equivalent for arangodb _key and not for arangodb _id
        $id = UrlHelper::getDocumentIdFromLocation($location);
        $document->setInternalId($json[$_documentClass::ENTRY_ID]);
        $document->setRevision($json[$_documentClass::ENTRY_REV]);
        // $id holds the equivalent for arangodb _key and not for arangodb _id
        if ($id !== $document->getKey()) {
            throw new ClientException('Got an invalid response from the server');
        }

Regards. u

[jsteemann]

The confusion may arise from the fact that Document::getId() for historical reasons only returns the ArangoDB _key part of a document whereas Document::getHandle() returns ArangoDB's _id.

But as you are getting exceptions, can you let me know in which case the failure occurs inside DocumentHandler::save()? I.e. what client code does trigger the exception?

[leotiger]

For sure, it's for abstractions done, but it's inconsistent and I don't see why arango-php should propagate this. There's no obvious reason. I worked with some abstractions of the document class that included a getId function and I had a hard time to figure out what was going wrong. arangodb-php has a getKey function and it should use it.

[leotiger]

Or to put it simple: key should always refer to arangodb _key and id should reflect arangodb _id. That's what the arango domain defines and the code should not introduce unnecessary confusion.

[leotiger]

Some background: I adapted an application that included entity definitions and I simply chained these definitions to extend the document class. It's common to find a getId function inside of entity definitions and while not elegant I put an underscore in front of id where I should have deleted the function together with a big hint in the documentation stating: Don't use Document::getId to get the arangoDb _id! You see, the confusion is in arangodb-php and it's more elegant to fix the internal confusion. As you are switching the namespace you could probably fix this as well to get rid of an historic inconsistency.

[frankmayer]

I think we should fix that in devel for 3.2. What do you think @jsteemann ? And of course also make a big note of that in the changelog...

[leotiger]

I'm working with a copy of the development version and this works now.