Allow empty null values for date and IP field mappers #62487
Conversation
romseygeek
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
| @@ -28,61 +28,28 @@ | |||
| import org.apache.lucene.util.BytesRef; | |||
There was a problem hiding this comment.
This also converts IpFieldMapperTests to MapperTestCase because it just makes testing so much easier...
| String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type") | ||
| .startObject("properties").startObject("field").field("type", "ip").endObject().endObject() | ||
| .endObject().endObject()); | ||
| mapper = createDocumentMapper(fieldMapping(b -> { |
There was a problem hiding this comment.
This is the part that is testing the new functionality
|
|
||
| public void testEmptyName() throws IOException { |
There was a problem hiding this comment.
testEmptyName and testSerializeDefaults are done as part of the parent class tests. testMeta in the parent class discovered that IpFieldMapper wasn't correctly serializing its meta param, so the test cutover is definitely worth it!
There was a problem hiding this comment.
@romseygeek and I talked about a general question I was having. At least on Masters DateFieldMapper, the “null_value” Parameter is initialized as non-updateable. From the issue underlying the PR I thought the idea for the user updating from an old cluster with “bad” null_value is to set it to “”, so the idea now is to to log a warning for "bad" formatted null_values if the index created version is < 8x, and reject otherwise. Please correct me if I'm missing sth.
server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java
Outdated
Show resolved
Hide resolved
|
I've updated the PR to take into account @cbuescher 's points. We now fail for indexes created in 8x, but emit a warning and treat the badly-formed null values as null in 7x. |
| @Override | ||
| protected List<Parameter<?>> getParameters() { | ||
| return List.of(indexed, hasDocValues, stored, ignoreMalformed, nullValue); | ||
| return List.of(indexed, hasDocValues, stored, ignoreMalformed, nullValue, meta); |
There was a problem hiding this comment.
I guess the missing "meta" was discovered by the test change?
In #57666 we changed when null_value was parsed for ip and date fields. Previously, the null value was stored as a string, and parsed into a date or InetAddress whenever a document containing a null value was encountered. Now, the values are parsed when the mappings are built, which means that bad values are detected up front; if you try and add a mapping with a badly-parsed ip or date for a null_value, the mapping will be rejected. This causes problems for upgrades in the case when you have a badly-formed null_value in a pre-7.9 cluster. This commit fixes the upgrade case by changing the logic to only logging a warning on the badly formed value, replicating the earlier behaviour. Fixes #62363
In #57666 we changed when null_value was parsed for ip and date fields. Previously, the null value was stored as a string, and parsed into a date or InetAddress whenever a document containing a null value was encountered. Now, the values are parsed when the mappings are built, which means that bad values are detected up front; if you try and add a mapping with a badly-parsed ip or date for a null_value, the mapping will be rejected. This causes problems for upgrades in the case when you have a badly-formed null_value in a pre-7.9 cluster. This commit fixes the upgrade case by changing the logic to only logging a warning on the badly formed value, replicating the earlier behaviour. Fixes #62363
In #57666 we changed when
null_valuewas parsed for ip and date fields. Previously,the null value was stored as a string, and parsed into a date or InetAddress whenever
a document containing a null value was encountered. Now, the values are parsed when
the mappings are built, which means that bad values are detected up front; if you try and
add a mapping with a badly-parsed ip or date for a null_value, the mapping will be
rejected.
This causes problems for upgrades in the case when you have a badly-formed null_value
in a pre-7.9 cluster. This commit fixes the upgrade case by changing the logic to only
fail on indexes created in 8x and later. For earlier indexes, we log a warning on the
badly formed value and ignore it, replicating the earlier behaviour.
Fixes #62363