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.

Sign up for GitHub

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

Jump to bottom

Using default:null for _timestamp field creates a index loss on restart #9233

Closed
wants to merge 1 commit into from
Closed

Using default:null for _timestamp field creates a index loss on restart #9233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/elasticsearch/cluster/metadata/MappingMetaData.java
View file
Open in desktop
Expand Up @@ -354,7 +354,7 @@ private void initMappers(Map<String, Object> withoutType) {
path = fieldNode.toString();
} else if (fieldName.equals("format")) {
format = fieldNode.toString();
} else if (fieldName.equals("default")) {
} else if (fieldName.equals("default") && fieldNode != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the same NPE possible above in all the other conditions? Should we just continue the loop before these checks?

if (fieldNode == null) continue;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. Though I think null values might be rejected at mapping creation time for other values.
But as it's a safer option, I'll modify the PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original reporter here who discovered this issue after using _timestamp with default=null.

For "default" field in particular, null actually means something (which is why i used it) - to ignore the normal _timestamp default of now if _timestamp is not provided, thereby making _timestamp (or the field providing it via path property) a required field. seems like "default" property needs separate handling from other fields that might inadvertently have been passed a null - as in this case passing a null has a meaning (that being: clear any default behavior that the _timestamp should have).

From _timestamp doc, section on Default.

By default, the default value is now which means the date the document was processed by the indexing chain.

You can disable that default value by setting default to null. It means that timestamp is mandatory:

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw -in the case where you inject a record without provided path property being there, and have set default=null, you'll properly get this error, as expected per the documentation quoted above.

{
    "error": "TimestampParsingException[failed to parse timestamp [timestamp is required by mapping]]",
    "status": 500
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic subscribing to what is described in the docs (ignoring null assigned to any other properties) would be more like this... making defaultTimestamp null if so, else the string provided.

else if (fieldName.equals("default")) {
      defaultTimestamp = (fieldNode == null ? null : fieldNode.toString());
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 1.5 we will change this logic: see #9104

defaultTimestamp = fieldNode.toString();
}
}
Expand Down
16 changes: 16 additions & 0 deletions src/test/java/org/elasticsearch/index/mapper/timestamp/TimestampMappingTests.java
View file
Open in desktop
Expand Up @@ -606,6 +606,22 @@ public void testMergingNullValues() throws Exception {
assertThat(mergeResult.hasConflicts(), is(true));
}

/**
* Test for issue #9223
*/
@Test
public void testInitMappers() throws IOException {
String mapping = XContentFactory.jsonBuilder().startObject()
.startObject("type")
.startObject("_timestamp")
.field("enabled", true)
.field("default", (String) null)
.endObject()
.endObject().endObject().string();
// This was causing a NPE
new MappingMetaData(new CompressedString(mapping));
}

@Test
public void testMergePaths() throws Exception {
String[] possiblePathValues = {"some_path", "anotherPath", null};
Expand Down