New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MongodbSerializer does not serialize isNotEmpty() properly #1264

Closed
johnktims opened this Issue Mar 17, 2015 · 16 comments

Comments

Projects
None yet
3 participants
@johnktims
Member

johnktims commented Mar 17, 2015

Original link: http://stackoverflow.com/questions/29104204/querydsl-isnotempty-expression-failing

There appears to be a serialization error in MongodbSerializer for

BooleanExpression expr = qa.matches.isNotEmpty();
Iterable<A> result = aRepo.findAll(expr);

assertThat(result, is(not((emptyIterable()))));

MongodbSerializer is spitting out

{ "$or" : { "$not" : [ { "matches" : [ ]} , { "matches" : { "$exists" : false}}]}}

which fails when I paste it into mongo.

> db.a.find({ "$or" : { "$not" : [ { "matches" : [ ]} , { "matches" : { "$exists" : false}}]}})
Error: error: {
    "$err" : "Can't canonicalize query: BadValue $or needs an array",
    "code" : 17287
}
> 

I rewrote the query as

> db.a.find({ "$or" : [ { "matches" : {$ne: [ ]}}, { "matches" : { "$exists" : false}}]}).pretty()
{
    "_id" : "a1",
    "_class" : "querydsldemo.A",
    "matches" : [
        {
            "_id" : ObjectId("55088ec4e4b0b6e56e76b1d6"),
            "name" : "b1"
        },
        {
            "_id" : ObjectId("55088ec4e4b0b6e56e76b1d7"),
            "name" : "b2"
        }
    ]
}
{ "_id" : "a3", "_class" : "querydsldemo.A" }

and then further simplified it as

> db.a.find({ "matches" : {$ne: [ ]}}).pretty()
{
    "_id" : "a1",
    "_class" : "querydsldemo.A",
    "matches" : [
        {
            "_id" : ObjectId("55088ec4e4b0b6e56e76b1d6"),
            "name" : "b1"
        },
        {
            "_id" : ObjectId("55088ec4e4b0b6e56e76b1d7"),
            "name" : "b2"
        }
    ]
}
{ "_id" : "a3", "_class" : "querydsldemo.A" }
> 

but I don't know if that's robust enough.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 17, 2015

Member

The negation seems to break, you can look at the handling of Ops.COL_IS_EMPTY and Ops.NOT.

Member

timowest commented Mar 17, 2015

The negation seems to break, you can look at the handling of Ops.COL_IS_EMPTY and Ops.NOT.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Mar 17, 2015

Member

It isn't really the negation, but the passing of an object to $or instead of elements. ('{ }' vs '[ ]') that breaks.

Member

Shredder121 commented Mar 17, 2015

It isn't really the negation, but the passing of an object to $or instead of elements. ('{ }' vs '[ ]') that breaks.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 18, 2015

Member

Yes, but I believe it is the negation processing that breaks it. The isempty logic looks ok.

Member

timowest commented Mar 18, 2015

Yes, but I believe it is the negation processing that breaks it. The isempty logic looks ok.

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Mar 18, 2015

Member

Ah in that sense I agree.

Member

Shredder121 commented Mar 18, 2015

Ah in that sense I agree.

@timowest timowest added the bug label Mar 18, 2015

@johnktims

This comment has been minimized.

Show comment
Hide comment
@johnktims

johnktims Mar 18, 2015

Member

I did switch the expression to use isEmpty and that appeared to work properly (although obviously the opposite of what the user wants) so I think you're right, @timowest .

Member

johnktims commented Mar 18, 2015

I did switch the expression to use isEmpty and that appeared to work properly (although obviously the opposite of what the user wants) so I think you're right, @timowest .

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 18, 2015

Member

Is someone already working on this? Otherwise I can give it a try.

Member

timowest commented Mar 18, 2015

Is someone already working on this? Otherwise I can give it a try.

@johnktims

This comment has been minimized.

Show comment
Hide comment
@johnktims

johnktims Mar 18, 2015

Member

I'm working on it now, but I'm not sure of the best way to fix this. The syntax for $not is making it difficult for me to fix. I haven't used mongodb a whole lot yet, so that might be the issue. :)

Member

johnktims commented Mar 18, 2015

I'm working on it now, but I'm not sure of the best way to fix this. The syntax for $not is making it difficult for me to fix. I haven't used mongodb a whole lot yet, so that might be the issue. :)

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Mar 18, 2015

Member

I am not, I only debugged it, but didn't yet solve it.

Member

Shredder121 commented Mar 18, 2015

I am not, I only debugged it, but didn't yet solve it.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 18, 2015

Member

@johnktims Ok, let me know if you have some questions.

Member

timowest commented Mar 18, 2015

@johnktims Ok, let me know if you have some questions.

@johnktims

This comment has been minimized.

Show comment
Hide comment
@johnktims

johnktims Mar 18, 2015

Member

Should every ticket be assigned to a milestone so that it isn't forgotten in the changelog?

Member

johnktims commented Mar 18, 2015

Should every ticket be assigned to a milestone so that it isn't forgotten in the changelog?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 18, 2015

Member

Yes.

Member

timowest commented Mar 18, 2015

Yes.

@johnktims johnktims added this to the 3.6.3 milestone Mar 18, 2015

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Mar 25, 2015

Member

If we want it to appear in 3.6.3, I think we'll have to backport it.

Member

Shredder121 commented Mar 25, 2015

If we want it to appear in 3.6.3, I think we'll have to backport it.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 26, 2015

Member

Yes, I can do it, if @johnktims doesn't beat me at it.

Member

timowest commented Mar 26, 2015

Yes, I can do it, if @johnktims doesn't beat me at it.

@johnktims

This comment has been minimized.

Show comment
Hide comment
@johnktims

johnktims Mar 26, 2015

Member

I was planning to backport it this morning. Have you started it?

Member

johnktims commented Mar 26, 2015

I was planning to backport it this morning. Have you started it?

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Mar 26, 2015

Member

No, haven't started yet. Feel free to do it, if you have time.

Member

timowest commented Mar 26, 2015

No, haven't started yet. Feel free to do it, if you have time.

@johnktims

This comment has been minimized.

Show comment
Hide comment
@johnktims

johnktims Mar 26, 2015

Member

Cool. I'll handle it.

Member

johnktims commented Mar 26, 2015

Cool. I'll handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment