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.

Sign up for GitHub

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

Jump to bottom

[GraphQL] Add a new multiGetObjects query on Query. #20300

Merged
merged 2 commits into from
Jan 3, 2025
Merged

[GraphQL] Add a new multiGetObjects query on Query. #20300

merged 2 commits into from
Jan 3, 2025
+553 −23

Conversation

stefan-mysten
Copy link
Contributor

@stefan-mysten stefan-mysten commented Nov 18, 2024 •
edited by ronny-mysten
Loading

Description

This PR adds a new query for fetching multiple objects by their ids and versions.

Test plan

Added two new tests to check that the limits work as expected.

  • test_multi_get_objects_query_limits_pass
  • test_multi_get_objects_query_limits_exceeded

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL: Adds a multiGetObjects top level query, which replaces the objectFilter in objects.
  • CLI:
  • Rust SDK:

@stefan-mysten stefan-mysten marked this pull request as draft November 18, 2024 03:04
@vercel Vercel
Copy link

vercel bot commented Nov 18, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 3, 2025 6:00pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jan 3, 2025 6:00pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jan 3, 2025 6:00pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Jan 3, 2025 6:00pm

@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 03:04 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 03:09 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 03:18 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 19, 2024 02:06 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 19, 2024 03:30 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 19, 2024 04:55 — with GitHub Actions Inactive
amnn
amnn requested changes Nov 19, 2024
View reviewed changes
Copy link
Member

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Requesting changes for the query limit checker stuff mainly!

crates/sui-graphql-rpc/schema.graphql Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/schema.graphql Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/config.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs Show resolved Hide resolved
crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/query.rs Outdated Show resolved Hide resolved
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 24, 2024 21:29 — with GitHub Actions Inactive
amnn
amnn reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Just the changes and tests related to query limits checker left, everything else is looking good here!

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs Outdated Show resolved Hide resolved
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 03:31 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten marked this pull request as ready for review December 19, 2024 04:03
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 04:03 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 04:06 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten requested review from wlmyng and amnn December 19, 2024 04:08
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 20:02 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 20, 2024 20:55 — with GitHub Actions Inactive
amnn
amnn approved these changes Dec 22, 2024
View reviewed changes
Copy link
Member

@amnn amnn 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! Can you also add some E2E tests for the happy path (not hitting the limit) before landing?

crates/sui-mvr-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved
crates/sui-mvr-graphql-rpc/src/extensions/query_limits_checker.rs Outdated Show resolved Hide resolved
crates/sui-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved
@stefan-mysten stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env January 3, 2025 17:59 — with GitHub Actions Inactive
@stefan-mysten stefan-mysten merged commit b3b5131 into MystenLabs:main Jan 3, 2025
51 of 54 checks passed
@stefan-mysten stefan-mysten deleted the gql_objects_fix branch January 3, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amnn amnn amnn approved these changes

@emmazzz emmazzz Awaiting requested review from emmazzz emmazzz is a code owner

@suiwombat suiwombat Awaiting requested review from suiwombat suiwombat is a code owner

@wlmyng wlmyng Awaiting requested review from wlmyng wlmyng is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stefan-mysten @amnn @wlmyng