coen-hyde / Shanty-Mongo

Shanty Mongo is a mongodb library for the Zend Framework. Its intention is to make working with mongodb documents as natural and as simple as possible. In particular allowing embedded documents to also have custom document classes.
Other
200 stars 52 forks source link

$id components of DBRef becoming String instead of MongoID #81

Closed tonymillion closed 12 years ago

tonymillion commented 12 years ago

This has happened enough times to warrant logging here, I don't know if the problem is specifically Shanty or in the MongoDriver/DB (I'm running 2.0.2 for reference).

I have documents that have DBRefs in them, and sometimes I've noticed an exception being thrown in shanty when I update the DBRef, invariably when I look at the DB I find the $id component of the DBRef has become a String type instead of a ObjectId (MongoID).

E.g. (this just happened now)

"user": {
    "$ref": "users",
    "$id":"4f60ddde304fbee862000003"
  }

should be

"user": {
    "$ref": "users",
    "$id": ObjectId("4f60ddde304fbee862000003")
  }

As I said I haven't debugged it properly yet, but I'm raising this here incase anyone else has the issue.

tonymillion commented 12 years ago

For reference the exception is thown on line 938 of Mongo/Document.php where it tries to convert the mongoID into a string..

            if (MongoDBRef::isRef($newValue) && MongoDBRef::isRef($oldValue)) {
                $newValue['$id'] = $newValue['$id']->__toString();
                $oldValue['$id'] = $oldValue['$id']->__toString();

(which seems the wrong thing to do maybe?

tholder commented 12 years ago

Hi Tony,

Yes that does look dodgy to me. @coen-hyde might be able to give some insight in to this.

I haven't actually experienced this issue as yet but obviously this is nasty behaviour.

If you do want to attempt to fix it that would be great, will just need a pull request and the tests to be running.

Cheers Tom

coen-hyde commented 12 years ago

Oh nasty. That's strange code I wrote right there .....

By the looks of it the only reason i was converting ObjectId's to strings was for comparison. I don't think I meant to save the ObjectsId's as strings. I'd welcome a fix, a bit busy atm so can't fix it right now sorry.

Cheers, Coen

tonymillion commented 12 years ago

I'm not sure if thats exactly where the problem is occurring, but once it has happened, then the next time thats where the exception is thrown, as apparently string doesn't have a __toString() method….

coen-hyde commented 12 years ago

My bets are that's where it's happening.

tholder commented 12 years ago

Cool, I won't get much time until the weekend to look at it but I'll try and find a condition under which it occurs.

Cheers Tom

tonymillion commented 12 years ago

Yeah I just caught it again - its anytime a DBRef is reassigned to another DBRef (in my case the user that owns an object changed and it happened)

tholder commented 12 years ago

Cheers Tony, that helps. I might try and tackle it this afternoon.

tonymillion commented 12 years ago

I think I may have resolved this, and have to apologise:

I checked out our code and its been modified by from the original code base incorporating some things from @gwagner after he did a fix (from an earlier issue) for DBRefs not returning the right Object type when dereferenced.

for reference the code was

            /* arrays do not work the same way in evaluations */
            if(is_array($oldValue) && is_array($newValue))
            {
                if(json_encode($oldValue) !== json_encode($newValue))
                    $this->addOperation('$set', $property, $newValue);

            }
            elseif ($newValue !== $oldValue)
                $this->addOperation('$set', $property, $value);

(notice the use of newValue in the first addOperation I changed this back to value and it works, and also apparently MongoDBRef's pass the is_array test.)

Anyway I have reverted this change and it appears to have resolved the issue.

tholder commented 12 years ago

Hi Tony,

No worries.

I'm actually going to put a test in for this anyway, as it is obviously bad news for something like this to occur.

Cheers Tom