Fixes #4047 - Empty objects are not stored in _source when an include/ex...

[RobCherry]

...clude list is present

Closes #4047

This was originally being tracked by pull request #4048. I closed that pull request because it was sloppy.

I am uncertain about the behavior for handling includes/excludes with empty objects and wildcards, so I added tests to cover the following cases, but please let me know if you think the results are incorrect...

Given

{
  "object1": { },
  "object2": { }
}

And a mapping

{
  ...
  {
    "_source" : {
      "excludes": "object1.*,
      "includes": "object2.*"
    }
  }
}

Results in a filtered source of

{
  "object1": { },
}

However, if given

{
  "object1": {
    "key1": "value1"
   },
  "object2": {
    "key2": "value2"
  }
}

and the same mapping, the filtered source would be

{
  "object2": {
    "key2": "value2"
  }
}

[bleskes]

Hi,

Once again - thx for looking into this. I don't think we should make a distinction between having an obj.* and obj.f as excludes (or includes for that matter) where f is the only field in that object. Also, for excludes, we always make to remove an object that became empty after applying filters to it's content. With includes, we do not return objects if they have no matching properties.

So with this object, I think this is what it should do:

{
   "object1": { "f1": 1},
   "object2": { "f2": 2},
   "object3": { }
}

Then exclude=["object1.*"] should return

{
   "object2": { "f2": 2},
   "object3": { }
}

Also,
include=["object3.*"] should return (because the filters found nothing):

{}

while include=["object3"] returns

{ "object3": {} }

and for your example, with the following object:

{
  "object1": { },
  "object2": { }
}

A mapping with { "excludes": "object1.*, "includes": "object2.*" } should result in:

{}

Makes sense?

[RobCherry]

@bleskes I believe this will take care of the use case you outlined.

[bleskes]

Thanks @RobCherry , will look at it asap.

[bleskes]

nice!

[bleskes]

Hi,

Left some comments. Thanks again for looking into this, I know it can be very tricky to get it right. Also the extra tests are a great addition.

Cheers,
Boaz

[bleskes]

Hi Rob,

Sorry for not looking at this. I was travelling the whole of last week. Back now and will pick it asap.

[RobCherry]

Great. Let me know what you think about #4080 (comment)

[bleskes]

Hi Rob,

I thought about this some more and I do think you have a point. However, that will be a bigger change and it will break backwards compatibility. I think suggest we do this in small steps - first solve the bug you've found and then open another issue to discuss the general behavior and wether we want to change it (with the next major version which is 1.0).

If you agree, here is my idea for solving the bug: let's change the internal filter function ( https://github.com/elasticsearch/elasticsearch/pull/4080/files#diff-85c60dcb441d2ac9b132d73b9b6d2bbbR143 ) to return a bool indicating whether it actually filtered something away. Then we can use the old code, but only skip empty objects if the recursive call actually filtered something. Note that this can happen if some elements did not match an include rule or if one element matched an exclude rule.

[RobCherry]

I have opened #4491.

[bleskes]

Hi Rob,

Do you still want on fixing this issue, while we resolve the discussion about the more general approach?

[RobCherry]

I don't have time to work on this right now, it may be a few weeks before I can circle back. If it is still open when I get time to work on it I would be happy to make the recommended changes.

[bleskes]

closing this one as well, as the issue is resolved: #4047