Fix dereferencing embedded documents refs and dereferencing tests.

[ilex]

What does these changes do:

1. Fix bug with dereference function: it does not dereference refs in embedded documents from list ListField(EmbeddedDoc(ReferenceField(X))). It performs an additional call to db using dereference_id function while accessing the field.
   • Add test to reproduce.
   • Add fix.
2. Fix false positive test test.test_dereference.DereferenceTestCase.test_reference_not_found. It saves an empty Post model which has title as a pk. That forces to store post document with _id as an ObjectId. So post.delete() does not actually delete any documents from database.

[llvtt]

Thanks for the pull request!

These fixes look good to me. Thanks also for the test case.

For (2), I think there's another problem here, too. I think the issue is that there's no ValidationError raised when saving a model whose primary key has not been given, when the primary key isn't implicit. Some examples:

class PostTitlePK(MongoModel):
    title = fields.CharField(primary_key=True)


class PostImplicitPK(MongoModel):
    title = fields.CharField()


# This should raise ValidationError, since we explicitly defined
# `title` as the primary_key, but it hasn't been given a value.
# `title` should be required:
# ValidationError: {'title': ['field is required']}
post = PostTitlePK().save()


# This is ok. There is no primary_key declared on the model, so the
# primary_key is a hidden ObjectIdField, which will be filled in when
# we call save().
post_ok = PostImplicitPK().save()

I think that test_reference_not_found has the right expectation, but it's written incorrectly. Post.save() should actually raise ValidationError as written. The fix for the test is the same as what you've done, though: to give the post a title.

So, I think your fixes all look good, but there's another change needed. We need to set required = True in contribute_to_class for fields when self.primary_key and not cls._mongometa.implicit_id. We should also therefore set _mongometa.implicit_id = True before calling add_to_class on the field when creating the model definition.

Do you agree? Would you like to make this fix as well?

Thank you so much for your time and effort to improve this project!

[ilex]

Yes, I agree. I'll try to make this fix too. Where is the best place to put test for this fix?

[llvtt]

I think test/test_model_basic.py would be a good place. The User model has a primary_key field called fname that could be used for the test.

Also, I wouldn't be surprised if fixing this bug causes other tests to fail, since there are probably other places where we are trying to save documents with an explicit primary_key that don't have a value for it. The solution will be the same: to provide a value for the primary_key field.

[ilex]

Would it be ok if I also add a small fix to make QuerySet._clone reusable in subclasses by replacing:

    clone = QuerySet(model=model or self._model, query=query or self._query)

with

    clone = type(self)(model=model or self._model, query=query or self._query)

?

[llvtt]

Ha! The same fix for clone is also proposed in this pull request: https://github.com/mongodb/pymodm/pull/13/files#diff-36aa0eb7af6d66c2411da533ad499995R63. Go for it. Thanks again for your hard work on this.

[llvtt]

Thank you for fixing this and updating all the tests. Your work here will be part of the forthcoming 0.3.1 release. Thanks again!