-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Convert timestamp before passing to validation #192379
Convert timestamp before passing to validation #192379
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
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.
code changes LGTM 👍
Would be nice to have a unit test for pagerduty
and servicenow_itom
.
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.
LGTM, but left a comment about the JSON.stringify(parsedTimestamp) === trimmedTimestamp)
- I'm guessing it's not needed, but ... not 100% sure ...
if (trimmedTimestamp.length > 0) { | ||
const parsedTimestamp = parseInt(trimmedTimestamp, 10); | ||
|
||
if (!isNaN(parsedTimestamp) && JSON.stringify(parsedTimestamp) === trimmedTimestamp) { |
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 guess this is for the case of 123_but_not_a_date
not returning 123? Good catch!
Note however:
parseInt('123.0') === 123
So that would fail. I think I usually just deal with the prefix, don't do that sort of check.
Poked around the Kibana repo looking at other parseInt()
calls - trying to find some parsing "human" data - and I'm not seeing anyone else making other checks. Of course, folks may not generally know that parseInt()
will stop once it hits a non-numeric character and return what it read up to that.
Do you think there's a need for us to do extra checking here?
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.
Yes you are right it is to avoid converting the strings starting with numbers.
For date inputs we should only convert the epoch numbers passed as string, all the rest should remain as string. Epoch numbers are always integer so JSON.stringify(parsedTimestamp) === trimmedTimestamp
check guarantees that.
'123.0'
would remain as string '123.0'
and moment validation would return false for it, as expected.
And if we don't do that check 2024-10-10
(valid date string) would be converted to 2024
and lead us to a bug :)
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @ersin-erdal |
Resolves: elastic#186114 This PR adds a utility function to `stack_connectors` plugin to convert given date string or number (epoch) to proper type before passing to Date object or validation functions. (cherry picked from commit 48de1a5)
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.x`: - [Convert timestamp before passing to validation (#192379)](#192379) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Ersin Erdal","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-13T21:48:05Z","message":"Convert timestamp before passing to validation (#192379)\n\nResolves: #186114\r\n\r\nThis PR adds a utility function to `stack_connectors` plugin to convert\r\ngiven date string or number (epoch) to proper type before passing to\r\nDate object or validation functions.","sha":"48de1a57e726f570feefe37e211d0b79033b1b75","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:ResponseOps","v9.0.0","v8.16.0"],"title":"Convert timestamp before passing to validation","number":192379,"url":"https://github.com/elastic/kibana/pull/192379","mergeCommit":{"message":"Convert timestamp before passing to validation (#192379)\n\nResolves: #186114\r\n\r\nThis PR adds a utility function to `stack_connectors` plugin to convert\r\ngiven date string or number (epoch) to proper type before passing to\r\nDate object or validation functions.","sha":"48de1a57e726f570feefe37e211d0b79033b1b75"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192379","number":192379,"mergeCommit":{"message":"Convert timestamp before passing to validation (#192379)\n\nResolves: #186114\r\n\r\nThis PR adds a utility function to `stack_connectors` plugin to convert\r\ngiven date string or number (epoch) to proper type before passing to\r\nDate object or validation functions.","sha":"48de1a57e726f570feefe37e211d0b79033b1b75"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Ersin Erdal <[email protected]>
Resolves: #186114
This PR adds a utility function to
stack_connectors
plugin to convert given date string or number (epoch) to proper type before passing to Date object or validation functions.