-
Notifications
You must be signed in to change notification settings - Fork 1
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
FAI-6093: Handle different timestamp types in adapter #137
Conversation
Kudos, SonarCloud Quality Gate passed!
|
@@ -757,7 +760,7 @@ describe('query adapter', () => { | |||
status: {category: 'c1a', detail: 'd1a'} | |||
}, | |||
{ | |||
changedAt: 1667871145261, | |||
changedAt: '2022-11-08T01:32:25.261Z', |
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.
Test a mix of epoch millis and ISO timestamps that are inside embedded object lists are converted to epoch millis.
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.
Thanks!
@@ -172,7 +176,7 @@ export function getFieldPaths( | |||
// In V2, it's stored and typed like every other timestamp. | |||
// We force conversion from ISO 8601 string => epoch millis | |||
// string by overriding the type from string to timestamp. |
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.
comment is out-of-date
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 think it's accurate? This is pointing out an inconsistency in the v1 schema where refreshedAt is serialized as a timestamp, but is typed as a string. If we left the type as a string instead of overriding it, then we wouldn't convert the ISO timestamp to an epoch millis string.
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 think I was being too literal. The override type is now epoch_millis_string
.
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.
Ah. True.
No description provided.