Respect flexible field access pattern in geoip and ip_location processors

[flash1293]

Currently in flexible field access mode, the geoip processor writes such an object:

{
  "geoip": {
    "field": "ip",
    "target_get": "my.field"
  }
}

results in

{
  "my.field": {
     "city": "...",
     "country": "...",
     "location": {
        "lat": "...",
        "lon": "..."
     }
  }
}

This is problematic because we go from dotted to subobject, which means that in subsequent processing steps, e.g. my.field.country can't be accessed anymore.

To align things, in flexible field access mode the geoip processor now writes the individual parts as individual properties:

{
  "my.field.city: "...",
  "my.field.country": "...",
   "my.field.location": [..., ...]
}

The same applies to the ip_location processor.

[elasticsearchmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[flash1293]

I extended this a bit - it's now also flattening the location geo point - this helps in some mapping constellations we have today and is also more consistent since it flattens out everything and doesn't introduce inaccessible sub-objects in any place.

[dakrone]

LGTM, I left a couple of comments about typos, but nothing major. We do need to update the documentation to document how the processor behaves in the flexible field access pattern however.

[dakrone]

Minor typo in the doc

Suggested change

	// Convert location from map {lat, lon} to array [lat, lon] in flexible mode
	// Convert location from map {lat, lon} to array [lon, lat] in flexible mode

[dakrone]

Suggested change

	* is written as an array [lat, lon] for better compatibility with geo_point fields in flexible mode.
	* is written as an array [lon, lat] for better compatibility with geo_point fields in flexible mode.

[dakrone]

Is there a reason you used notNullValue here instead of the actual resolved longitude and latitude (to ensure they are in the right order)?

[flash1293]

Thanks for the review @dakrone , adjusted

[joegallo]

Github isn't letting me put this comment where I want to. 😠

I think you still have to handle the dataList case -- see document.setFieldValue(targetField, dataList); below.

It's also possible that you totally discussed this already and I haven't read the appropriate description/comments, I just jumped straight into the code.

[flash1293]

we didn't, I added the multi-value case. Let me know whether this behavior makes sense

[joegallo]

++, I'm fine with the code of the handling of the dataList case. Nice work!

I think you should probably have somebody more familiar with FLEXIBLE mode review the semantics of the way you're handling the dataList case, though, too. @dakrone would be fine, or @jbaiera, for example.

[joegallo]

I added a tidiness commit, I don't think it's controversial, but we can revert it if I'm wrong. 🤷

[joegallo]

This is kind of an annoying thing, but this will end up resulting in the targetField.location field value being immutable -- if you look at the non-FLEXIBLE side of the house, everything is HashMaps all the way down, so that users can turn around and fuss with these things afterwards (with other ordinary processors or a script processor or whatever).

I don't necessarily think a customer is going to take advantage of this, but if somebody tried, I imagine they'd be annoyed that we've taken it away from them.

Perhaps use an ArrayList instead? You lose the nice constructor call, though, but that's nothing a quick static method couldn't fix.

[flash1293]

switched to arraylist

[joegallo]

I think it might make sense to assert here that the value is not an instanceof Map. My reasoning is that although all the current databases currently return flat maps except for the location, some future PR could add a new map-valued key and then this code would (as written) very quietly start doing the wrong thing.

It seems like it would be less trappy for us to alert that future developer at dev-time as soon as they go to run some tests of their new code.

[flash1293]

Good idea, added

[joegallo]

Unless anything else jumps into my mind while I'm brewing tea or whatever, I think I've hit all my requests for changes in my previous comments.

Overall this PR looks pretty dang good, but I think we can make it just a little better in a few places, and I do think the dataList issue is a real one.

[flash1293]

Thanks @joegallo , addressed your points, could you take another look?

[joegallo]

So let's say the lat or lon (or both) are null. Are you okay with what happens then?

I'm on the fence about what to do about it -- part of me thinks that the null checks are making this worse, and you should just always go down the return newMutableLocationList(...) path.

[flash1293]

Yeah, not sure, that's probably very edge-casey

[joegallo]

I get that it's an edge case, but it seems like the point of the flexible access pattern is not to emit nested maps, and yet that's what the code would do if either of the things were null (at least by my read of things).

Even if it's never going to come up in practice, the code is capturing our intent of what would happen if it did, so should it:

• emit a map? (current behavior), seems contrary to the spirit of the flexible access pattern
• emit a list with one or more null elements?
• throw an exception?

[flash1293]

Agreed, let's go with the null elements, seems like the best behavior

[elasticsearchmachine]

Hi @flash1293, I've created a changelog YAML for you.

[joegallo]

LGTM! Given that it's a weekend and a holiday, I'll smash that merge button for you on Monday morning assuming I don't hear otherwise from you before then. I had some trouble with the backport machinery on a different recent PR, so if the backport doesn't go through automatically, I'll handle that then, too.

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	9.3