[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 Should be: BaseQueryConditionList All(BsonArray values) // QueryConditionList 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). // MongoDB.Driver.Builders.QueryConditionList and etc. // MongoDB.Driver.Builders.BaseQueryConditionList |
| 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; 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). |