Unable to get resources containing an embedded document with a ReferencedListField

[bendemboski]

With the following documents:

from mongoengine import *

class Referred(Document):
    something = StringField()

class Embedded(EmbeddedDocument):
    refs = ListField(ReferenceField(Referred))

class TopLevel(Document):
    embedded = EmbeddedDocumentField(Embedded)

class TopLevelList(Document):
    embedded = ListField(EmbeddedDocumentField(Embedded))

and the following resources:

from tastypie_mongoengine.resources import MongoEngineResource, fields

class ReferredResource(MongoEngineResource):
    class Meta:
        list_allowed_methods = ('get',)
        detail_allowed_methods = ('get',)
        object_class = Referred

class EmbeddedResource(MongoEngineResource):
    refs = fields.ReferencedListField(ReferredResource, 'refs')

    class Meta:
        list_allowed_methods = ('get',)
        detail_allowed_methods = ('get',)
        object_class = Embedded

class TopLevelResource(MongoEngineResource):
    embedded = fields.EmbeddedDocumentField(EmbeddedResource, 'embedded')

    class Meta:
        list_allowed_methods = ('get',)
        detail_allowed_methods = ('get',)
        object_class = TopLevel

class TopLevelListResource(MongoEngineResource):
    embedded = fields.EmbeddedListField(EmbeddedResource, 'embedded', full=True)

    class Meta:
        list_allowed_methods = ('get',)
        detail_allowed_methods = ('get',)
        object_class = TopLevelList

getting a list of TopLevelResources throws this exception:

  File "/.../site-packages/django/core/handlers/base.py", line 115, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/.../site-packages/django/views/decorators/csrf.py", line 77, in wrapped_view
    return view_func(*args, **kwargs)
  File "/.../site-packages/tastypie/resources.py", line 217, in wrapper
    response = callback(request, *args, **kwargs)
  File "/.../site-packages/tastypie/resources.py", line 459, in dispatch_list
    return self.dispatch('list', request, **kwargs)
  File "/.../site-packages/tastypie_mongoengine/resources.py", line 374, in dispatch
    return super(MongoEngineResource, self).dispatch(request_type, request, **kwargs)
  File "/.../site-packages/tastypie/resources.py", line 491, in dispatch
    response = method(request, **kwargs)
  File "/.../site-packages/tastypie/resources.py", line 1310, in get_list
    bundles.append(self.full_dehydrate(bundle, for_list=True))
  File "/.../site-packages/tastypie_mongoengine/resources.py", line 415, in full_dehydrate
    return super(MongoEngineResource, self).full_dehydrate(bundle, for_list)
  File "/.../site-packages/tastypie/resources.py", line 865, in full_dehydrate
    bundle.data[field_name] = field_object.dehydrate(bundle, for_list=for_list)
  File "/.../site-packages/tastypie/fields.py", line 729, in dehydrate
    return self.dehydrate_related(fk_bundle, self.fk_resource, for_list=for_list)
  File "/.../site-packages/tastypie/fields.py", line 565, in dehydrate_related
    return related_resource.full_dehydrate(bundle)
  File "/.../site-packages/tastypie_mongoengine/resources.py", line 415, in full_dehydrate
    return super(MongoEngineResource, self).full_dehydrate(bundle, for_list)
  File "/.../site-packages/tastypie/resources.py", line 865, in full_dehydrate
    bundle.data[field_name] = field_object.dehydrate(bundle, for_list=for_list)
  File "/.../site-packages/tastypie_mongoengine/fields.py", line 255, in dehydrate
    if not bundle.obj or not bundle.obj.pk:
AttributeError: 'Embedded' object has no attribute 'pk'

and getting a list of TopLevelListResources returns a 400, throwing this exception: https://github.com/mitar/django-tastypie-mongoengine/blob/master/tastypie_mongoengine/fields.py#L261 because bundle.obj.pk is 0.

I'm not sure why the check (https://github.com/mitar/django-tastypie-mongoengine/blob/master/tastypie_mongoengine/fields.py#L259) checks for bundle.obj.pk, but in the case of an embedded document it's clearly not correct. If I remove it, and only check for bundle.obj everything appears to work, but I'm worried it will mess up some case that I'm not testing...

[mitar]

Can you run tests and see what happens?

What exactly is your expected behavior? How should the URL look like for your resources?

