morphia icon indicating copy to clipboard operation
morphia copied to clipboard

Allow to disable the Discriminators by default

Open Javabien opened this issue 5 years ago • 4 comments
trafficstars

Is your feature request related to a problem? I don't see the need to store discriminators by default, it just adds more data to the document, so it has an impact on the IO + storage. The only use case I see for this feature, is when we have to deal with polymorphism, which is not required for every project.

Describe the solution you'd like In the MapperOptions.Builder, we can already configure the discriminator key & discriminator function, why not also include a method enableDiscrimitatorByDefault(boolean). This way we could easily override this value only when there's some polymorphism involve in the class we are designing.

Describe alternatives you've considered No

Additional context I'm not an MongoDB expert, so I don't know if the discriminator feature is use for other things, would this have a performance impact?! (I doubt) Unfortunately, the documentation I found was not very clear about what a discriminator was used for, so I would also recommend to include a brief explanation here: https://morphia.dev/2.0.0/guides/configuration/#discriminator-keys-and-values In google, I found more information about the discriminator in the csharp driver page: https://mongodb.github.io/mongo-csharp-driver/2.0/reference/bson/mapping/polymorphism/ I understand this is a common feature in Mongo no matter which language we are using on the client side, but it would be nice to still explain not only the how, but the why of the discriminator within Morphia.

Javabien avatar Jul 10 '20 16:07 Javabien

In our project, we don't need or plan to use inheritance, so I wanted to find a way to disable this feature, I created this in Kotlin:

        MapperOptions.builder()
            .discriminator(object: DiscriminatorFunction() {
                override fun compute(builder: EntityModelBuilder<*>): String {
                    // If we don't disable it, it would store the class name in each document, while this feature
                    // is only for discriminating sub-classes when the persisted entity has inheritance
                    throw Exception("Missing @Entity(useDiscriminator = false) on ${builder.type}")
                }
            })
            .build()

Unfortunately, it doesn't work, because even when we disable the discriminator for a class, the code still get invoked by MorphiaDefaultsConvention.apply(final Datastore datastore, final EntityModelBuilder<?> modelBuilder): https://github.com/MorphiaOrg/morphia/blob/r2.0.0/morphia/src/main/java/dev/morphia/mapping/MorphiaDefaultsConvention.java#L56 ... which invokes: compute(builder); I think this should clearly avoided, if the flag is set to false, why would we still compute the name?

Javabien avatar Jul 10 '20 17:07 Javabien

the discriminators are used heavily for polymorphic containers (Lists, Sets, Maps, etc.) and polymorphic collections and queries. Given 2.0's ability to define custom (read: small) discriminator values, I'm actually leaning toward removing the ability to disable them altogether. But that's a big reach and I'm not sure it's that practical. But the ability to disable them has caused bugs in the past and complicated the implementation details. I'm not a huge fan of that ambiguity in the mapping set up from an implementation perspective.

evanchooly avatar Jul 10 '20 17:07 evanchooly

the discriminators are used heavily for polymorphic containers (Lists, Sets, Maps, etc.) and polymorphic collections and queries. Given 2.0's ability to define custom (read: small) discriminator values, I'm actually leaning toward removing the ability to disable them altogether. But that's a big reach and I'm not sure it's that practical. But the ability to disable them has caused bugs in the past and complicated the implementation details. I'm not a huge fan of that ambiguity in the mapping set up from an implementation perspective.

still not get the point why we have useDiscriminator true for default. Just upgrade to v2.1.4 and find out that I have to set useDiscriminator = false on every @Entity

can you give us some real examples that explain why such discriminator is so important?

chenchenyangll avatar Feb 01 '21 15:02 chenchenyangll

@chenchenyangll I believe it's simply to discriminate on entities being extended:

class Child1 extends Parent
class Child2 extends Parent

So here the Parent could be an entity linked to a document in MongoDB and this flag will help to deserialize to the corresponding Child1 or Child2.

Javabien avatar Feb 06 '21 22:02 Javabien