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

FAI-6093: Handle different timestamp types in adapter #137

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

eskrm
Copy link
Contributor

@eskrm eskrm commented May 2, 2023

No description provided.

@sonarcloud
Copy link

sonarcloud bot commented May 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -757,7 +760,7 @@ describe('query adapter', () => {
status: {category: 'c1a', detail: 'd1a'}
},
{
changedAt: 1667871145261,
changedAt: '2022-11-08T01:32:25.261Z',
Copy link
Contributor Author

@eskrm eskrm May 2, 2023

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.

@eskrm eskrm requested review from ted-faros and nat-casey May 2, 2023 22:02
@eskrm eskrm changed the title FAI-6093: Handle different timestamp source types in adapter FAI-6093: Handle different timestamp types in adapter May 2, 2023
Copy link
Contributor

@ted-faros ted-faros left a 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.
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. True.

@eskrm eskrm merged commit a4ec178 into main May 2, 2023
4 checks passed
@eskrm eskrm deleted the es/timestamps branch May 2, 2023 22:26
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants