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

fix: Store validateArgs wrongfully overwriting start, end unix time #24741

Open
wants to merge 3 commits into
base: main-2.x
Choose a base branch
from

Conversation

paulheg
Copy link

@paulheg paulheg commented Mar 7, 2024

When querying data before 1970-01-01 (UNIX time 0) validateArgs would set start to -in64 max and end to int64 max.

Closes #24669

Changes made:
Removed two faulty if statements.

  • I've read the contributing section of the project README.
  • Signed CLA (if not already signed).

When querying data before 1970-01-01 (UNIX time 0) validateArgs would set start to -in64 max and end to int64 max.
@paulheg paulheg changed the title fix: Store validateArgs wronfully overwriting start, end unix time fix: Store validateArgs wrongfully overwriting start, end unix time Mar 7, 2024
@philjb
Copy link
Contributor

philjb commented Mar 7, 2024

@gwossum - if you have a moment, could you look at this potential bug fix from @paulheg? See the bug report for details. The PR does seem to have targeted the impact code where it is replacing any timestamp less than or equal to 0 aka 1970-01-01 with the smallest signed int (plus 2).

I think the code was intending to do this (similar issue for end)

	if start < models.MinNanoTime {
		start = models.MinNanoTime
	}

as math.MinInt64 and math.MinInt64 + 1 are apparently used as sentinels. @paulheg - I think you need to adjust your fix. Check out the comment on models.MinNanoTime.

This bug seems to be encountered only for queries looking before 1970 so it is rather unlikely to be encountered in the wild, except for @paulheg here!

…ls.MinNanoTime or higher than models.MaxNanoTime
@paulheg
Copy link
Author

paulheg commented Mar 7, 2024

@philjb with the comment of models.MinNanoTime it makes a lot more sense what was going on there.
I updated the pr.

@paulheg
Copy link
Author

paulheg commented Mar 7, 2024

I just added some tests to check this behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants