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

FieldMapper null values, serialization and include_defaults #58823

Closed
romseygeek opened this issue Jul 1, 2020 · 0 comments · Fixed by #61798
Closed

FieldMapper null values, serialization and include_defaults #58823

romseygeek opened this issue Jul 1, 2020 · 0 comments · Fixed by #61798
Labels
:Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team team-discuss

Comments

@romseygeek
Copy link
Contributor

We have a number of field mappers that allow users to set a null_value, for use when a document
does not contain a value for that particular field. In general, the default for this value is null, meaning
don't do anything, but we do not allow null to be set explicitly as this value.

When we serialize a field mapper, we allow users to ask for the default values to be emitted even if they
have not been set. For null_values, this yields a quandary: if we emit the value null, then the serialized
mapping cannot be used in a subsequent put mapping request, because null is not accepted as a
valid value; but if we don't emit anything, then it looks as though we're missing a field even though the
user has asked for all configuration values. #57666 added tests to check serialization and deserialization,
and stopped null values being emitted, but it's not clear what the right choice is.

@romseygeek romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types team-discuss Team:Search Meta label for search team labels Jul 1, 2020
romseygeek added a commit that referenced this issue Sep 1, 2020
Several field mappers have a null_value parameter, that allows you to specify a placeholder
value to insert into a document if the incoming value for that field is null. The default value
for this is always null, meaning "add no placeholder". However, we explicitly bar users from
setting this parameter directly to null (done in #7978, in order to fix an NPE).

This exclusion means that if a mapper is serialized with include_defaults, then we either need
to special-case null_value to ensure that it is not output when it holds the default value, or
we find that the resulting serialized form cannot be used to create a mapping. This stops us
doing some useful generic testing of mappers.

This commit permits null as a parameter value for null_value, and changes the tests to check
that it is a) permissible and b) applied without throwing errors. As part of the testing changes,
a new base class MapperServiceTestCase is refactored from MapperTestCase, holding
the various helper methods related to building mappings but not the single-mapper specific
abstract methods.

Closes #58823
romseygeek added a commit to romseygeek/elasticsearch that referenced this issue Sep 1, 2020
Several field mappers have a null_value parameter, that allows you to specify a placeholder
value to insert into a document if the incoming value for that field is null. The default value
for this is always null, meaning "add no placeholder". However, we explicitly bar users from
setting this parameter directly to null (done in elastic#7978, in order to fix an NPE).

This exclusion means that if a mapper is serialized with include_defaults, then we either need
to special-case null_value to ensure that it is not output when it holds the default value, or
we find that the resulting serialized form cannot be used to create a mapping. This stops us
doing some useful generic testing of mappers.

This commit permits null as a parameter value for null_value, and changes the tests to check
that it is a) permissible and b) applied without throwing errors. As part of the testing changes,
a new base class MapperServiceTestCase is refactored from MapperTestCase, holding
the various helper methods related to building mappings but not the single-mapper specific
abstract methods.

Closes elastic#58823
romseygeek added a commit that referenced this issue Sep 2, 2020
Several field mappers have a null_value parameter, that allows you to specify a placeholder
value to insert into a document if the incoming value for that field is null. The default value
for this is always null, meaning "add no placeholder". However, we explicitly bar users from
setting this parameter directly to null (done in #7978, in order to fix an NPE).

This exclusion means that if a mapper is serialized with include_defaults, then we either need
to special-case null_value to ensure that it is not output when it holds the default value, or
we find that the resulting serialized form cannot be used to create a mapping. This stops us
doing some useful generic testing of mappers.

This commit permits null as a parameter value for null_value, and changes the tests to check
that it is a) permissible and b) applied without throwing errors. As part of the testing changes,
a new base class MapperServiceTestCase is refactored from MapperTestCase, holding
the various helper methods related to building mappings but not the single-mapper specific
abstract methods.

Closes #58823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team team-discuss
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant