Skip to content

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions src/MongoDB.Driver/SortDefinitionBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ public static SortDefinition<TDocument> MetaTextScore<TDocument>(this SortDefini
/// <typeparam name="TDocument">The type of the document.</typeparam>
public sealed class SortDefinitionBuilder<TDocument>
{
/// <summary>
/// Creates a value ascending sort.
/// </summary>
/// <returns>An ascending sort.</returns>
public SortDefinition<TDocument> Ascending()
{
return new NoFieldDirectionalSortDefinition<TDocument>(SortDirection.Ascending);
}

/// <summary>
/// Creates an ascending sort.
/// </summary>
Expand Down Expand Up @@ -170,6 +179,15 @@ public SortDefinition<TDocument> Combine(IEnumerable<SortDefinition<TDocument>>
return new CombinedSortDefinition<TDocument>(sorts);
}

/// <summary>
/// Creates a value descending sort.
/// </summary>
/// <returns>A descending sort.</returns>
public SortDefinition<TDocument> Descending()
{
return new NoFieldDirectionalSortDefinition<TDocument>(SortDirection.Descending);
}

/// <summary>
/// Creates a descending sort.
/// </summary>
Expand Down Expand Up @@ -232,6 +250,11 @@ internal sealed class CombinedSortDefinition<TDocument> : SortDefinition<TDocume
public CombinedSortDefinition(IEnumerable<SortDefinition<TDocument>> sorts)
{
_sorts = Ensure.IsNotNull(sorts, nameof(sorts)).ToList();

if (_sorts.Any(sort => sort is NoFieldDirectionalSortDefinition<TDocument>))
{
throw new InvalidOperationException("Value-based sort cannot be combined with other sorts. When sorting by the entire element value, no other sorting criteria can be applied.");
}
}

public override BsonDocument Render(RenderArgs<TDocument> args)
Expand Down Expand Up @@ -288,4 +311,26 @@ public override BsonDocument Render(RenderArgs<TDocument> args)
return new BsonDocument(renderedField.FieldName, value);
}
}

internal sealed class NoFieldDirectionalSortDefinition<TDocument> : SortDefinition<TDocument>
{
private readonly SortDirection _direction;

public NoFieldDirectionalSortDefinition(SortDirection direction)
{
_direction = direction;
}

public override BsonDocument Render(RenderArgs<TDocument> args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a Direction property like DirectionalSortDefinition has?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have no use for that property, so I decided to omit it for now to avoid introducing unused code.

{
BsonValue value = _direction switch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have done this:

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

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

};

return new BsonDocument("direction", value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

}
}
}
8 changes: 7 additions & 1 deletion src/MongoDB.Driver/UpdateDefinitionBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1685,7 +1685,13 @@ public override BsonValue Render(RenderArgs<TDocument> args)

if (_sort != null)
{
document["$push"][renderedField.FieldName]["$sort"] = _sort.Render(args.WithNewDocumentType((IBsonSerializer<TItem>)itemSerializer));
var newArgs = args.WithNewDocumentType((IBsonSerializer<TItem>)itemSerializer);

var renderedSort = _sort is NoFieldDirectionalSortDefinition<TItem>
? _sort.Render(newArgs)["direction"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Note the added angle brackets.

: _sort.Render(newArgs);

document["$push"][renderedField.FieldName]["$sort"] = renderedSort;
}

return document;
Expand Down
27 changes: 27 additions & 0 deletions tests/MongoDB.Driver.Tests/SortDefinitionBuilderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* limitations under the License.
*/

using System;
using FluentAssertions;
using MongoDB.Bson;
using MongoDB.Bson.Serialization;
Expand All @@ -31,6 +32,14 @@ public void Ascending()
Assert(subject.Ascending("a"), "{a: 1}");
}

[Fact]
public void Ascending_no_field()
{
var subject = CreateSubject<BsonDocument>();

Assert(subject.Ascending(), "{direction: 1}");
}

[Fact]
public void Ascending_Typed()
{
Expand Down Expand Up @@ -76,6 +85,16 @@ public void Combine_with_repeated_fields_using_extension_methods()
Assert(sort, "{b: -1, a: -1}");
}

[Fact]
public void Combine_with_value_based_sort_and_additional_sort_should_throw()
{
var subject = CreateSubject<BsonDocument>();

var exception = Record.Exception(() => subject.Ascending().Descending("b"));

exception.Should().BeOfType<InvalidOperationException>();
}

[Fact]
public void Descending()
{
Expand All @@ -84,6 +103,14 @@ public void Descending()
Assert(subject.Descending("a"), "{a: -1}");
}

[Fact]
public void Descending_no_field()
{
var subject = CreateSubject<BsonDocument>();

Assert(subject.Descending(), "{direction: -1}");
}

[Fact]
public void Descending_Typed()
{
Expand Down
Loading