INTPYTHON-624 Add PolymorphicEmbeddedModelField

[timgraham]

No description provided.

[WaVEV]

what if multiple submodes has the same field name?

[timgraham]

I called this out in the design doc. I think we'll have to use the system check framework to enforce that common field names at least use the same type, otherwise, we won't know how to prepare the lookup value (e.g. the DatabaseOperations.adapt_<field>_value() methods).

Thinking about it some more, there is also the possibility that nested embedded documents share a field name. In that case, we won't know which field to traverse for the nested lookups that follow.

[Jibola]

there is also the possibility that nested embedded documents share a field name

This would only matter in the case of nested polymorphic fields, right? I think we can maintain that polymorphic fields of the same name should also require the same embedded models to define them? This then keeps us in alignment with the requirement that all name conflicts "are of the same type".

class Colonial(EmbeddedModel):
    ...

class Townhome(EmbeddedModel):
    ...

class Condo(EmbeddedModel):
    ...
    
class AmericanAddress(EmbeddedModel):
    city = models.CharField(...)
    state = models.CharField(...)
    zip_code = models.CharField(...)
    home_specifications = PolymorphicEmbeddedModelField([Colonial, Townhome, Condo],...)
    

class CanadianAddress(EmbeddedModel):
    city = models.CharField(...)
    province = models.CharField(...)
    zip_code = models.CharField(...)
    home_specifications = PolymorphicEmbeddedModelField([Colonial, Townhome, Condo],...)
    
class Home(models.Model):
    address = PolymorphicEmbeddedModelField([AmericanAddress, CanadianAddress], ...)
    num_inhabitants = models.IntegerField(...)

[timgraham]

That seems like a reasonable restriction. Whether it's worth adding a system check for that at this stage, I'm doubtful. There are many other cases where the current system check falls short. For example, the db_type() of EmbeddededModelField and the polymorphic version are both "embeddedDocuments", but there could be an incompatibility there. It just seems like a lot of various cases and combinations to consider, and they are technically okay, except for the querying issue which the user may or may not care about (thus a system check warning rather than an error).

[WaVEV]

This check should run anytime? or only when a migration is applied on the database? 🤔
Given that make migrations does not pass the database, because it should be agnostic to that but when a migration is applied it should not be agnostic. So when do we want to check this? when a migration is created, or anytime?

Edit: change the way that field types are compared is viable?

[timgraham]

It's intended to run at the makemigrations stage in order to flag the issue as early as possible. I think it's better for all database-specific checks to run during makemigrations but this requires changes to Django as discussed in https://code.djangoproject.com/ticket/31286#comment:7.

[Jibola]

I haven't finished looking at the tests, but I've given a look at the code. Overall, I'm in alignment with the approach and am even down to discuss "doubling-down" on embedded model usage. We can weigh the pros and cons outside of this PR.

[Jibola]

there is also the possibility that nested embedded documents share a field name

This would only matter in the case of nested polymorphic fields, right? I think we can maintain that polymorphic fields of the same name should also require the same embedded models to define them? This then keeps us in alignment with the requirement that all name conflicts "are of the same type".

class Colonial(EmbeddedModel):
    ...

class Townhome(EmbeddedModel):
    ...

class Condo(EmbeddedModel):
    ...
    
class AmericanAddress(EmbeddedModel):
    city = models.CharField(...)
    state = models.CharField(...)
    zip_code = models.CharField(...)
    home_specifications = PolymorphicEmbeddedModelField([Colonial, Townhome, Condo],...)
    

class CanadianAddress(EmbeddedModel):
    city = models.CharField(...)
    province = models.CharField(...)
    zip_code = models.CharField(...)
    home_specifications = PolymorphicEmbeddedModelField([Colonial, Townhome, Condo],...)
    
class Home(models.Model):
    address = PolymorphicEmbeddedModelField([AmericanAddress, CanadianAddress], ...)
    num_inhabitants = models.IntegerField(...)

[Jibola]

This could be where we add the warning to try and maintain consistency as best as possible.

[timgraham]

A warning when querying? Probably a system check warning to flag it at model setup time similar to the check I already implemented would be more useful in case the user wants to redefine their models. As I mentioned in the other comment, I'm not sure it's worth doing right now.

[aclark4life]

Just passing by, could this be is_model_instance or is it important to know that the field is considered a "value" to something else ?

[timgraham]

I'm not in love with the name either. It means "the field's value is a model instance." An alternative could be Field.stores_model_instance. It's not a big deal, since it's merely a private API in order to allow the check in django_mongodb_backend/compiler.py to be more generic.

[aclark4life]

Drive by comment. I am not trying to convince anyone of anything, but I would love to use this feature if it were called GenericEmbeddedModelField. Apologies but I just cannot with the Polymorphic… for two reasons:

• What is Polymorphic? I have to look it up every single time.
• Generic is shorter than Polymorphic
• Generic has some context in Django

Unlike my concern about using Embedded… terminology, there may be some justification that Generic could be used instead of Polymorphic and still be appropriate (whereas with Embedded terminology we're purposely steering away from SQL terminology.)

[Jibola]

TL;DR -- Polymorphic is a more holistically accurate representation of this field and avoids confusion.

I'm against the use of GenericEmbeddedModelField for several reasons, but the most pertinent ones are:

1. It's the incorrect usage of the term Generic

   • Our polymorphism (what's commonly viewed as polymorphism) abides by subtype polymorphism which focuses more on fields sharing commonality, whereas Generics abide by parametric polymorphism which emphasize a set of "actions" available to any object/value passed. The contents of that object mean much less than what it can actually do. See the blurb below:

We have already seen that object-oriented languages support subtype polymorphism; generics give us a different kind of polymorphism, parametric polymorphism. Recall that polymorphism means that something can be used at multiple types. Subtype polymorphism allows client code to be written with respect to a single type (as specified by, say, an interface or abstract class) while interoperating with multiple implementations of that type. Parametric polymorphism, also known as generics, solves the opposite problem, in a sense: it allows a single implementation to interoperate with multiple clients who want to use that implementation for different types. This goal is achieved by writing the implementation with respect to a type parameter, a variable ranging over types that can be instantiated with different concrete types. Support for generics is not an essentially object-oriented idea, and was not originally part of Java, having been first introduced in Java 5.

Source

2. It would further confuse folks

   • In both Django Generic Relations and the, now defunct, Django-MongoDB-Engine Generic Embedding "Generics" implement the properties defined in the point above. Rather than a list of explicitly defined items like in our polymorphic design, it takes quite literally anything and will make it work in its system. Were we to also call it a Generic* it would confuse developers because they could expect those qualities instead of realizing things still need to be explicitly typed.

[aclark4life]

Thanks!