CSHARP-5201: Allow update to supply sort option #1453

papafe · 2024-09-18T14:21:09Z

No description provided.

BorisDog

Looks good overall! Now we need to wait for spec finalization.

BorisDog · 2024-09-19T00:54:22Z

tests/MongoDB.Driver.Tests/Specifications/crud/CrudTestRunner.cs

@@ -342,7 +342,7 @@ private class TestCaseFactory : JsonDrivenTestCaseFactory
                new[]
                {
                    "MongoDB.Driver.Tests.Specifications.crud.tests.v1",
-                    "MongoDB.Driver.Tests.Specifications.crud.tests.v2"
+                    "MongoDB.Driver.Tests.Specifications.crud.tests.v2"  //TODO I don't think we have v2 anymore?


Good catch, we don't, can be removed.

BorisDog · 2024-09-19T00:55:20Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedTestsDiscoverer.cs

@@ -124,8 +124,8 @@ protected override IEnumerable<JsonDrivenTestCase> CreateTestCases(BsonDocument
                    }

                    var description = testCase.Test["description"].AsString;
-                    var lineNumer = lines.FirstOrDefault(p => p.Line.Contains(description)).Index;
-                    testCase.Test.Add("_lineNumber", lineNumer);
+                    var lineNumber = lines.FirstOrDefault(p => p.Line.Contains(description)).Index;


BorisDog · 2024-09-19T00:56:47Z

tests/MongoDB.Driver.Tests/UnifiedTestOperations/UnifiedUpdateOneOperation.cs

@@ -112,6 +112,7 @@ public UnifiedUpdateOneOperation Build(string targetCollectionId, BsonDocument a
            {
                switch (argument.Name)
                {
+                    //TODO Why don't we create UpdateOptions beforehand?


Probably to exercise options == null path.

BorisDog · 2024-09-19T01:00:50Z

src/MongoDB.Driver/MongoCollectionBase.cs

            };
+            //TODO Why don't UpdateOne and UpdateOneAsync have a common path for creating the UpdateOneModel and BulkWriteOptions (if not more)?


In that case that can be done elegantly, we should do that in a separate ticket for other methods as well. Usually we can address such improvements on green-build Fridays.

BorisDog · 2024-09-19T01:02:23Z

src/MongoDB.Driver/Core/Operations/UpdateRequest.cs

+        /// <summary>
+        /// Gets or sets the sort document.
+        /// </summary>
+        public BsonDocument Sort


Alphabetical order.

src/MongoDB.Driver/BulkWriteReplaceOneModel.cs

src/MongoDB.Driver/BulkWriteUpdateOneModel.cs

sanych-sun · 2024-10-16T16:13:23Z

src/MongoDB.Driver/BulkWriteUpdateOneModel.cs

+        /// <summary>
+        /// The sort document to use.
+        /// </summary>
+        public BsonDocument Sort { get; init; }


Should we use SortDefinition<TDocument>?

sanych-sun · 2024-10-16T16:16:08Z

src/MongoDB.Driver/ReplaceOneModel.cs

@@ -31,6 +31,7 @@ public sealed class ReplaceOneModel<TDocument> : WriteModel<TDocument>
        private BsonValue _hint;
        private bool _isUpsert;
        private readonly TDocument _replacement;
+        private BsonDocument _sort;


Should we use SortDefinition<TDocument>?

sanych-sun · 2024-10-16T16:17:19Z

src/MongoDB.Driver/ReplaceOptions.cs

+        /// <summary>
+        /// Gets or sets the sort document.
+        /// </summary>
+        public BsonDocument Sort


Use SortDefinition<TDocument>.

sanych-sun · 2024-10-16T16:18:12Z

src/MongoDB.Driver/UpdateOneModel.cs

+        /// <summary>
+        /// Gets or sets the sort document.
+        /// </summary>
+        public BsonDocument Sort


Use SortDefinition<TDocument>.

sanych-sun · 2024-10-16T16:18:18Z

src/MongoDB.Driver/UpdateOptions.cs

+        /// <summary>
+        /// Gets or sets the sort document.
+        /// </summary>
+        public BsonDocument Sort


Use SortDefinition<TDocument>.

Unfortunately UpdateOptions and ReplaceOptions are not generic classes, so we can't parametrize the properties. We can add the parameter, but I think it would be a breaking change.
We could also add the generic UpdateOptions<TDocument> alongside the non generic version and make new methods that use this, while deprecating the methods that are using the non generico version. This would be the long road

I have done this only for ReplaceOptions for now. If all I've done makes sense, I'll do the same for UpdateOptions.
This required to create a new class ReplaceOptions<T> and an implicit conversion operator from ReplaceOptions to ReplaceOptions<T>.

sanych-sun

We should decide if we are ready for the breaking change by replacing ReplaceOption with ReplaceOption before reviewing further changes.

sanych-sun · 2024-10-30T21:25:55Z

...ver/Core/WireProtocol/Messages/Encoders/BinaryEncoders/ClientBulkWriteOpsSectionFormatter.cs

@@ -200,6 +201,7 @@ public void RenderUpdateOne<TDocument>(RenderArgs<BsonDocument> renderArgs, Bson
            WriteArrayFilters(serializationContext, model.ArrayFilters);
            WriteHint(serializationContext, model.Hint);
            WriteCollation(serializationContext, model.Collation);
+            WriteSort(serializationContext, model.Sort?.Render(renderArgs.WithNewDocumentType(documentSerializer)));


Is it possible to make WriteSort and pass model.Sort there? And move Render there as well.

sanych-sun · 2024-10-30T21:28:22Z

src/MongoDB.Driver/IMongoCollection.cs

@@ -972,8 +972,7 @@ public interface IMongoCollection<TDocument>
        /// <returns>
        /// The result of the replacement.
        /// </returns>
-        ReplaceOneResult ReplaceOne(FilterDefinition<TDocument> filter, TDocument replacement, ReplaceOptions options = null, CancellationToken cancellationToken = default(CancellationToken));
-
+        ReplaceOneResult ReplaceOne(FilterDefinition<TDocument> filter, TDocument replacement, ReplaceOptions<TDocument> options = null, CancellationToken cancellationToken = default(CancellationToken));


It will be a code breaking change (client's code has to be rewritten) as far as I understood. Should we create a new overload instead? And support both ReplaceOptions and ReplaceOptions<TDocument>

@BorisDog WDYT?

Yup we avoid breaking changes. We'll probably need an extension method.

This looks like a binary breaking change but not a source code breaking change.

There is also an implicit conversion operator from ReplaceOptions to ReplaceOptions<TDocument> that provides backward compatibility.

Yes, I discussed this with Robert and this was an attempt to avoid creating more methods/overloads and keep source compatibility.
I was reading a little bit around online about semver in regards to binary compatibility. It seems that there is not a complete agreement on it, but overall it's recommended to increase major version with a binary-incompatible change.
Personally (irregardless of this case), I think this is a matter of deciding where do we stand on this and then take it from there.

I would prefer a solution that doesn't involve doubling (or worse) the number of overloads. Too many overloads is a bad thing.

The current approach uses an implicit conversion from ReplaceOptions to ReplaceOptions<TDocument> to preserve backward compatibility.

Another approach could be to use subclassing: ReplaceOptions<TDocument> : ReplaceOptions. Then we could leave the current overloads unchanged at the expense of some type safety. The code that uses the replaceOptions would have to test whether the passed in options are the subclass or not.

Given what we discussed at Triage today I'll park this ticket until next when we can have a more general discussion about breaking changes in general

I have to apologize, I did not notice the implicit conversion. So this is not code breaking change.
On other hand I suppose it's better to follow Robert's suggestion with using of subclasses as clearer way to do the same thing. At least because this way we could avoid changes to public API.

I went for Robert suggestion and made the generic options derive from the base class we have now

BorisDog · 2024-10-30T22:30:19Z

src/MongoDB.Driver/ReplaceOptions.cs

+    /// <summary>
+    /// Options for replacing a single document.
+    /// </summary>
+    public sealed class ReplaceOptions : ReplaceOptionsBase


Need to ensure that this change is not breaking.

BorisDog · 2024-10-30T22:51:45Z

src/MongoDB.Driver/IMongoCollection.cs

@@ -972,8 +972,7 @@ public interface IMongoCollection<TDocument>
        /// <returns>
        /// The result of the replacement.
        /// </returns>
-        ReplaceOneResult ReplaceOne(FilterDefinition<TDocument> filter, TDocument replacement, ReplaceOptions options = null, CancellationToken cancellationToken = default(CancellationToken));
-
+        ReplaceOneResult ReplaceOne(FilterDefinition<TDocument> filter, TDocument replacement, ReplaceOptions<TDocument> options = null, CancellationToken cancellationToken = default(CancellationToken));


Yup we avoid breaking changes. We'll probably need an extension method.

papafe · 2024-11-04T14:31:29Z

src/MongoDB.Driver/BulkWriteReplaceOneModel.cs

-            : this(CollectionNamespace.FromFullName(collectionNamespace), filter, replacement, collation, hint, isUpsert)
+            bool isUpsert = false,
+            SortDefinition<TDocument> sort = null)
+            : this(CollectionNamespace.FromFullName(collectionNamespace), filter, replacement, collation, hint, isUpsert, sort)


This is not binary compatible.
The alternative would be to add other two constructors (given that we already have two) but:

Then we have 4 very similar constructors

Developers using those constructors will have errors due to constructor ambiguity if they do not specify all parameters. This is theoretically still source compatible, but definitely annoying
Any suggestions about this? Personally I'd just keep the sort definition out of the constructor if we cannot find another alternative

What if we add new constructor with sort as the very last non-optional parameter? In such way it would not be a breaking change and there would not be an ambiguity.

That's a good idea, I didn't think about it :)
I've added two new constructor to mirror the ones we have already, and added a new private one that is being called by all the others

Nice! And I suppose the plan should be to eliminate this copied ctors in next major release, by making the sort parameter as optional. So the question is: how we going to track this and not forget to do?

Yup, that would be the plan! I've added the constructors and the non generic options to the list of breaking changes for next version.

sanych-sun · 2024-11-06T00:05:45Z

tests/MongoDB.Driver.Tests/IMongoCollectionExtensionsTests.cs

@@ -1132,7 +1132,7 @@ public void ReplaceOne_should_call_collection_with_expected_arguments(

            assertReplaceOne();

-            var replaceOptions = new ReplaceOptions();
+            var replaceOptions = new ReplaceOptions<Person>();


Should we revert this change as we should be backward compatible now?

sanych-sun · 2024-11-06T04:44:45Z

src/MongoDB.Driver/BulkWriteReplaceOneModel.cs

-            : this(CollectionNamespace.FromFullName(collectionNamespace), filter, replacement, collation, hint, isUpsert)
+            bool isUpsert = false,
+            SortDefinition<TDocument> sort = null)
+            : this(CollectionNamespace.FromFullName(collectionNamespace), filter, replacement, collation, hint, isUpsert, sort)


What if we add new constructor with sort as the very last non-optional parameter? In such way it would not be a breaking change and there would not be an ambiguity.

sanych-sun

I'm ready to LGTM the changes, but could you please rebase before that? Because in the current main we already have that apicompat script, just want to be sure we did everything right in terms of backward compatibility.

… update models

papafe · 2024-11-06T20:09:19Z

@sanych-sun just rebased on main, curious to see what will the script say 😁

papafe · 2024-11-07T08:50:39Z

@sanych-sun it seems the validate-apicompat task gave its ok 😁
Do you think this is mergeable now?

papafe requested a review from BorisDog September 18, 2024 14:21

BorisDog reviewed Sep 19, 2024

View reviewed changes

papafe force-pushed the csharp5201 branch from 19a4459 to 8c866be Compare October 16, 2024 09:44

sanych-sun requested changes Oct 16, 2024

View reviewed changes

papafe requested a review from sanych-sun October 23, 2024 12:22

sanych-sun requested changes Oct 30, 2024

View reviewed changes

BorisDog requested changes Oct 30, 2024

View reviewed changes

papafe force-pushed the csharp5201 branch from 733c0fd to ed6c5ea Compare November 4, 2024 13:23

papafe requested review from sanych-sun and BorisDog November 4, 2024 14:16

papafe commented Nov 4, 2024

View reviewed changes

papafe marked this pull request as ready for review November 4, 2024 14:31

papafe requested a review from a team as a code owner November 4, 2024 14:31

sanych-sun requested changes Nov 6, 2024

View reviewed changes

papafe requested a review from sanych-sun November 6, 2024 09:28

sanych-sun reviewed Nov 6, 2024

View reviewed changes

papafe added 14 commits November 6, 2024 20:52

CSHARP-5201: Allow update to supply sort option

d5bf4ae

Removed note

d0d482c

Small corrections

ef1eb12

Added missing specs

cc10fc3

Fixed tests for replace-one

dec8ade

Corrected client bulk write

f5a7082

Corrected bulk write tests

bf8e935

Small fixes

4b514de

Using sortdefinition instead of bsondocument in bulkwrite replace and…

af8574e

… update models

Test with ReplaceOne

b878172

Various fixes

593e1a1

Small fix

e940d7b

Adding ReplaceOptions<T> as subclass of ReplaceOptions

d295e63

Removed generic

b0b7cb9

papafe added 8 commits November 6, 2024 20:52

Small fixes

9392e96

Fixed update model

d8b6e93

Small fixesd

76f2382

Final fixes

a08e29e

Small correction

521aefb

Removed extras

3b38cd7

Corrections

d976964

Fixed constructorrs

103fe15

papafe force-pushed the csharp5201 branch from db7cadd to 103fe15 Compare November 6, 2024 20:08

		};
		//TODO Why don't UpdateOne and UpdateOneAsync have a common path for creating the UpdateOneModel and BulkWriteOptions (if not more)?

CSHARP-5201: Allow update to supply sort option #1453

Are you sure you want to change the base?

CSHARP-5201: Allow update to supply sort option #1453

Conversation

papafe commented Sep 18, 2024

BorisDog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisDog Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun left a comment

Choose a reason for hiding this comment

papafe commented Nov 6, 2024

papafe commented Nov 7, 2024

BorisDog Oct 30, 2024 •

edited

Loading

papafe Oct 31, 2024 •

edited

Loading

BorisDog Oct 30, 2024 •

edited

Loading