CSHARP-5543: Add new options for Atlas Search Text and Phrase operators #1678
Conversation
| /// <summary> | ||
| /// The score modifier. | ||
| /// </summary> | ||
| public SearchScoreDefinition<TDocument> Score { get; set; } |
| /// The criteria to use to match the terms in the query. Value can be either "any" or "all". | ||
| /// Defaults to "all" if omitted. | ||
| /// </summary> | ||
| public string MatchCriteria { get; set; } |
There was a problem hiding this comment.
Please check if this can be enum.
| @@ -457,6 +470,7 @@ internal sealed class TextSearchDefinition<TDocument> : OperatorSearchDefinition | |||
| private readonly SearchFuzzyOptions _fuzzy; | |||
| private readonly SearchQueryDefinition _query; | |||
| private readonly string _synonyms; | |||
| private readonly string _matchCriteria; | |||
src/MongoDB.Driver/MatchCriteria.cs
Outdated
| /// <summary> | ||
| /// Represents the criteria used to match terms in a query for the Atlas Search Text operator. | ||
| /// </summary> | ||
| public enum MatchCriteria |
There was a problem hiding this comment.
I think this should in Search folder (and namespace).
| @@ -329,11 +330,23 @@ public PhraseSearchDefinition( | |||
| _slop = slop; | |||
| } | |||
|
|
|||
| public PhraseSearchDefinition( | |||
There was a problem hiding this comment.
Do we need two ctors? This class is internal so it's safe to change.
| /// The criteria to use to match the terms in the query. Value can be either "any" or "all". | ||
| /// Defaults to "all" if omitted. | ||
| /// </summary> | ||
| public MatchCriteria MatchCriteria { get; set; } |
There was a problem hiding this comment.
I would say we do not need the property be nullable. It should simply have the default value of MatchCriteria.All. Why do we need additional third state of null?
There was a problem hiding this comment.
As per discussion with Boris, we probably should have it nullable so the user could explicitly have a 3rd unset state.
| /// The criteria to use to match the terms in the query. Value can be either "any" or "all". | ||
| /// Defaults to "all" if omitted. | ||
| /// </summary> | ||
| public MatchCriteria MatchCriteria { get; set; } |
There was a problem hiding this comment.
I would say we do not need the property be nullable. It should simply have the default value of MatchCriteria.All. Why do we need additional third state of null?
| private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) => | ||
| new() | ||
| { | ||
| { "query", _query.Render() }, | ||
| { "fuzzy", () => _fuzzy.Render(), _fuzzy != null }, | ||
| { "synonyms", _synonyms, _synonyms != null } | ||
| { "synonyms", _synonyms, _synonyms != null }, |
There was a problem hiding this comment.
Should we omit empty string also?
There was a problem hiding this comment.
Empty string is an invalid input and I think we should error on that but I usually prefer letting the server error rather than the driver unless the server doesn't provide a nice error.
| /// <summary> | ||
| /// The name of the synonym mapping definition in the index definition. Value can't be an empty string. | ||
| /// </summary> | ||
| public string Synonyms { get; set; } |
There was a problem hiding this comment.
xml-docs says: Value can't be an empty string. Should we enforce this condition?
There was a problem hiding this comment.
Similar to my other comment I prefer letting the server error rather than enforcing the condition in the driver. Yes, it's simple but I don't think the driver should be doing that much hand holding.
| private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) => | ||
| new() | ||
| { | ||
| { "query", _query.Render() }, | ||
| { "slop", _slop, _slop != null } | ||
| { "slop", _slop, _slop != null }, | ||
| { "synonyms", _synonyms, _synonyms != null } |
There was a problem hiding this comment.
Should we omit empty string also?
No description provided.