[bendemboski]

As far as the URL for my resources, I assume you mean the contents of the refs ReferencedListField? I'd just expect them to be the resource URLs of the ReferredResources (e.g. /api/referredresource/51faa46e40c6915fe3569294/) just like they would be if refs were a member of TopLevelResource or TopLevelListResource.

I removed the check for bundle.obj.pk (just leaving the check for bundle.obj) and all the tests pass (except the one that fails because of https://github.com/MongoEngine/mongoengine/issues/381). That code appears to be copied and pasted from tastypie's ToManyField.dehydrate() implementation, so I tried removing the pk check there and I get the following unit test failure:

ERROR: test_dehydrate (core.tests.fields.ToManyFieldTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/django-tastypie/tests/core/tests/fields.py", line 1034, in test_dehydrate
    field_1.dehydrate(bundle_1)
  File "/tmp/django-tastypie/tastypie/fields.py", line 796, in dehydrate
    the_m2ms = getattr(the_m2ms, attr, None)
  File "/home/bend/.virtualenvs/test/local/lib/python2.7/site-packages/django/db/models/fields/related.py", line 840, in __get__
    through=self.related.field.rel.through,
  File "/home/bend/.virtualenvs/test/local/lib/python2.7/site-packages/django/db/models/fields/related.py", line 586, in __init__
    (instance, source_field_name))
ValueError: "<Note: >" needs to have a value for field "note" before this many-to-many relationship can be used.

So it looks like this check is supposed to preempt the case where you are trying to dehydrate a ToMany relationship before the source object has been saved (which obviously will not work with a relational DB). However, in our case this doesn't really matter because "ToMany" relationships are just lists, rather than join tables or back references, and it doesn't matter if the object has been saved or not. So I think it's safe to remove this check. If you agree, I'll put it into a pull request.

[mitar]

So please be using MongoEngine 0.8.1 for tests.

It seems we had a similar discussion in #16 as well. Can you check that and see how does it compare to what you are thinking here? See also my fix e6e982bf48cc228aeb74efbc7190f67887660342 at that time.

However, in our case this doesn't really matter because "ToMany" relationships are just lists, rather than join tables or back references, and it doesn't matter if the object has been saved or not. So I think it's safe to remove this check. If you agree, I'll put it into a pull request.

I am still not sure about that. Why we are not catching this problem already with current tests? We do have tests for embedded lists? So what is different here?

[bendemboski]

16 fixes an issue with an EmbeddedListField in an embedded document. This addresses essentially the same issue (using essentially the same fix) with a ReferencedListField in an embedded document.

My explanation of why the pk check is not needed basically comes down to a fundamental difference between relational DBs and Mongo. In a relational DB if you have a to-many relationship, it's either a one-to-many relationship where, for example, BlogPost objects store foreign keys back to the "source" Blog object, or it's a many-to-many relationship where, for example, there is a join table with foreign keys to Publication objects and to Article objects creating the relationships. In either case, the primary key of the source object (Blog or Publication) needs to be known when saving the related objects (BlogPosts or Articles). However, in Mongo that's not the case because in both cases you would just implement the relationship as a list field, which you can set on a document whether or not it already has a pk. So, even though this check is needed when hydrating relational ToManyFields, it is not needed when hydrating ReferencedListFields (or EmbeddedListFields).

I wrote some unit tests that demonstrate this issue, and also a thornier issue around object creation, in this branch: https://github.com/bendemboski/django-tastypie-mongoengine/tree/EmbeddedDocRefListUnitTests.

I implemented my proposed fix (just removing the pk check, like in #16) in this branch: https://github.com/bendemboski/django-tastypie-mongoengine/tree/EmbeddedDocRefListPartialFix.

You can see that my fix partially addresses this bug, as the __get tests pass in the PartialFix branch, but not in the UnitTests branch. Unfortunately, the two _create tests fail and that is a thornier issue that I'm not exactly sure how to solve. The problem is that tastypie's ToManyField's hydrate() implementation is a no-op because it's supposed to be covered later by a save_m2m call, but during the save_m2m phase there is no logic to notice m2m fields inside_ embedded documents. This is only a problem for ReferencedListFields and not EmbeddedListFields because EmbeddedListField can and does override hydrate() to do the m2m hydration on the spot. I think this probably isn't possible for ReferencedListFields because the related objects would need to be saved first (in case they are being created as part of the request), so they really do need to be hydrated later during the m2m phase.

So, I'm not sure how to proceed here. Perhaps rewrite this bug to be just about getting/dehydration, submit a pull request from the Fix branch with the two failing unit tests marked to skip, then file a new bug about creating/updating/hydration?

[mitar]

16 fixes an issue with an EmbeddedListField in an embedded document. This addresses essentially the same issue (using essentially the same fix) with a ReferencedListField in an embedded document.

Be careful. Fix in #16 was not applied! See e6e982bf48cc228aeb74efbc7190f67887660342 for what was applied at the end and after quite some testing. Regrettably, I don't remember anymore exactly all rationale behind things, but I hope comments explain things enough.

So I would prefer if you can check that commit and see if it can something similar be applied to embedded documents as well. I must admit that I have not yet look in depth into the issue you are reporting because I didn't have enough free time yet. I am just trying to point you to things I think they might help you fix the problem yourself in a correct way.

But I still don't understand why list of embedded documents work. But list of embedded documents with list of referenced documents do not. Can you make test where you remove that list from the embedded document and see if it still fail? Or instead of a list, have just a normal reference?

My explanation of why the pk check is not needed basically comes down to a fundamental difference between relational DBs and Mongo.

But why it then works for when there is no list of referenced documents? (Sorry for asking you those questions but I would like to be able to understand the issue without me having to check the code myself.)

Unfortunately, the two _create tests fail and that is a thornier issue that I'm not exactly sure how to solve. The problem is that tastypie's ToManyField's hydrate() implementation is a no-op because it's supposed to be covered later by a save_m2m call, but during the save_m2m phase there is no logic to notice m2m fields inside embedded documents.

How does this work in Tastypie itself? If you have for example two nested models with m2m in between?

Perhaps rewrite this bug to be just about getting/dehydration, submit a pull request from the Fix branch with the two failing unit tests marked to skip, then file a new bug about creating/updating/hydration?

This is always a good approach. :-)

[bendemboski]

Okay, I checked out https://github.com/mitar/django-tastypie-mongoengine/commit/e6e982bf48cc228aeb74efbc7190f67887660342 and it applies fixes to the meta classes as well as to EmbeddedListField.dehydrate(). I believe the fixes in the meta classes apply to ReferencedListFields in the same way as they apply to EmbeddedListFields, so there is nothing there to "port" to my fix. The fix to EmbeddedListField is pretty much the same as my fix to ReferencedListField (it asserts whereas I conditionally throw an exception), but I made another commit to https://github.com/bendemboski/django-tastypie-mongoengine/tree/EmbeddedDocRefListPartialFix changing my fix so it's the same as the existing EmbeddedListField fix. So now the two fixes really are the same.

But I still don't understand why list of embedded documents work. But list of embedded documents with list of referenced documents do not.

The bug is in the ReferencedListField class, so it only repros when a ReferencedListField is present. So the real question here is why it repros when a ReferencedListField is a member of an embedded document, but not when it's a member of a top-level document. The answer is that embedded documents don't have pk's like top-level documents do, but ReferencedListField.dehydrate() was requiring that its object (bundle.obj) always has a pk. Exact same issue as in #16 and exactly the same fix, just on a different field class (in fact, I'm pretty sure the code for the two different dehydrate() methods was copied from the same place -- ToManyField.dehydrate()).

How does this work in Tastypie itself? If you have for example two nested models with m2m in between?

I'm not sure, and I don't know if I'll have time right now to dig into this. My fix will allow retrieval of documents with referenced lists inside of embedded documents to work, but supporting creation/update of such documents is harder. So, as we discussed, how about I tweak this bug to just represent the retrieval side, submit a pull request from my branch to fix it, and then file another bug focusing on creation/update?

If that sounds like a plan, then how do you want to handle the failing unit tests? Probably not a good idea to check in unit tests that are known to fail, right? Maybe just disable them with @skip decorators?

[mitar]

Yes. Make a pull request just for the retrieval part with tests and no tests for creation/update. Then start another pull request where you just provide those failing test for creation/update.

Thanks.

[bendemboski]

Updated this issue to be specific to getting resources -- another one will track creating/updating.

[mitar]

Where is pull request for this then?