[SERVER-12263] Support $elemMatch with DBRefs that may contain additional properties Created: 06/Jan/14 Updated: 26/Jan/17 Resolved: 07/Feb/14 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Querying |
| Affects Version/s: | 2.5.4 |
| Fix Version/s: | 2.6.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Jeremy Mikola | Assignee: | Benety Goh |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||
| Operating System: | ALL | ||||||||||||||||||||
| Participants: | |||||||||||||||||||||
| Description |
|
tl;dr:
The current implementation simply treats an $elemMatch on a DBRef as an equality match, which doesn't seem correct. Consider the following, which uses the latest nightly:
First, I create three documents in the test.people collection. The middle document has an extra field in its DBRef object, which is something Doctrine ODM has done for years to discriminate references to super-classes. Based on the DBRef docs, they're only objects that required $ref and $id as the first two fields with an optional $db to follow. There was never a restriction on additional fields, even though the shell likes to hide them (as it does $db).
The example below demonstrates the shortcoming of treating $elemMatch as an equality comparison, and relying on _isDBRefDocument() for the detection:
In the first and last example, we get the original error because $elemMatch doesn't recognize its argument as a DBRef and apply the patch logic. But even with the patch, the equality comparison fails to match anything in the middle two examples, since the DBRef in the database does have an extra _doctrine_class_name field within. Repeating the test with the third example document, which doesn't include the extra Doctrine field:
We still have the same shortcomings, although the equality match in the last example does happen to work. I'm still not keen on that behavior because this is nothing like a real $elemMatch evaluation. |
| Comments |
| Comment by Jeremy Mikola [ 12/Feb/14 ] | |||||||
|
benety.goh: Just tested master and the above cases all work. Thanks for the fix. | |||||||
| Comment by Githook User [ 07/Feb/14 ] | |||||||
|
Author: {u'username': u'benety', u'name': u'Benety Goh', u'email': u'benety@mongodb.com'}Message: | |||||||
| Comment by Benety Goh [ 07/Feb/14 ] | |||||||
|
hope this last patch submitted to master works for the doctrine test cases. | |||||||
| Comment by Jeremy Mikola [ 06/Feb/14 ] | |||||||
|
How so? The original description always included the following case:
Likewise, scotthernandez's test cases pre-date any work on the ticket. | |||||||
| Comment by Benety Goh [ 06/Feb/14 ] | |||||||
|
Can we make support for incomplete DBRefs in $elemMatch another ticket? That wasn't in the scope of the original description that you wrote. | |||||||
| Comment by Jeremy Mikola [ 05/Feb/14 ] | |||||||
|
I noticed that this commit included a test for $in where the DBRef was missing fields, which seems well and good ($in should have an array of valid DBRefs in that case), but there is no test for $elemMatch with an incomplete DBRef, which is what should be supported per Scott's earlier comment. | |||||||
| Comment by Jeremy Mikola [ 05/Feb/14 ] | |||||||
|
benety.goh: I compiled https://github.com/mongodb/mongo/commit/bd801007fa10cadfb25c1b211249a17fb0b97ac5, which includes your commits above, and some broken behavior remains:
The first query succeeds, so that bit was fixed, but the second and third queries still fail. | |||||||
| Comment by Githook User [ 05/Feb/14 ] | |||||||
|
Author: {u'username': u'benety', u'name': u'Benety Goh', u'email': u'benety@mongodb.com'}Message: | |||||||
| Comment by Githook User [ 05/Feb/14 ] | |||||||
|
Author: {u'username': u'benety', u'name': u'Benety Goh', u'email': u'benety@mongodb.com'}Message: | |||||||
| Comment by Scott Hernandez (Inactive) [ 22/Jan/14 ] | |||||||
|
This needs to work.
|