CSHARP-4901 Add support for SearchMeta facet without operator#1947
CSHARP-4901 Add support for SearchMeta facet without operator#1947kyra-rk wants to merge 3 commits intomongodb:mainfrom
Conversation
kyra-rk
added
bug
kyra-rk
requested review from
ajcvickers and
papafe
and removed request for
ajcvickers
There was a problem hiding this comment.
Pull request overview
Adds the ability to build/render Atlas Search facet definitions without specifying an underlying search operator, enabling $searchMeta facet use cases that only bucket on facets.
Changes:
- Added
SearchDefinitionBuilder.Facet(...)overloads that accept only facets (no operator). - Updated facet operator rendering to omit the
"operator"field when not provided. - Added unit/integration tests covering facet-without-operator for both typed/untyped builders and
$searchMeta.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/MongoDB.Driver/Search/SearchDefinitionBuilder.cs |
Adds Facet(IEnumerable<SearchFacet<TDocument>>) and Facet(params SearchFacet<TDocument>[]) overloads to support operator-less facet definitions. |
src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs |
Makes facet rendering conditional on whether an operator was provided (omit "operator" when null). |
tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs |
Adds rendering tests for facet definitions without an operator (typed and untyped). |
tests/MongoDB.Driver.Tests/Search/AtlasSearchTests.cs |
Adds an integration test verifying $searchMeta facet behavior without an operator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : base(OperatorType.Facet) | ||
| { | ||
| _operator = Ensure.IsNotNull(@operator, nameof(@operator)); | ||
| _operator = @operator; |
There was a problem hiding this comment.
Removing the non-null check for @operator changes behavior for the existing Facet(SearchDefinition<TDocument> @operator, ...) overloads: passing null previously threw ArgumentNullException, but now it silently renders a facet without an operator. To avoid masking bugs and preserve the prior contract, add a null check in the Facet overloads that take @operator (or restore validation in FacetSearchDefinition and introduce a separate code path/ctor for the operator-optional variant).
| _operator = @operator; | |
| _operator = Ensure.IsNotNull(@operator, nameof(@operator)); |
There was a problem hiding this comment.
Ah, this contradicts the original purpose of the ticket, which was to to make operator optional as per the Atlas Search docs.
There was a problem hiding this comment.
I think it is fine to remove the check here but we probably should then move the check up to the SearchDefinitionBuilder overloads that provide an operator. My take is if a user knowingly uses the overload that takes an operator and ends up passing a null we should throw.
| subject.Facet( facetBuilder.String("string", x => x.FirstName, 100)), | ||
| expected); | ||
| AssertRendered( | ||
| subject.Facet( facetBuilder.String("string", "FirstName", 100)), |
There was a problem hiding this comment.
There’s an extra space after the opening parenthesis in subject.Facet( facetBuilder... which is inconsistent with the surrounding formatting in this test file and makes the call harder to scan. Consider removing the space for consistency.
| subject.Facet( facetBuilder.String("string", x => x.FirstName, 100)), | |
| expected); | |
| AssertRendered( | |
| subject.Facet( facetBuilder.String("string", "FirstName", 100)), | |
| subject.Facet(facetBuilder.String("string", x => x.FirstName, 100)), | |
| expected); | |
| AssertRendered( | |
| subject.Facet(facetBuilder.String("string", "FirstName", 100)), |
There was a problem hiding this comment.
Will adjust the formatting to be on the next line!
| subject.Facet( facetBuilder.String("string", x => x.FirstName, 100)), | ||
| expected); | ||
| AssertRendered( | ||
| subject.Facet( facetBuilder.String("string", "FirstName", 100)), |
There was a problem hiding this comment.
There’s an extra space after the opening parenthesis in subject.Facet( facetBuilder... which is inconsistent with the surrounding formatting in this test file and makes the call harder to scan. Consider removing the space for consistency.
| subject.Facet( facetBuilder.String("string", x => x.FirstName, 100)), | |
| expected); | |
| AssertRendered( | |
| subject.Facet( facetBuilder.String("string", "FirstName", 100)), | |
| subject.Facet(facetBuilder.String("string", x => x.FirstName, 100)), | |
| expected); | |
| AssertRendered( | |
| subject.Facet(facetBuilder.String("string", "FirstName", 100)), |
There was a problem hiding this comment.
Will adjust the formatting to be on the next line!
| Facet(@operator, (IEnumerable<SearchFacet<TDocument>>)facets); | ||
|
|
||
| /// <summary> | ||
| /// Creates a search definition that groups results by values or ranges in the specified |
There was a problem hiding this comment.
The four Facet overloads now have near-identical doc text. A user in IntelliSense has no signal for when to pick the operator-less form. The no-operator overloads could say something like "Facets are computed over all documents in the index" to help a little.
| : base(OperatorType.Facet) | ||
| { | ||
| _operator = Ensure.IsNotNull(@operator, nameof(@operator)); | ||
| _operator = @operator; |
There was a problem hiding this comment.
I think it is fine to remove the check here but we probably should then move the check up to the SearchDefinitionBuilder overloads that provide an operator. My take is if a user knowingly uses the overload that takes an operator and ends up passing a null we should throw.
| { | ||
| _operator = Ensure.IsNotNull(@operator, nameof(@operator)); | ||
| _operator = @operator; | ||
| _facets = Ensure.IsNotNull(facets, nameof(facets)).ToArray(); |
There was a problem hiding this comment.
With the new Facet(params SearchFacet<TDocument>[] facets) overload, we probably want to check the facets array is not null and is not empty (so use Ensure.IsNotNullOrEmpty()).
if we don't then something like this, compiles and run;
Builders.Search.Facet();
It renders { facet: { facets: {} } } and gets rejected server-side instead of at build time.
No description provided.