Support values of inner docs given as AttrDict instances

[miguelgrinberg]

Fixes #3075.

It appears that some users pass AttrDict instances as values for fields of type Object or Nested. If the document is validated (the default when save() is called) then these are handled well, but some users also call save(validate=False) or just to_dict(), in which case the AttrDicts cause the error described in #3075. This change adds logic that tolerates AttrDict values, and treats them like regular dicts (which are already tolerated).

[isra17]

Why not just isinstance(data, AttrDict)?

[miguelgrinberg]

Because this would also include support for other types that are not AttrDict. And because it avoids evaluating the conditional for every field that is saved, so it should perform a bit better.

[isra17]

Because this would also include support for other types that are not AttrDict.

Which would probably lead to more crash anyway?

And because it avoids evaluating the conditional for every field that is saved, so it should perform a bit better.

Handling an exception will cost many time what an isinstance cost. But does a few nano seconds performance of this even matter here 🤔 Just seems an unneeded "complex" structure flow to hide a very simple intent.

[miguelgrinberg]

The exception would be handled only for the fields that have a wrong type. The conditional would have to be evaluated for every single field.

[isra17]

And to my point, does it even matter in this case? Elasticsearch-dsl serialization/deserialization code is clearly not designed with performance in mind, so I don't really see why this one isinstance is seen as a potential issue at the cost of readability. But that's just my 2 cents. (and my last comment on this :) )

[isra17]

Actually, fwiw, I suppose your method is consistent with Field._safe_serialize

[miguelgrinberg]

@isra17 Thanks for the discussion and the insights you provided.

I'm going to do some additional testing, and if you don't mind, I'm also going to bring the unit test that you created in your PR, which is different than mine.

As far as the exception vs. no exception method for preventing this error it's really not a big deal, but I have seen other methods that do an instance check like you suggested, so maybe that wins in terms of consistency. But I'm not sure if that is enough. I don't know yet if we need a similar fix for AttrList instances in Nested fields, which inherit from Object and use the same serialize/deserialize logic.

[isra17]

@isra17 Thanks for the discussion and the insights you provided.

Thanks for looking at the issue! I understand this is a bit of a niche error, so it's nice to see it fixed :)

I'm going to do some additional testing, and if you don't mind, I'm also going to bring the unit test that you created in your PR, which is different than mine.

Of course!

I don't know yet if we need a similar fix for AttrList instances in Nested fields, which inherit from Object and use the same serialize/deserialize logic.

For what it's worth, I think if AttrList would have been an issue we would have seen it in the past since the [de]serialize method never dealt with list? Isn't any fields can be a list anyway? My guess is that to_dict on the document take care of unrolling list? Seems like AttrDict.to_dict does, but I can't find how DocumentBase does it 🤔

[isra17]

Ha, Field.serialize deals with the lists.

[pquentin]

Thanks! LGTM.