Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update MongoDB.Driver to 3.0.0 #23

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

micdah
Copy link
Contributor

@micdah micdah commented Nov 8, 2024

In 3.0.0, all of Core has been merged into one, which is why we can drop dependency on MongoDB.Driver.GridFS.

GuidRepresentation has also been altered, so now to get a default Guid representation behaviour, we must register a global GuidSerializer with a selected representation.

Alternatively, a representation must be choosen on a per-field basis using annotations.


Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

In 3.0.0, all of Core has been merged into one, which is why we can drop dependency on `MongoDB.Driver.GridFS`.

`GuidRepresentation` has also been altered, so now to get a default Guid representation behaviour, we must register a global `GuidSerializer` with a selected representation.

Alternatively, a representation must be choosen on a per-field basis using annotations.
E.g. `net6.0`, `netstandard2.1` and `net472`
MongoDB have dropped support for .Net Framework 4.6 and .Net Core 2.x (https://www.mongodb.com/docs/drivers/csharp/upcoming/upgrade/v3/#version-3.0-breaking-changes)
@micdah
Copy link
Contributor Author

micdah commented Nov 12, 2024

Well, there seems to be a potential breaking change here.

Specifically MongoDb.Driver 3.0 requires explicit specification of GuidRepresentation, either thorugh a BsonClassMap of annotations on the classes.

So stuff like BsonValue.Create(object) when given a Guid will now throw, you must use stuff like new BsonBinaryData(Guid, GuidRepresentation).

I tried playing around with just doing that - but it seems that MongoDbSagaStorage is sometimes given a string typed propertyValue for the Id field, where I would have assumed only a Guid would ever be a valid option.

So I'm unsure the best approach to handle this breaking change in Rebus.

We can't just set up a global GuidSerializer as that might interfere with / break customer code as the serializer is global.

We can't just add a BsonClassMap on ISagaData because the MongoDB driver requires the target type to be a class, not an interface. I tried applying it to SagaData just to see, but there were still a lot of places which brokke. Applying to the default implementation would also not work for any users implementing the ISagaData interface directly.

I guess the intended approach from MongoDB.Driver is that it must be made explicit, e.g. users of Rebus.MongoDB needs to ensure a BsonClassMap is registered for all concrete implementations of ISagaData in use - and then decide which GuidRepresentation to use (and we'd need to know that choice somehow, as the Find etc., will need to serialise an appropriate BsonBinaryData to search for) or that annotations are placed on all Guid properties (which would not work if inheriting from SagaData which is missing those).

This is not intended for production - but PoC to see if I can get test suite green.

Here I just assume all clients will be globally using `CSharpLegacy` representation.

In reality, we need to address the now explicit choice of Guid representation that MongoDB.Driver enforces, which allows Guid's to be represented differently, even within the same document.
@micdah
Copy link
Contributor Author

micdah commented Nov 12, 2024

Please note, even though this PR is green - it's not actually production ready.

This just demonstrates that we can upgrade to 3.0, but we need to figure out how to handle GuidRepresentation from now on.

I have just configured it such that we are assuming CSharpLegacy everywhere, which can be done i 3.0, but is not done so by default.

Clients may also mix and match as they see fit, e.g. we could have a ISagaData implementation where _id is using CSharpLegacy but it may also contain other Guid fields which could be using Standard, as such this code especially has issues with working in 3.x

    public async Task<ISagaData> Find(Type sagaDataType, string propertyName, object propertyValue)
    {
        var collection = await GetCollection(sagaDataType);

        var value = propertyValue is Guid guid
            ? new BsonBinaryData(guid, GuidRepresentation.CSharpLegacy)
            : BsonValue.Create(propertyValue);

        var criteria = propertyName == nameof(ISagaData.Id)
            ? new BsonDocument { { "_id", value } }
            : new BsonDocument { { propertyName, value } };

        var result = await collection.Find(criteria).FirstOrDefaultAsync().ConfigureAwait(false);

        if (result == null) return null;

        return (ISagaData)BsonSerializer.Deserialize(result, sagaDataType);
    }

For _id we could enforce CSharpLegacy (for backwards compatibility), but the client may have more Guid fields on the concrete class, which may or may not be using that representation - so we'd need to know that on a per-field basis for the above to work in a general approach.

I think for 3.x, when using the LINQ approach or using Mongo.Driver.Builder it will infer the type from available annotations / class maps, but when we are using BsonDocument (untyped) we explicitly need to use the appropriate binary representation to match the stored document.

@AGiorgetti
Copy link
Contributor

I think that enforcing CSharpLegacy is not a good idea, it will force new project behave in a "bad" way.

I think that the following scenarios should be supported:

  • The users should map all their classes (this is how MongoDB driver is intended to be used anyway) and we should add a warning in the readme pointing to MongoDB changelog; there's no way to overcome this issue because it's a breking change of the driver. If you want to keep compatibility you have to properly map all the guid fields (or change the serializer globally).
  • Rebus Mongodb have to be extended with a configuration settings that let the user decide the "GuidRepresentation" for all the manual bson document generation that happens in the code (like find method you reported above).
  • defaults should be aligned to those of the mongodb driver and we should provide a breaking change section that explains how to configure things to keep compatibility.

@micdah
Copy link
Contributor Author

micdah commented Nov 18, 2024

I think that enforcing CSharpLegacy is not a good idea, it will force new project behave in a "bad" way.

I think that the following scenarios should be supported:

  • The users should map all their classes (this is how MongoDB driver is intended to be used anyway) and we should add a warning in the readme pointing to MongoDB changelog; there's no way to overcome this issue because it's a breking change of the driver. If you want to keep compatibility you have to properly map all the guid fields (or change the serializer globally).
  • Rebus Mongodb have to be extended with a configuration settings that let the user decide the "GuidRepresentation" for all the manual bson document generation that happens in the code (like find method you reported above).
  • defaults should be aligned to those of the mongodb driver and we should provide a breaking change section that explains how to configure things to keep compatibility.

I agree, and in my opinion the first option seems the most "correct" by leaning against what the developers of MongoDB.Driver intends - iff a user then opts to just apply a GuidRepresentation globally, that's fine and will also just work here.

The main pain point of that though, is the MongoDbSagaStorage.Find method I think, as it has to lookup a class map to find the appropriate serialization strategy to use for the given root property of the saga data type.

@micdah
Copy link
Contributor Author

micdah commented Nov 18, 2024

I have updated the PR with a proposed implementation.

This requires the user to setup appropriate BsonClassMap's for all their implementations of saga data.

If they inherit from SagaData, they will need to register a map for this base class:

BsonClassMap.RegisterClassMap<SagaData>(map =>
{
    map.MapIdMember(obj => obj.Id).SetSerializer(new GuidSerializer(GuidRepresentation.Standard));
    map.MapMember(obj => obj.Revision);
});

If their inheriting class also contains Guid's, they will further also have to create a class map for it:

BsonClassMap.RegisterClassMap<MySagaData>(map =>
{
    map.MapMember(obj => obj.MyGuidProperty).SetSerializer(new GuidSerializer(GuidRepresentation.Standard));
});

If they inhertit from ISagaData, they will have to create a class map for their own class including all members of ISagaData (MongoDB driver only allows mapping of defined, not inherited, members - inherited members must be defined in a BsonClassMap for that specific class, which is applied to all classes inheriting for it).

I opted to register a global GuidSerializer in the Rebus test suite, because I could not create class maps for the generic tests such as ConcurrencyHandling<TestMongoDbSagaStorage> as they contain private ISagaData implementations for which we cannot create a BsonClassMap here.

@micdah
Copy link
Contributor Author

micdah commented Nov 25, 2024

@mookid8000 thoughts on this approach?

Would be awesome to get Rebus.MongoDB unblocked for MongoDB.Driver 3.x as we have a pending upgrade internally which is blocked by this package alone 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants