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

[ui] Sensor history: Fix tick ID variable #23802

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

hellendag
Copy link
Member

@hellendag hellendag commented Aug 21, 2024

Summary & Motivation

There is a bug on the sensor tick table where clicking to view a run requested for a tick can show information about an unrelated tick.

This is because sensor tick IDs can be larger than JavaScript's Number.MAX_SAFE_INTEGER, but they're being cast to Number for the GraphQL variable. JS may cast the value to a number that doesn't match the intended value, resulting in the wrong tick ID being sent in the query.

We shouldn't be trying to represent them as number in JS. The GraphQL input type expects it to be a BigInt, and we can just pass a string for that.

How I Tested These Changes

View sensor tick history with ticks that have tick IDs being cast to incorrect values. Verify that the values are now correct, and that when I click to view runs requested for the tick, the query variable and rendered output are correct.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hellendag and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Aug 21, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-gnowzlreh-elementl.vercel.app
https://dish-wrong-tick-id.core-storybook.dagster-docs.io

Built with commit ff263ac.
This pull request is being automatically deployed with vercel-action

@hellendag hellendag requested review from bengotow and salazarm August 21, 2024 20:38
@hellendag hellendag marked this pull request as ready for review August 21, 2024 20:38
Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

nice find

@hellendag hellendag merged commit 9e2898c into master Aug 22, 2024
2 checks passed
@hellendag hellendag deleted the dish/wrong-tick-id branch August 22, 2024 01:51
jmsanders pushed a commit that referenced this pull request Aug 22, 2024
## Summary & Motivation

There is a bug on the sensor tick table where clicking to view a run
requested for a tick can show information about an unrelated tick.

This is because sensor tick IDs can be larger than JavaScript's
`Number.MAX_SAFE_INTEGER`, but they're being cast to `Number` for the
GraphQL variable. JS may cast the value to a number that doesn't match
the intended value, resulting in the wrong tick ID being sent in the
query.

We shouldn't be trying to represent them as `number` in JS. The GraphQL
input type expects it to be a BigInt, and we can just pass a string for
that.

## How I Tested These Changes

View sensor tick history with ticks that have tick IDs being cast to
incorrect values. Verify that the values are now correct, and that when
I click to view runs requested for the tick, the query variable and
rendered output are correct.

(cherry picked from commit 9e2898c)
sryza pushed a commit that referenced this pull request Aug 24, 2024
## Summary & Motivation

There is a bug on the sensor tick table where clicking to view a run
requested for a tick can show information about an unrelated tick.

This is because sensor tick IDs can be larger than JavaScript's
`Number.MAX_SAFE_INTEGER`, but they're being cast to `Number` for the
GraphQL variable. JS may cast the value to a number that doesn't match
the intended value, resulting in the wrong tick ID being sent in the
query.

We shouldn't be trying to represent them as `number` in JS. The GraphQL
input type expects it to be a BigInt, and we can just pass a string for
that.

## How I Tested These Changes

View sensor tick history with ticks that have tick IDs being cast to
incorrect values. Verify that the values are now correct, and that when
I click to view runs requested for the tick, the query variable and
rendered output are correct.
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.

2 participants