[CSHARP-289] QueryConditionList and QueryNotConditionList to have the same abstract base class BaseQueryConditionList or implement IQueryConditionList Created: 27/Jul/11  Updated: 14/May/14  Resolved: 19/Oct/12

Status: Closed
Project: C# Driver
Component/s: None
Affects Version/s: 1.1
Fix Version/s: None

Type: Improvement Priority: Minor - P4
Reporter: Roman Kuzmin Assignee: Robert Stam
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Minor Change

 Description   

QueryConditionList and QueryNotConditionList have a bunch of similar methods – All, ElemMatch, Exists, GT, GTE, In, LT, LTE, Mod, NE, NotIn, Size, Type. An application would like to build queries dynamically chaining them, say, in this way:

BaseQueryConditionList GT(BaseQueryConditionList conditionList, BsonValue value)

{ return conditionList.GT(value); }

This scenario is not possible now. The application has to (just for example):

QueryComplete GT(QueryComplete conditionList, BsonValue value)

{ if (conditionList is QueryConditionList) return ((QueryConditionList)conditionList).GT(value); else if (conditionList is QueryNotConditionList) return ((QueryNotConditionList)conditionList).GT(value); else throw new InvalidArgumentException(); }

This is equally awkward to write and not effective at runtime. The proposal is to derive these two classes from the same abstract class or introduce a common interface and make these classes to implement it.

Related question about QueryConditionList and QueryNotConditionList. QueryConditionList has Within* methods, QueryNotConditionList does not. Is this difference intentional?

==

Details and potential drawbacks. If we go with, say, the abstract base class then common methods should change their return values to that base class. That is:

QueryConditionList All(BsonArray values) // QueryConditionList
QueryNotConditionList All(BsonArray values) // QueryNotConditionList

Should be:

BaseQueryConditionList All(BsonArray values) // QueryConditionList
BaseQueryConditionList All(BsonArray values) // QueryNotConditionList

Is this going to be troublesome? Of course it is breaking (but it should not be difficult to adopt). As for the other drawbacks, I cannot see them (but I see many advantages). What do you think, who reads this?

==

As far as I can see, many methods in these two classes have duplicated code. A base class could have them implemented in one place (virtual methods perhaps would not even be needed).
Example:

// MongoDB.Driver.Builders.QueryConditionList
public QueryConditionList ElemMatch(IMongoQuery query)
{
this.conditions.Add("$elemMatch", query.ToBsonDocument<IMongoQuery>());
return this;
}
// MongoDB.Driver.Builders.QueryNotConditionList
public QueryNotConditionList ElemMatch(IMongoQuery query)
{
this.conditions.Add("$elemMatch", query.ToBsonDocument<IMongoQuery>());
return this;
}

and etc.
Instead, it could be the only method of BaseQueryConditionList:

// MongoDB.Driver.Builders.BaseQueryConditionList
public BaseQueryConditionList ElemMatch(IMongoQuery query)
{
this.conditions.Add("$elemMatch", query.ToBsonDocument<IMongoQuery>());
return this;
}



 Comments   
Comment by Robert Stam [ 19/Oct/12 ]

With the new Query builders introduced in version 1.5 of the C# driver the QueryConditionList and QueryNotConditionList classes have gone away, so I'm closing this issue as it is no longer applicable.

Comment by Roman Kuzmin [ 27/Jul/11 ]

Robert,

Thank you for your suggestion. Perhaps I go this lower way. But this is a pity. It's like reinventing the wheel which is already implemented in the driver.

What driver does is good, it hides the details. A user can but does not have to know that $xyz stuff. A user has fewer chances to make mistakes with the driver (in theory).

Still, consider to merge these two lists. This approach is simpler both for the driver code and its API users. It does not exclude forks (derived classes) in the future, by the way, but only if we really need this.

But even if these two branches are still going to coexist then a common base class or an interface would not hurt anybody. Moreover it will give us an API easier to use (my case) and understand (with a base class/interface it is quite clear what these query lists have in common).

Comment by Robert Stam [ 27/Jul/11 ]

The original intent of having two separate classes (QueryConditionList and QueryNotConditionList) was because some operations that might be allowed in one might not be allowed in the other. I see your point though, that at the moment they are very similar.

One comment about your usage, though. The entire Query static class and QueryBuilder class hierarchy is designed to support Intellisense driven query building while writing source code in an IDE, not necessarily for dynamically building a query at runtime. If you want to create your query dynamically at runtime you can just create a QueryDocument directly. For example, instead of writing:

var query1 = Query.GT("x", 1);

you can write:

var query2 = new QueryDocument("$gt", new BsonDocument("x", 1));

You can build arbitrarily complex queries dynamically just by building up the QueryDocument/BsonDocument and not using the Query builder at all.

QueryDocument is a subclass of BsonDocument. When building up a query dynamically at runtime, you can either use BsonDocument or QueryDocument throughout. If you build up your query using BsonDocument, at the very end you need to encapsulate it as a query using either:

BsonDocument query;
var copiedQuery = new QueryDocument(query); // copies the query into a QueryDocument
var wrappedQuery = QueryWrapper.Create(query); // wraps the query in a QueryWrapper

The second option is more efficient because it doesn't have to copy the whole query document.

Comment by Roman Kuzmin [ 27/Jul/11 ]

On the second thought, do we really have to maintain two branches QueryConditionList and QueryNotConditionList? Unless I miss something, they differ only by constructors and QueryConditionList has extra Within* methods. It looks like we can have the only class QueryConditionList with different constructors and, if "Not" list really does not support Within* methods then they may throw an NotSupportedException. Perhaps it makes sense for the sake of simplicity of API and ease of use (remember, I have started complaining about practical inconveniences I had to deal with my application).

Generated at Wed Feb 07 21:36:23 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.