Skip to content
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

QueryValidator canQueryPast NPE #881

Closed
agiannone opened this issue Jan 15, 2016 · 11 comments
Closed

QueryValidator canQueryPast NPE #881

agiannone opened this issue Jan 15, 2016 · 11 comments
Labels
Milestone

Comments

@agiannone
Copy link

The QueryValidator validateQuery() method calls the canQueryPast() method, but at this stage the mf variable can be equal to null. This causes the canQueryPast() method to throw an exception. A null check should be added to the canQueryPast() method.

To recreate this you need a nested interface:

public interface Nested {}

public class NestedImplA implements Nested {
private String field;
}

public class WithNested {
private Nested nested;
}

When we query WithNested using: nested.field and disable validation, we end up getting an NPE because the MappedField will be null.

This is stopping any query which uses a field nested in an interface implementation.

@evanchooly
Copy link
Member

this sounds like a refinement of the problem we've been discussing in #777, yes? or is this a new one?

@agiannone
Copy link
Author

This is a new one, although the root cause of both bugs seems to be querying or indexing on a field nested in an interface (or better, nested in a class which implements the interface).

In both cases the code trys to get the MappedField for the actual interface implementation which isn't available and that causes the problems.

How long do you recon before this could be fixed?

I ask because our product stopped working after the update done a few days ago. The only alternative I see is reverting back to 1.0.1 but I remember migrating off that because of a problem.

@evanchooly
Copy link
Member

I'll take a look now at the test case. I have a feeling I know where/what the issue is and then take it from there.

@agiannone
Copy link
Author

I found what changed and started causing this bug... in r1.1.0-alpha1 on line 49 of the QueryValidator we have:

if (validateNames) {

In r1.1.0 this is missing. As we're querying on a field inside the implementation of an interface, we don't want validation to take place. This is why there is an NPE in the method, because it trying to validate the names.

If the line was removed in order to ensure that MappedField is not null at a later stage, then I would recommend creating a MappedField.EMPTY value in order to avoid nulls floating around. This will avoid NPEs further down the line, reduce null checks and allow the validation of the name to be skipped.

@evanchooly
Copy link
Member

I can't recreate your error on master using this test:

    @Test
    public void testNestedInterfaces() {
        getMorphia().map(WithNested.class);
        WithNested nested = new WithNested();
        nested.nested = new NestedImpl("nested value");
        getDs().save(nested);

        final WithNested found;
        try {
            getDs().createQuery(WithNested.class)
                                            .field("nested.field").equal("nested value")
                                            .get();
            Assert.fail("Querying against an interface should fail validation");
        } catch (Exception ignore) {
            // all good
        }
        found = getDs().createQuery(WithNested.class)
                                        .disableValidation()
                                        .field("nested.field").equal("nested value")
                                        .get();
        Assert.assertNotNull(found);
        Assert.assertEquals(nested, found);
    }

What am I missing?

@agiannone
Copy link
Author

Ok, I've found the issue. If you modify the following line in the test case:

found = getDs().createQuery(WithNested.class)
                                        .disableValidation()
                                        .field("nested.field.fails").equal("nested value")
                                        .get();

You'll notice that the there is a NPE. This is because the following statement is no longer valid:

                if (i >= parts.length) {
                    break;
                }

To solve this, I made a local build which changes the following line in validateQuery():

if (!origProp.substring(0, 1).equals("$")) {

to:

if (validateNames && !origProp.substring(0, 1).equals("$")) {

@agiannone
Copy link
Author

If you add the following line to the WithNested class:

@Indexes({
    @Index( fields={ @Field("nested.field.fail") },
            options = @IndexOptions( disableValidation = true, sparse = true ) )
})

Then call ensureIndexes() on the class, you will get the error from issue: #777

@mthomas87
Copy link

So happy that I finally found this issue. Same NPE on our project.

Caused by: java.lang.NullPointerException: null
at org.mongodb.morphia.query.QueryValidator.canQueryPast(QueryValidator.java:135) ~    [morphia-1.1.0-SNAPSHOT.jar:na]
at org.mongodb.morphia.query.QueryValidator.validateQuery(QueryValidator.java:91) ~[morphia-1.1.0-SNAPSHOT.jar:na]
at org.mongodb.morphia.query.FieldCriteria.<init>(FieldCriteria.java:40) ~[morphia-1.1.0-SNAPSHOT.jar:na]
at org.mongodb.morphia.query.FieldEndImpl.addCriteria(FieldEndImpl.java:288) ~[morphia-1.1.0-SNAPSHOT.jar:na]
at org.mongodb.morphia.query.FieldEndImpl.hasThisOne(FieldEndImpl.java:145) ~[morphia-1.1.0-SNAPSHOT.jar:na]
at org.mongodb.morphia.query.FieldEndImpl.hasThisOne(FieldEndImpl.java:25) ~[morphia-1.1.0-SNAPSHOT.jar:na]</code>

Code looks like this, crashes on last line here:

Query<Editorial> query = this.dao.newQuery();
query.disableValidation();
query.criteria("data.type").equal("gallery");
query.criteria("data.data.id").hasThisOne(galleryId);

Really looking forward to have this fixed. 👍 :

@evanchooly
Copy link
Member

I think i have it. Your suggested fix breaks another case but I think I have it tweaked properly enough to work in both cases. Hopefully a snapshot build later this afternoon to try.

@evanchooly
Copy link
Member

The new -SNAPSHOT build is up if you'd like to give it a try. The test case above and the one listed in #777 both pass now.

@agiannone
Copy link
Author

Two birds, one stone. That resolved both issues =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants