CSHARP-5672: Support sorting by value in PushEach operation #1748

adelinowona · 2025-08-07T15:05:21Z

No description provided.

rstam

Looks good. Minor changes requested.

src/MongoDB.Driver/SortDefinitionBuilder.cs

rstam · 2025-08-07T21:19:55Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+            {
+                SortDirection.Ascending => 1,
+                SortDirection.Descending => -1,
+                _ => throw new InvalidOperationException("Unknown value for " + typeof(SortDirection) + ".")


I would have done this:

_ => throw new InvalidOperationException($"Invalid sort direction: {_direction}.")

but I suppose then we might want to change DirectionalSortDefinition also...

rstam · 2025-08-07T21:21:21Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+                _ => throw new InvalidOperationException("Unknown value for " + typeof(SortDirection) + ".")
+            };
+
+            return new BsonDocument("direction",  value);


When we use "magic" values like this we usually enclose them in angle brackets to make it visually apparent that something is special about this value:

return new BsonDocument("<direction>", value);

We only have a field name here because Render is already declared to return BsonDocument and changing it to return BsonValue would be a breaking change.

So what happens in other cases (which are not UpdateDefinitionBuilder), will it be rendered as
sort: { direction: ... } and do we want that?

We could add
internal virtual BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => Render(args);
to SortDefinition, and refactor Render to return BsonValue in 4.0.

Nice, I like this! I thought of something similar but thought adding something to SortDefinition would be a breaking change but I suppose adding an internal virtual method is not? Learning some library maintainer tricks :)

With this updated approach, I am also thinking of throwing an exception when a call to Render is made on a ValueDirectionalSortDefinition. A call to render for ValueDirectionalSortDefinition is suggesting someone is trying to use this where a field-based sort is expected which is invalid.

rstam · 2025-08-07T21:21:40Z

src/MongoDB.Driver/UpdateDefinitionBuilder.cs

+                var newArgs = args.WithNewDocumentType((IBsonSerializer<TItem>)itemSerializer);
+
+                var renderedSort = _sort is NoFieldDirectionalSortDefinition<TItem>
+                    ? _sort.Render(newArgs)["direction"]


? _sort.Render(newArgs)["<direction>"]

Note the added angle brackets.

tests/MongoDB.Driver.Tests/SortDefinitionBuilderTests.cs

BorisDog · 2025-08-08T17:06:40Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+
+        public override BsonDocument Render(RenderArgs<TDocument> args)
+        {
+            BsonValue value = _direction switch


We do the SortDirection to BsonValue conversion in 2 different places already.
Consider extorting this to internal SortDirectionExtensions or similar?

BorisDog · 2025-08-08T17:30:03Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+                _ => throw new InvalidOperationException("Unknown value for " + typeof(SortDirection) + ".")
+            };
+
+            return new BsonDocument("direction",  value);


So what happens in other cases (which are not UpdateDefinitionBuilder), will it be rendered as
sort: { direction: ... } and do we want that?

We could add
internal virtual BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => Render(args);
to SortDefinition, and refactor Render to return BsonValue in 4.0.

adelinowona · 2025-08-12T16:58:02Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+
+        internal override BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => _direction.ToBsonValue();
+
+        public override BsonDocument Render(RenderArgs<TDocument> args)


For now, I'm using the exception thrown during rendering as the mechanism to inform users that value-based sorts aren't supported. Since I've only updated call sites where value-based sorts are explicitly supported, any Render call that returns a BsonDocument will naturally fail in unsupported contexts, keeping this PR's changes minimal. However, Alex raised a point that when we update Render to return BsonValue in 4.0, we'll need to add validation at all call sites to check for value-based sort support before rendering. The question is whether to implement this validation now or defer it to 4.0. Implementing it now would distribute some of the 4.0 migration work and provide users with cleaner exceptions higher in the stack trace (and potentially a better exception type). I suppose we'll probably want to do that but what's the preference? @BorisDog @rstam

I think the way you have it now is fine.

We can address whatever changes (if any) are needed later.

Most places that currently call Render should work with a BsonValue also. If not there is some work to be done a the CALL site, not here.

I agree with @rstam, we can defer the work to 4.0.

rstam

Minor changes requested.

rstam · 2025-08-12T20:47:11Z

src/MongoDB.Driver/SortDefinition.cs

@@ -47,6 +47,9 @@ public abstract class SortDefinition<TDocument>
        /// <returns>A <see cref="BsonDocument"/>.</returns>
        public abstract BsonDocument Render(RenderArgs<TDocument> args);

+        // TODO: remove this and refactor Render to return a BsonValue in 4.0
+        internal virtual BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => Render(args);


