-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Handle malformed date when objects are supplied #110049
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
Handle malformed date when objects are supplied #110049
Conversation
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
Hi @salvatore-campagna, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One question.
server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
Show resolved
Hide resolved
refresh: true | ||
index: test | ||
id: "2" | ||
body: { "date_ignored": [ 1, 2, 3 ], "date_not_ignored": "2024-02-03T10:12:43.123Z", date_nanos_ignored: [ 10, 20, 30 ], "date_nanos_not_ignored": "2024-02-03T10:12:43.123456789Z" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvg see this
refresh: true | ||
index: test | ||
id: "3" | ||
body: { "date_ignored": [ { "key": "a", "value": 10 }, { "key": "c", "value": 100 } ], "date_not_ignored": "2024-02-03T10:12:43.123Z", date_nanos_ignored: [ { "key": "b", "value": 20 }, { "key": "d", "value": 40 } ], "date_nanos_not_ignored": "2024-02-03T10:12:43.123456789Z" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvg and this other one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from my side
This reverts commit 579675b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a question about a potential API change, otherwise LGTM
} | ||
return; | ||
} else { | ||
throw new IllegalArgumentException("Unable to parse object as a " + mappedFieldType.name() + " field"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paranoid check - isn't this an API change? Previously we would have returned an error directly from parser which is not only a different text but a different type of exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will double check this...probably I will add also full-bwc
label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AT least I hope this is tested...but probably it is not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea i don't know if we explicitly test such things.
index: | ||
refresh: true | ||
index: test | ||
id: "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that we use the same id as previous successful request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is a mistake.
- match: { hits.hits.0._source.date_nanos_ignored.value: 20 } | ||
- match: { hits.hits.0._source.date_nanos_not_ignored: "2024-02-03T10:12:43.123456789Z" } | ||
|
||
# NOTE: this is interesting...numeric values translate to millis since epoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default format indeed accepts epoch_millis
so these are not malformed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to test a couple of more cases...arrays with valid values and arrays with mixed valid and invalid values.
When an
object
is supplied as a value for a field whose type isdate
ordate_nanos
we needto trigger handling of the field value using our ignore malformed handling strategy.
Resolves #109539