Skip to content

fix: resolves #5550 - make graphql value nullable #5551

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

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

dwerner
Copy link
Contributor

@dwerner dwerner commented Jul 17, 2024

This PR fixes #5550 by making the paused field in the status graphql nullable, which matches the struct field of Option<bool> and allows the indexer-agent to handle the None/null case when appropriate.

@dwerner dwerner requested review from incrypto32 and fordN July 17, 2024 21:30
@dwerner
Copy link
Contributor Author

dwerner commented Jul 17, 2024

@fordN and I had done some work over in the agent on this but found the root issue to be this field. Here is the indexer-agent PR graphprotocol/indexer#958

@incrypto32
Copy link
Member

incrypto32 commented Jul 19, 2024

@dwerner we'd need to address the root cause here, the paused field should never be null, it should either be true or false, The only reason i can see that a null field is possible there is when the deployment_schemas table doesn’t have the subgraph_hash we are looking for, im not sure if thats even possible.
If that is the case then the paused field gets populated with a None.

This is the part of the code where paused gets populated

.filter(ds::subgraph.eq_any(ids))

To address the root cause we'll need to inspect the database if this is reproduced somewhere

@dwerner
Copy link
Contributor Author

dwerner commented Jul 19, 2024

Closing this as a root cause fix is preferred

@dwerner dwerner closed this Jul 19, 2024
@dwerner dwerner reopened this Aug 2, 2024
@dwerner
Copy link
Contributor Author

dwerner commented Aug 2, 2024

In a conversation with @lutter and @fordN, deployments can exist where assignments do not. Then this being bool! fails where an assignment does not exist. So this is the correct way to express this. Just like the node field above it.

Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

Looks good. Could you add comments to the node and paused field in the schema to say that a null means that the deployment is not assigned to an indexing node?

@dwerner dwerner force-pushed the bug/null-paused-value-breaks-status-endpoint branch from 26c8b34 to 35fce71 Compare August 2, 2024 17:45
@dwerner
Copy link
Contributor Author

dwerner commented Aug 2, 2024

image

@lutter's pretty diagram. Thanks for helping me understand what's going on here.

@dwerner dwerner force-pushed the bug/null-paused-value-breaks-status-endpoint branch from 35fce71 to df9b8aa Compare August 2, 2024 19:08
@fordN fordN added the bug Something isn't working label Aug 8, 2024
@dwerner dwerner merged commit e2e6925 into master Aug 9, 2024
7 checks passed
@dwerner dwerner deleted the bug/null-paused-value-breaks-status-endpoint branch August 23, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] paused_at null values lead to error querying status endpoint
4 participants