new implementation of $elemMatch support #985

evanchooly · 2016-08-31T20:51:38Z

This is a replacement implementation of $elemMatch. The old "hasThisElement" and "doesNotHaveThisElement" are being deprecated as they are rather limited in what they support. This resolves #970.

add support for $not with $elemMatch

jyemin · 2016-09-01T02:03:45Z

morphia/src/main/java/org/mongodb/morphia/query/FieldEndImpl.java

@@ -142,7 +142,13 @@ public T doesNotHaveThisElement(final Object val) {
    @Override
    public T hasThisElement(final Object val) {
        Assert.parametersNotNull("val", val);
-        return addCriteria(FilterOperator.ELEMENT_MATCH, val, false);
+        return addCriteria(FilterOperator.ELEMENT_MATCH, val, not);


Why did the 3rd param just from false to 'not'? Is this fixing a bug?

It is. It's currently not possible to negate this operation because of that hard coded 'false'

Is that bug reported? If not, it should be so we can track that it's getting fixed. Also, ensure that the bug fix should be done in a separate commit from the new feature work for elemMatch (I didn't check).

Ping on this last comment. Really should avoid drive-by bug fixes like this. Open a separate issue and fix it separately.

I rolled back that fix but hadn't filed the issue. I'll do that now.

jyemin · 2016-09-01T14:57:16Z

morphia/src/test/java/org/mongodb/morphia/TestQuery.java

@@ -369,37 +391,67 @@ public void testElemMatchQueryCanBeNegated() {

        Mapper mapper = getMorphia().getMapper();

-        List<Key<PhotoWithKeywords>> keys = getDs().find(PhotoWithKeywords.class)
+        validate(pwk1, pwk2, pwk3, pwk4, mapper, getDs().find(PhotoWithKeywords.class)


validate and validate2 are not testing what the test method says it is: that elemMatch queries can be negated. Only validate3 is doing that. So put the validate/validate2 assertions in a separate test.

those are literally just refactorings of existing tests in to methods so i can use both elemMatch queries with the same asserts. I didn't change what they were testing

Yes, I see that the validate methods are a refactoring. But a bunch more assertions have been added to this test method and I'm suggesting that it would be better to restructure into multiple test methods whose named better describe what it is that is being tested. And once you've done that, the test names you've chosen will suggest what the validate* methods should be renamed to to reflect their purpose.

Let me know if this doesn't make sense and we can talk about it more.

I was trying to avoid cutting too deeply but I bit the bullet and cleaned that up a bit. less code now and easier to reason about what's being asserted.

I like the new validate method... but there are still assertions that have nothing to do with negation in a test method called testElemMatchQueryCanBeNegated. That's just confusing, don't you think?

a bit, yeah. there's only the one negation at the end, really. i'll brainstorm some new names. to be honest, i think this test duplicates a number of others and can probably be combined...

Looks good now.

evanchooly · 2016-09-07T18:38:18Z

with the method renamed, all else is good @jyemin ?

jyemin · 2016-09-07T18:51:05Z

I'd still like to see the bug on the existing method reported and the fix pulled out into a separate commit.

jyemin · 2016-09-07T19:10:03Z

LGTM

* use a query for complex hasThisElement clauses? * introduce elemMatch() with better semantics * expand tests to ensure old and new methods are tested. add support for $not with $elemMatch * deprecate `doesNotHaveThisElement` * clean up validations * rename method

yizhangy · 2019-02-10T10:09:57Z

Hi @evanchooly
I find the the error warning message when used the "elemMatch"

Query<SelectValue> selectValueQuery = super.getMongoDatastore().createQuery(SelectValue.class);
		selectValueQuery.filter("id =", voteValueId);
		selectValueQuery.filter("checked =", true);
		
	
		
		Query<FeedMemberEntity> query = super.getMongoDatastore().find(FeedMemberEntity.class);
		query.field("feedId").equal(feedId);
		query.field("voteValues").elemMatch(selectValueQuery);
		
		return query.count();

My log had

[Validation failed: 'Type java.util.List may not be queryable with value '{ query: { "id" : "0" , "checked" : true}  }' with class xyz.morphia.query.QueryImpl', Validation failed: 'Type com.canaan.config.SelectValue may not be queryable with value '{ query: { "id" : "0" , "checked" : true}  }' with class xyz.morphia.query.QueryImpl']
Feb 10, 2019 11:04:44 PM xyz.morphia.query.QueryValidator validateQuery

Is this still a bug

evanchooly · 2019-02-10T16:22:55Z

Probably related #1288

yizhangy · 2019-02-12T04:21:57Z

#1288

Probably related #1288

thanks refer me to this issue. It is not big issue, just a warning. we could disable the validation when make this query for now. thanks.

Justin Lee added 3 commits August 31, 2016 16:48

use a query for complex hasThisElement clauses?

59d59fe

introduce elemMatch() with better semantics

7275121

expand tests to ensure old and new methods are tested.

5feff43

add support for $not with $elemMatch

evanchooly added bug enhancement labels Aug 31, 2016

evanchooly added this to the 1.3.0 milestone Aug 31, 2016

evanchooly assigned jyemin Aug 31, 2016

jyemin reviewed Sep 1, 2016
View reviewed changes

deprecate doesNotHaveThisElement

457f63d

jyemin reviewed Sep 1, 2016
View reviewed changes

Justin Lee added 2 commits September 1, 2016 13:28

clean up validations

32404d3

rename method

4e7071a

evanchooly merged commit 94a1bbc into master Sep 7, 2016

evanchooly removed the bug label Sep 7, 2016

evanchooly deleted the hasThisElement branch September 16, 2016 17:32

philipcregan unassigned jyemin Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new implementation of $elemMatch support #985

new implementation of $elemMatch support #985

evanchooly commented Aug 31, 2016

jyemin Sep 1, 2016

evanchooly Sep 1, 2016

jyemin Sep 1, 2016 •

edited

Loading

jyemin Sep 7, 2016

evanchooly Sep 7, 2016

jyemin Sep 1, 2016

evanchooly Sep 1, 2016

jyemin Sep 1, 2016

evanchooly Sep 1, 2016

jyemin Sep 1, 2016

evanchooly Sep 1, 2016

jyemin Sep 7, 2016

evanchooly commented Sep 7, 2016

jyemin commented Sep 7, 2016

jyemin commented Sep 7, 2016

yizhangy commented Feb 10, 2019 •

edited

Loading

evanchooly commented Feb 10, 2019

yizhangy commented Feb 12, 2019

new implementation of $elemMatch support #985

new implementation of $elemMatch support #985

Conversation

evanchooly commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin Sep 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanchooly commented Sep 7, 2016

jyemin commented Sep 7, 2016

jyemin commented Sep 7, 2016

yizhangy commented Feb 10, 2019 • edited Loading

evanchooly commented Feb 10, 2019

yizhangy commented Feb 12, 2019

jyemin Sep 1, 2016 •

edited

Loading

yizhangy commented Feb 10, 2019 •

edited

Loading