The RenderAsBsonValue method should probably be placed after the Render method.

It already is? Maybe you got a defective diff

I tagged the wrong one. It's the one on line 311 of SortDefinitionBuilder.cs that's out of order.

rstam · 2025-08-12T20:47:36Z

src/MongoDB.Driver/SortDirectionExtensions.cs

+{
+    internal static class SortDirectionExtensions
+    {
+        internal static BsonValue ToBsonValue(this SortDirection direction)


I think this method should probably be named Render.

rstam · 2025-08-12T20:48:46Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+
+        internal override BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => _direction.ToBsonValue();
+
+        public override BsonDocument Render(RenderArgs<TDocument> args)


I think the way you have it now is fine.

We can address whatever changes (if any) are needed later.

Most places that currently call Render should work with a BsonValue also. If not there is some work to be done a the CALL site, not here.

BorisDog · 2025-08-12T23:24:03Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+
+        internal override BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => _direction.ToBsonValue();
+
+        public override BsonDocument Render(RenderArgs<TDocument> args)


I agree with @rstam, we can defer the work to 4.0.

BorisDog · 2025-08-12T23:27:02Z

src/MongoDB.Driver/SortDirectionExtensions.cs

+{
+    internal static class SortDirectionExtensions
+    {
+        internal static BsonValue ToBsonValue(this SortDirection direction)


Nice, would be good to use it in other places as well, like DirectionalIndexKeyDefinition.

BorisDog · 2025-08-13T15:37:34Z

src/MongoDB.Driver/SortDirectionExtensions.cs

+    {
+        internal static BsonValue Render(this SortDirection direction)
+        {
+            return direction switch


minor: you can use expression body definition.

BorisDog · 2025-08-13T15:38:44Z

tests/MongoDB.Driver.Tests/SortDefinitionBuilderTests.cs

+        {
+            var subject = CreateSubject<BsonDocument>();
+
+            Assert(subject.Ascending(), "{direction: 1}");


Fix the tests, and add new one to test exception?

rstam

Looks good with one minor change requested.

rstam · 2025-08-14T18:17:45Z

src/MongoDB.Driver/SortDefinition.cs

@@ -47,6 +47,9 @@ public abstract class SortDefinition<TDocument>
        /// <returns>A <see cref="BsonDocument"/>.</returns>
        public abstract BsonDocument Render(RenderArgs<TDocument> args);

+        // TODO: remove this and refactor Render to return a BsonValue in 4.0
+        internal virtual BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => Render(args);


I tagged the wrong one. It's the one on line 311 of SortDefinitionBuilder.cs that's out of order.

rstam · 2025-08-14T18:18:20Z

src/MongoDB.Driver/SortDefinitionBuilder.cs

+            _direction = direction;
+        }
+
+        internal override BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => _direction.Render();


Move RenderAsBsonValue to after Render.

adelinowona requested a review from BorisDog August 7, 2025 15:05

adelinowona requested a review from a team as a code owner August 7, 2025 15:05

adelinowona added improvement feature and removed improvement labels Aug 7, 2025

rstam requested changes Aug 7, 2025

View reviewed changes

BorisDog requested changes Aug 8, 2025

View reviewed changes

adelinowona added 3 commits August 11, 2025 17:47

CSHARP-5672: Support sorting by value in updateDefinition builders

9f29350

add simple tests

ea3784a

address pr comments

121021d

adelinowona force-pushed the csharp5672 branch from a47ba07 to 121021d Compare August 11, 2025 21:47

adelinowona requested review from rstam and BorisDog August 11, 2025 21:54

adelinowona commented Aug 12, 2025

View reviewed changes

rstam requested changes Aug 12, 2025

View reviewed changes

BorisDog requested changes Aug 12, 2025

View reviewed changes

address more pr comments

c0d08cf

adelinowona requested review from rstam and BorisDog August 13, 2025 00:43

BorisDog requested changes Aug 13, 2025

View reviewed changes

fix tests

533fe3e

adelinowona requested a review from BorisDog August 14, 2025 07:22

rstam requested changes Aug 14, 2025

View reviewed changes

move overriden method

e998575

adelinowona requested a review from rstam August 14, 2025 18:51


		internal override BsonValue RenderAsBsonValue(RenderArgs<TDocument> args) => _direction.ToBsonValue();

		public override BsonDocument Render(RenderArgs<TDocument> args)

CSHARP-5672: Support sorting by value in PushEach operation #1748

Are you sure you want to change the base?

CSHARP-5672: Support sorting by value in PushEach operation #1748

Conversation

adelinowona commented Aug 7, 2025

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!