Fix embedded document inheritance

[lafrech]

I'm afraid there's an issue with embedded document inheritance when deserializing from DB: the _cls attribute is ignored. Looks like it only works when loading from outside world, not from DB.

This dirty fix seems to do the trick in my use case, but it is certainly not the right implementation. It is mostly a copy-paste from _deserialize to _deserialize_from_mongo. It should at least be factorized.

I'm not sure the instance should be created using to_use_cls(**value). It didn't work for me (complained about an unknown field name due to a dump_only field), so I randomly tried with build_from_mongo and it happened to work...

I think I had this case covered in my PR since I overrode build_from_mongo in EmbeddedDocumentImplementation to pick the right class. But I'm not sure this is relevant to your implementation. I basically copied inheritance related stuff from Document to EmbeddedDocument, while you addressed inheritance in EmbeddedField, AFAIU.

BTW, isn't there a way in EmbeddedField to get embedded_document_cls at init time (when declaring the field in the schema)?

[coveralls]

Coverage decreased (-0.3%) to 95.506% when pulling 6d72bc6 on Nobatek:dev_fix_embedded_doc_inheritance into 26ff4b8 on Scille:master.

[coveralls]

Coverage decreased (-0.3%) to 95.466% when pulling a1861fd on Nobatek:dev_fix_embedded_doc_inheritance into 26ff4b8 on Scille:master.

[lafrech]

I'm still struggling with embedded document inheritance.

I don't understand why your implementation calls from_mongo stuff when deserializing from OO world. I'm not sure this is wanted, or a consequence of the use of super()._deserialize the line above. In fact, I don't understand those two lines.

When modifying them as I did (see new commit), I get both my use case and your tests to pass.

I realize this is a pretty crappy bug report... It's still a bit blurry to me and I was not able to isolate a broken use case I could provide to illustrate the issue I had.

If you're sure about your implementation, then there's definitely something I need to understand. Otherwise, I hope this will help. And I'm available to talk about it.

[coveralls]

Coverage decreased (-0.08%) to 95.482% when pulling 7753be4 on Nobatek:dev_fix_embedded_doc_inheritance into c09cc8a on Scille:master.

[lafrech]

I just pushed an update to my workaround branch and I supposed you were notified, so I might as well comment on this.

It is just a rebase to latest master. It is still hackish. Just a workaround that seems to work for me.

Anyway the core issue (with failing test) is in #62.

Unless I'm mistaken, this is a major fail in the EmbeddedField inheritance feature.

[touilleMan]

e5cd0bb should have fixed this, thx for the work anyway 👍

[lafrech]

I'm still having issues with this even with latest revision from master.

I think it comes from the call to self._deserialize_from_mongo in self._deserialize in EmbeddedField (here).

I really don't understand why we'd call _deserialize_from_mongo with data from the OO world.

The line before,

data, errors = self.schema.load(value, partial=True)

returns OO stuff. There can be Implementation classes in there (think embedded doc in embedded doc). Not just data.

Why not just call

TheRightClassSelectedUsingCls(**data)

(See updated branch. I can't reopen this PR and since it is closed, the new diff was not computed by GitHub.)

If I get the time and manage to do it, I'll propose a failing test.

[touilleMan]

Yes, please provide a testcase ^^

[lafrech]

Reopening so that GitHub tracks my rebases and updates the diffs.

Did you get any time to look into the test case I pushed (#67)?

[coveralls]

Coverage decreased (-0.05%) to 95.548% when pulling c0ee027 on Nobatek:dev_fix_embedded_doc_inheritance into 65c8e01 on Scille:master.

[coveralls]

Coverage decreased (-0.05%) to 95.548% when pulling c0ee027 on Nobatek:dev_fix_embedded_doc_inheritance into 65c8e01 on Scille:master.

[coveralls]

Coverage decreased (-0.05%) to 95.548% when pulling e34daf7 on Nobatek:dev_fix_embedded_doc_inheritance into 65c8e01 on Scille:master.

[lafrech]

Fixed in #84.