Discriminator added to all Queries when Polymorphism is not enabled

[dfjones]

Describe the bug
Morphia 2.4 appears to add the discriminator to all queries even when polymorphism is not enabled on the mapper.

To Reproduce
Create any query for an object using a condition other than the @ID field.
Inspect the resulting query document and see that the discriminator has automatically been added as a criteria, for example:

Document{{field=value, discriminatorKey=Document{{$in=[com.example.ClassName]}}}}

Expected behavior
The discriminator is not automatically added as a parameter, unless polymorphism is enabled.

** Please complete the following information: **

• Server Version: 4.0.20
• Driver Version: 4.11.1
• Morphia Version: 2.4.9

[dfjones]

Update, I implemented a super simple test for this in my repo: dfjones#1

Here's the whole test:

package dev.morphia.test.mapping;

import static dev.morphia.query.filters.Filters.eq;

import org.bson.Document;
import org.testng.Assert;
import org.testng.annotations.Test;

import dev.morphia.Datastore;
import dev.morphia.annotations.Entity;
import dev.morphia.annotations.Id;
import dev.morphia.mapping.DiscriminatorFunction;
import dev.morphia.query.Query;
import dev.morphia.test.TestBase;

@SuppressWarnings("DataFlowIssue")
public class TestQueryDiscriminator extends TestBase {

    public TestQueryDiscriminator() {
        super(
            buildConfig(TestEntity.class)
                .discriminator(DiscriminatorFunction.className())
                .discriminatorKey("className")
                .enablePolymorphicQueries(false)
        );
    }

    @Test
    public void testQueryDoesNotContainDiscriminator() {
        Datastore datastore = getDs();
        Query<TestEntity> q = datastore.createQuery(TestEntity.class);
        q.filter(eq("something", "foo"));
        Document doc = q.toDocument();
        System.out.println(doc);
        Assert.assertFalse(doc.containsKey("className"), "The className field is not found in the query document");
    }

    interface TestInterface {

    }

    @Entity
    static class TestEntity implements TestInterface {
        @Id
        String id;

        String something;
    }
}

I also took another look at the code and I can see that the mapper is always adding the discriminator for every query, unless that query has disabled it in the annotation: @Entity(useDiscriminator = false).

From, MorphiaQuery:

Document getQueryDocument() {
        if (invalid != null) {
            throw invalid;
        }
        try {
            DocumentWriter writer = new DocumentWriter(mapper.getConfig(), seedQuery);
            document(writer, () -> {
                EncoderContext context = EncoderContext.builder().build();
                for (Filter filter : filters) {
                    filter.encode(datastore, writer, context);
                }
            });

            Document query = writer.getDocument();
            if (mapper.isMappable(getEntityClass())) {
                mapper.updateQueryWithDiscriminators(mapper.getEntityModel(getEntityClass()), query);
            }

            return query;
        } catch (ValidationException e) {
            invalid = e;
            throw e;
        }

    }

Note mapper.updateQueryWithDiscriminators(mapper.getEntityModel(getEntityClass()), query); is called for every mappable entity.

Then, in Mapper:

public void updateQueryWithDiscriminators(EntityModel model, Document query) {
        Entity annotation = model.getEntityAnnotation();
        if (annotation != null && annotation.useDiscriminator()
                && !query.containsKey("_id")
                && !query.containsKey(model.getDiscriminatorKey())) {
            List<String> values = new ArrayList<>();
            values.add(model.getDiscriminator());
            if (config.enablePolymorphicQueries()) {
                for (EntityModel subtype : model.getSubtypes()) {
                    values.add(subtype.getDiscriminator());
                }
            }
            query.put(model.getDiscriminatorKey(),
                    new Document("$in", values));
        }
    }

And here the discriminator is added to every query, unless it's a query by _id or the discriminator has been disabled by annotation.

[dfjones]

I found that the LegacyQuery also adds the discriminator using similar logic:

 @Override
    @MorphiaInternal
    public Document toDocument() {
        final Document query = getQueryDocument();
        EntityModel model = datastore.getMapper().getEntityModel(getEntityClass());
        Entity entityAnnotation = model.getEntityAnnotation();
        if (entityAnnotation != null && entityAnnotation.useDiscriminator()
                && !query.containsKey("_id")
                && !query.containsKey(model.getDiscriminatorKey())) {

            List<String> values = new ArrayList<>();
            values.add(model.getDiscriminator());
            for (EntityModel subtype : datastore.getMapper().getEntityModel(getEntityClass()).getSubtypes()) {
                values.add(subtype.getDiscriminator());
            }
            query.put(model.getDiscriminatorKey(),
                    new Document("$in", values));
        }
        return query;
    }

[evanchooly]

Where did this one end up, @dfjones ?

[danielmerchant]

Where did this one end up, @dfjones ?

Hi @evanchooly , @dfjones is no longer working on our team's Morphia upgrade. Do you have any updates here?

[evanchooly]

I don't have an update, per se, I just wanted to check that this issue hadn't already been resolved via another effort. There was a flurry of discussions and issues and patches and wasn't sure if this was still a thing for you there. I just wanted to confirm before starting to dig in to it a bit more.

[danielmerchant]

This is still important to us long-term. FWIW I think the more immediate priority for me is #2936 - if there's a simple solution that you had in mind, I'm happy to submit a patch to fix.

[evanchooly]

So I'm finally digging in to this and this is actually how it should work. If you don't want the discriminators you can set useDiscriminator = false on your @Entity annotation. usePolymorphicQueries determines whether the discriminators of subtypes get included in the query to pick those subtypes with the query.

[danielmerchant]

So I'm finally digging in to this and this is actually how it should work. If you don't want the discriminators you can set useDiscriminator = false on your @Entity annotation. usePolymorphicQueries determines whether the discriminators of subtypes get included in the query to pick those subtypes with the query.

Is the expectation that developers are aware that the discriminator is appended to queries? I think this could be a performance issue as it means that by default, you'll have to include the discriminator in every index on the collection to make use of covered queries.

[evanchooly]

That's a fair point actually. We're up against a few conflicting goals and I'm not sure I have the data to make the decision myself. Historically, useDiscriminator has just applied to including that value in the persisted document. With the advent of polymorphic queries, that job gets a bit blurrier. In an effort to preserve existing behavior where a query against a type would bring only that type, lines were drawn and perhaps they were drawn in the wrong places.

I think polymorphic queries is/should be the default, expected behavior but I didn't want to change the behavior of existing queries and thus that flag. If Morphia were to assume that useDiscriminator implies polymorphism, then things become a bit simpler but that could lead to some unexpected results on existing queries.

It's entirely possible that I'm overthinking it and polymorphism should be the default and then discriminators would only be used when useDiscriminator is true or there are multiple types mapped to a given collection. And maybe that should be the outcome of this discussion. That change would simplify a number of things but I don't want to make that change without some discussion. Thoughts? Perhaps the question should go in to a distinct issue/discussion for visibility. What do you think?

[danielmerchant]

I think the last suggestion you made makes sense - that discriminators should only be included in queries if the flag is true, or if there are multiple entities mapped to the same collection. We can move this conversation to separate issue as well, if you want greater visibility around proposed changes.