Re-Open #764: @Index disableValidation is not used when finding the MappedField

[agiannone]

@evanchooly I just ran our Integration Tests and spotted an issue with the fix. The indexes are indexed correctly when validation is disabled, however the namePath is used to build the key for the index and the namePath will be empty if the findField() method is not called. This means we end up with an empty key which in turn causes save() calls to fail (as it saves the index as _1).

To fix this issue, the following should work:

            if (!"$**".equals(value) && !options.disableValidation()) {
                List<String> namePath = new ArrayList<String>();
                if ( findField(namePath, mc, value) == null) {
                    throw new MappingException(format("Unknown field '%s' for index: %s", value, mc.getClazz().getName()));
                } else {
                    StringBuilder sb = new StringBuilder();
                    for (String s : namePath) {
                        if (sb.length() != 0) {
                            sb.append(".");
                        }
                        sb.append(s);
                    }
                    key = sb.toString();
                }
            }

This way the key doesn't get overwritten by the empty namePath and uses the actual field.value() value as the key.

[evanchooly]

Can you attach a test case and I'll schedule an rc1?

[evanchooly]

Reading through your proposed change, it looks like it will break index definitions on embedded fields.
This change might satisfy both cases:

            if (!"$**".equals(value)) {
                List<String> namePath = new ArrayList<String>();
                final MappedField mappedField = findField(namePath, mc, value);
                if (!options.disableValidation() && mappedField == null) {
                    throw new MappingException(format("Unknown field '%s' for index: %s", value, mc.getClazz().getName()));
                } else {
                    StringBuilder sb = new StringBuilder();
                    for (String s : namePath) {
                        if (sb.length() != 0) {
                            sb.append(".");
                        }
                        sb.append(s);
                    }
                    key = sb.toString();
                }
            }

Can you try that change and see if it works for you? Tests are all green here but I'm curious to hear if it works for you. If you can build from master you can try now or jenkins will push a new snapshot out in an hour or so.

[agiannone]

Which snapshot can I use from Maven to test this?

Once you let me know I'll run our integration tests again to see if everything is working as expected with your proposed fix

[evanchooly]

The latest snapshot should work. There should be another snapshot later today, too.

[agiannone]

Ok, I'm going to run the tests with 1.1.0-SNAPSHOT. I'll get back to you once the results are in.

[agiannone]

That still doesn't work. I now get a NullPointerException on line 1334 of DatastoreImpl. The reason for this is that findField() gets called regardless of whether validation is disabled or not. It then fails to find the relevant MappedFields and either throws an exception or builds the wrong index key.

This is the type of index I am using (in the old format) where the pts object is an Interface:

@Index( value="siteId, pts.pid, -pts.bal", disableValidation = true )

What should happen here is that it skips trying to find the field entirely (disableValidation()) and sets the index key to the field.value() received, in this case for example, pts.pid and -pts.bal.

Does that make sense?

[agiannone]

So i've run the full test suite and it looks like 95% of the time it works. However, the following @Index annotation failed:

@Index( value = "siteId, attrs.type, actv, attrs.bid, attrs.times", sparse = true, disableValidation = true ),
@Index( value = "siteId, actv, attrs.pts.pid, attrs.pts.pts", sparse = true, disableValidation = true ),
@Index( value = "siteId, actv, attrs.bid, attrs.times", sparse = true, disableValidation = true )

When ensuring indexes this throws an error. This is most likely because attrs is an interface.

[evanchooly]

I just tried recreating this but I'm not having much luck. I'm just creating the three indexes you listed against a random class with validation disabled and it's working. If leave validation on, it fails as expected because fields are missing. Do you have a test case I could look at?

[evanchooly]

Can you retry this with the latest -SNAPSHOT build? I just fixed an NPE with validation disabled that might fix your case as well.

[agiannone]

We're back to the situation of the original bug. The following line in the ensureIndex method throws an exception.

            final MappedField mappedField = findField(namePath, mc, value);

The reason is that the index is based on an interface and the findField() will try to find a MappedClass which doesn't exist.

I think the solution would be to do something like this:

        if ( !"$**".equals(value) && !options.disableValidation() ) {
            List<String> namePath = new ArrayList<String>();
            final MappedField mappedField = findField(namePath, mc, value);
            if (mappedField == null) {
                throw new MappingException(format("Unknown field '%s' for index: %s", value, mc.getClazz().getName()));
            } else {
                StringBuilder sb = new StringBuilder();
                for (String s : namePath) {
                    if (sb.length() != 0) {
                        sb.append(".");
                    }
                    sb.append(s);
                }
                key = sb.toString();
            }
        }

This would avoid the NPE on the findField() method and would avoid building the key from an empty namePath (i.e. generating an index with no field like _1 index).

[agiannone]

I actually added a test case here: #881

If you add an index on WithNested you should be able to reproduce the issue above.

[evanchooly]

Lovely, thanks. I'll try to iron this one out then. Maybe put out a 1.1.1.

[agiannone]

If you look at issue #881 there is now a test case which can reproduce this bug :)

Also, if you try the solution from the first post in this issue, it should work without breaking embeddable field definitions. The reason being that if we skip validation, what we are expecting is for Morphia to use the field value we specified with no checks or changes...

[evanchooly]

The latest snapshot build seems to fix this. If you can confirm, we can start looking at 1.1.1 build.

[agiannone]

Resolved