Ensure Operations uses the supplied document class when creating BsonDocumentWrapper

[rozza]

JAVA-5349

[rozza]

Made abstract so that can test Operations changes via both sync and async paths

[jyemin]

From the Jira ticket

This can lead to loss of discriminator information with bson-kotlinx.

Does the same problem happen with PojoCodec?

[rozza]

Does the same problem happen with PojoCodec?

Discriminators in the PojoCodec are configured by annotations so its most likely inherited with Pojos.

[rozza]

I'm concerned this could have an unintended consequences on application code.

It may be better to ensure that bson-kotlinx handles the lookup for polymorphic classes.

[robbamberg]

I'm concerned this could have an unintended consequences on application code.

It may be better to ensure that bson-kotlinx handles the lookup for polymorphic classes.

How could bson-kotlinx fix this? The driver is now calling the serializer for the data class instead of for the interface/collection class, so it is the driver doing it wrong? It is now impossible to have a document collection that is polymorphic. Kotlinx would also need to know the interface to which to serialize to. So it is either the default behaviour change (as your fix is doing) or we need a way to provided the interface during write somehow (e.g. json encode requires the interface present to work json.encode(document).

[rozza]

@robbamberg thanks again for the feedback :) I'm still considering the best way forward. I think the fix in this ticket is the correct and matches the probable original intent of the code.

However, I'm trying to think if any user may rely on that current behaviour as it has been there with the driver for some time. PojoCodecs possibly the largest custom mappings inherit discriminator annotations so wouldn't be impacted and as such the risk may be small and worth fixing.

For Kotlin there is also another ticket: JAVA-5338 - when using Updates.set("field", myPolyDataClass) it also doesn't include the discriminator. bson-kotlinx could ensure it checks for polymorphic / sealed implementations and check the serializers. Similar to how KMongo's serialization repository worked.

That could fix both usecases, however, it could easily break existing bson-kotlinx users. It depends if there are many users that use MongoCollection<Cat> and don't want to store the discriminator but when using MongoCollection<Pet> they want discriminator support. Any thoughts?

Also can you elaborate on the json.encode example? I'm not sure I fully understand or it could be the above fixes it.

[robbamberg]

That could fix both usecases, however, it could easily break existing bson-kotlinx users. It depends if there are many users that use MongoCollection<Cat> and don't want to store the discriminator but when using MongoCollection<Pet> they want discriminator support. Any thoughts?

@rozza I can imagine you could want a Cat collection without the discriminator field, so breaking that flow feels risky for me as well.
So I prefer your current fix, as that is very clear that it is currently not working as intended. If I insert a Cat in a Pet collection I always need a discriminator, else I would have just created a Cat collection. Especially when during deserializing the collection class is taking into account, so would make sense to do it on serializing as well.

[rozza]

@jyemin - this is ready for review. I think this is the correct behavior and is of low risk.

I had thought that I just change the Codec for bson-kotlinx but that would be higher risk and limit control for users. JAVA-5338 will require more thought on how best to fix for kotlinx users.

[rozza]

I tested this code fix with the Pojos and the PojoCodec and found that the behavior remains the same - although somethings surprised me and / or I had forgotten.

Interface:

@BsonDiscriminator
public interface Pet {
    String getType();
}

public class Cat implements Pet {

    private String name;
    // ...
}

Findings:

• MongoCollection#insertOne(cat) Succeeds: But Discriminator field NOT stored.
• MongoCollection#insertMany(cats) Succeeds: But Discriminator field NOT stored.
• MongoCollection#find() fails: Cannot find a public constructor for 'Pet'. Please ensure the class has a public, empty constructor with no arguments, or else a constructor with a BsonCreator annotation

Interface 2:

public interface Pet {
    String getType();
}

@BsonDiscriminator
public class Cat implements Pet {

    private String name;
    // ...
}

Findings:

• MongoCollection#insertOne(cat) Succeeds: Has discriminator.
• MongoCollection#insertMany(cats) Succeeds: Has discriminator.
• MongoCollection#find() fails: Cannot find a public constructor for 'Pet'. Please ensure the class has a public, empty constructor with no arguments, or else a constructor with a BsonCreator annotation

Abstract

@BsonDiscriminator
public abstract class Pet {
    abstract String getType();
}

public class Cat extends Pet {

    private String name;
    // ...
}

Findings:

• Regardless of this change it doesn't work
• MongoCollection#insertOne(cat) Fails: Can't find a codec for CodecCacheKey{clazz=class tour.Pet, types=null}.
• Fails regardless of the change as getCodec is called to check for collectible codec in both paths.

Extends

@BsonDiscriminator
public class Pet {
    public Pet() {
    }

    public String getType() {
        return "PET";
    }
}

public class Cat extends Pet {

    private String name;
    // ...
}

Findings:

• MongoCollection#insertOne(cat) Succeeds: Has discriminator.
• MongoCollection#insertMany(cats) Succeeds: Has discriminator.
• MongoCollection#find() Succeeds

[jyemin]

I tested this code fix with the Pojos and the PojoCodec and found that the behavior remains the same - although somethings surprised me and / or I had forgotten.

Good to know. Does this change anything about what we want to do with this PR, or suggest additional work that we should track going forward?

[rozza]

Added JAVA-5370 and JAVA-5371 to track the PojoCodec issues.

[jyemin]

Seems like this is good to merge then.

LGTM