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

feat: limit functions exposed in indexing status API #55

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

hopeyen
Copy link
Collaborator

@hopeyen hopeyen commented Sep 27, 2023

limit indexing status to serve supported root fields: indexingStatuses, publicProofsOfIndexing, entityChangesInBlock, blockData, cachedEthereumCalls, subgraphFeatures, apiVersions

So queries with root fields like indexingStatusForCurrentVersion, indexingStatusForPendingVersion, indexingStatusesForSubgraphName, ... should fail with bad request with a hint on the unsupported fields

Using Regex is brutal but get the job done while I didn't find similar functions in graphql crates like async-graphql and graphql-parser (also searched in graphql-tools-rs maintained by the guild.

Resolve #6

@hopeyen hopeyen added size:medium Medium p3 Low priority type:feature New or enhanced functionality labels Sep 27, 2023
@hopeyen hopeyen requested a review from aasseman September 27, 2023 02:17
@hopeyen hopeyen self-assigned this Sep 27, 2023
@hopeyen hopeyen force-pushed the hope/limit-status-api branch from 478befd to 18882a1 Compare September 27, 2023 02:18
@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2023

Pull Request Test Coverage Report for Build 6332462657

  • 71 of 85 (83.53%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.3%) to 46.49%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/graphql.rs 71 72 98.61%
service/src/server/routes/status.rs 0 13 0.0%
Totals Coverage Status
Change from base Build 6331848844: 1.3%
Covered Lines: 1265
Relevant Lines: 2721

💛 - Coveralls

aasseman
aasseman previously approved these changes Sep 27, 2023
Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

Simple and effective solution.

I think graphql_parser could have worked, but it would have been up to you to write a graph visitor to look for all the fields of the generated AST. More complex to write though, and probably worth it only if there's a need for a fancier GQL manipulations.

service/src/server/routes/status.rs Outdated Show resolved Hide resolved
@hopeyen
Copy link
Collaborator Author

hopeyen commented Sep 27, 2023

@aasseman Thank you for the review! I've updated to use lazy_static.

I think graphql_parser could have worked, but it would have been up to you to write a graph visitor to look for all the fields of the generated AST

I agree, was well on my way in handling different GraphQL executions with graphql_parser and wanted to use it for a better error messages, but became complicated with the nested types and enums. perhaps we can submit a feature request instead of making it ourselves

@aasseman aasseman self-requested a review September 28, 2023 00:57
Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

:shipit:

@hopeyen hopeyen merged commit 50fb34e into main Sep 28, 2023
4 checks passed
@hopeyen hopeyen deleted the hope/limit-status-api branch September 28, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3 Low priority size:medium Medium type:feature New or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Limit indexing status API
2 participants