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

stefan-mysten · 2024-11-18T03:04:33Z

Description

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

Test plan

How did you test the new or updated feature?

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.

vercel · 2024-11-18T03:04:43Z

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	Nov 19, 2024 4:57am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Nov 19, 2024 4:57am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Nov 19, 2024 4:57am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Nov 19, 2024 4:57am

crates/sui-graphql-rpc/src/types/query.rs

crates/sui-graphql-rpc/src/types/object.rs

stefan-mysten · 2024-11-18T03:09:20Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

+        let limits = self.reporter.limits;
+        let mut stack = vec![&value.node];
+        while let Some(value) = stack.pop() {
+            match value {


Need to handle V::Variable

wlmyng · 2024-11-18T18:49:31Z

crates/sui-graphql-rpc/schema.graphql

@@ -2882,7 +2882,7 @@ input ObjectFilter {
 	"""
 	Filter for live or potentially historical objects by their ID and version.
 	"""
-	objectKeys: [ObjectKey!]
+	objectKeys: [ObjectKey!] @deprecated(reason: "Use multiGetObjects instead. This field will be removed in a future release.")


does this need to be updated again since you mentioned async-graphql doesnt work with this directive?

circling back, then we're losing the ability to select objects that match additional criteria among a list of objects (and optional versions)

crates/sui-graphql-rpc/src/config.rs

wlmyng · 2024-11-18T19:13:38Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

+    /// multiGet queries can only pass a number of keys that does not exceed the max page size.
+    /// This checks for the number of keys and deducts the raw size of the keys from the payload
+    /// size.
+    fn check_multiget_arg(


As multi-get functionality is achieved through connections, we do a pretty simple assumption that we will yield connection_page_size results, which is the default_page_size unless an override has been specified by the caller. I wonder if we could do something similar here, assume that a particular multi-get if invoked will yield the maximum allowed number of results?

Then we can do something similar, where in the resolver (such as TransactionBlock::paginate) we have the actual check for ids.len() > limits.max_multiget -> return Error

How can I tell multi-get functionality is achieved through connections?

This query does not return a page, but returns the number of keys passed in, which cannot be larger than max page size.

Sorry for the confusion, what I meant is that today to do multi-get you go through objects connection, which assumes that we'll yield default_page_size results unless this is explicitly overwritten with first or last. Wonder if we could simplify by making a similar assumption, if we see multiGet, then we defer to the default or max limit for that multiGet

I see. But this function still needs to account for those input object keys and remove them from the query limit. So I am not sure what you're thinking of.

amnn

Requesting changes for the query limit checker stuff mainly!

amnn · 2024-11-19T13:29:38Z

crates/sui-graphql-rpc/schema.graphql

@@ -2880,7 +2880,8 @@ input ObjectFilter {
 	"""
 	objectIds: [SuiAddress!]
 	"""
-	Filter for live or potentially historical objects by their ID and version.
+	Filter for live objects by their ID and version.
+	For historical versions, use multiGetObjects query instead.


We do want to deprecate this input, it doesn't make a ton of sense to query live objects by their ID and version. Add a deprecation notice here (it can just be in the doc comment if the @deprecated support isn't there in async-graphql) that mentions the following:

Why this is being deprecated (it doesn't mean what people think it means).

What to use instead.

When it's going away (earliest date and major version)

Check out other deprecation notices in the schema for examples:

sui/crates/sui-graphql-rpc/schema.graphql

Lines 4344 to 4359 in a25c0cb

"""

Limit to transactions that accepted the given object as an input. NOTE: this input filter

has been deprecated in favor of `affectedObject` which offers an easier to under behavior.

This filter will be removed with 1.36.0 (2024-10-14), or at least one release after

`affectedObject` is introduced, whichever is later.

"""

inputObject: SuiAddress

"""

Limit to transactions that output a versioon of this object. NOTE: this input filter has

been deprecated in favor of `affectedObject` which offers an easier to understand behavor.

This filter will be removed with 1.36.0 (2024-10-14), or at least one release after

`affectedObject` is introduced, whichever is later.

"""

changedObject: SuiAddress

amnn · 2024-11-19T13:31:16Z

crates/sui-graphql-rpc/schema.graphql

@@ -3399,6 +3400,10 @@ type Query {
 	- `author` is the address of the signer of the transaction or personal msg.
 	"""
 	verifyZkloginSignature(bytes: Base64!, signature: Base64!, intentScope: ZkLoginIntentScope!, author: SuiAddress!): ZkLoginVerifyResult!
+	"""


nit: can you move the implementation of this up near the other accessor functions so it shows up in that order in the schema? Let's put multi-gets between the point lookups and the rich paginated queries (so at the moment, between fn transaction_block and fn coins.

amnn · 2024-11-19T13:34:21Z

crates/sui-graphql-rpc/src/config.rs

        Self {
            max_query_depth: 20,
            max_query_nodes: 300,
            max_output_nodes: 100_000,
            max_query_payload_size: 5_000,
            max_db_query_cost: 20_000,
            default_page_size: 20,
-            max_page_size: 50,
+            max_page_size,
+            max_multi_get_objects_keys: max_page_size, // keep the same as max_page_size


Let's not make this the same as max_page_size, the reason we factored it out into its own config is that it can/should be different -- we can start by setting this to 500 or something.

Let's move it next to max_transaction_ids below, because that's what it is semantically closest to.

amnn · 2024-11-19T13:34:43Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

@@ -67,6 +68,9 @@ struct LimitsTraversal<'a> {
    input_budget: u32,
    output_budget: u32,
    depth_seen: u32,
+
+    // The total number of object keys  in multiGetObjects query
+    num_obj_keys_seen: u32,


I haven't seen how this is used, but you probably don't need this.

(I was right, you don't need this).

amnn · 2024-11-19T13:36:03Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

+        match &item.node {
+            Selection::Field(f) => {
+                let name = &f.node.name.node;
+                if name.starts_with(MULTI_GET_OBJECTS) {


The idea was for support in the query limits checker to be fully generic, so that in future, we could add multi gets without making further changes here (so we wouldn't forget to add them). To achieve that we should look for names that start with "multiGet".

amnn · 2024-11-19T13:43:04Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

+                // If the field being traversed is a multiGet query, multiplicity is the number of
+                // keys seen in the request.


No, it is the number of keys passed in the parameter to this multi-get query multiplied by the existing multiplicity (similar to how page size works).

amnn · 2024-11-19T13:45:37Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

Let's write a lot of tests for this:

For the potential panic introduced by the expect.

For how multiplicity should be handled for multi-gets.

If it's not clear what the limit check change is supposed to be doing, let's discuss during pair programming.

amnn · 2024-11-19T13:46:22Z

crates/sui-graphql-rpc/src/extensions/query_limits_checker.rs

@@ -425,6 +529,10 @@ impl<'a> LimitsTraversal<'a> {
                        .ok_or_else(|| self.output_node_error())?
                };

+                if multiplicity > self.output_budget {


This change should not be necessary

amnn · 2024-11-19T13:47:37Z

crates/sui-graphql-rpc/src/types/object.rs

@@ -883,6 +884,40 @@ impl Object {
        }
    }

+    /// Fetch objects by their id and version. If you need to query for live objects, use the


nit: ordering, I think we put the query functions above the paginate functions usually.

amnn · 2024-11-19T13:48:51Z

crates/sui-graphql-rpc/src/types/query.rs

+        if object_keys.len() > cfg.limits.max_multi_get_objects_keys as usize {
+            return Err(Error::Client(format!(
+                "Number of keys exceeds max limit of '{}'",
+                cfg.limits.max_multi_get_objects_keys
+            ))
+            .into());
+        }


this check is being duplicated here and in the query limits check. I think it's fine/good to have it done here -- that mirrors what we do for pages, but let's keep it down to one place where we do the check.

stefan-mysten added 2 commits November 17, 2024 12:27

WIP

b23ff07

Add query limits check

52a419e

stefan-mysten requested review from amnn, wlmyng, emmazzz and suiwombat as code owners November 18, 2024 03:04

stefan-mysten marked this pull request as draft November 18, 2024 03:04

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 03:04 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 18, 2024 03:06 View deployment

stefan-mysten commented Nov 18, 2024

View reviewed changes

crates/sui-graphql-rpc/src/types/query.rs Show resolved Hide resolved

stefan-mysten commented Nov 18, 2024

View reviewed changes

crates/sui-graphql-rpc/src/types/object.rs Outdated Show resolved Hide resolved

Add deprecate macro on the schema

43f889e

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 03:09 — with GitHub Actions Inactive

stefan-mysten commented Nov 18, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs November 18, 2024 03:10 View deployment

Fix clippy

0246208

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 03:18 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 18, 2024 03:19 View deployment

wlmyng reviewed Nov 18, 2024

View reviewed changes

Address feedback

db4c397

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 19, 2024 02:06 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 19, 2024 02:07 View deployment

stefan-mysten added 2 commits November 18, 2024 18:10

Fix config

765f667

Fix config

2cf6f86

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 19, 2024 03:30 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 19, 2024 03:32 View deployment

Fix staging

47bb848

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env November 19, 2024 04:55 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 19, 2024 04:57 View deployment

amnn requested changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stefan-mysten commented Nov 18, 2024 •

edited

Loading

vercel bot commented Nov 18, 2024 •

edited

Loading

stefan-mysten Nov 18, 2024

wlmyng Nov 18, 2024

wlmyng Nov 18, 2024

wlmyng Nov 18, 2024

stefan-mysten Nov 19, 2024

wlmyng Nov 19, 2024

stefan-mysten Nov 19, 2024

amnn left a comment

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

amnn Nov 19, 2024

	"""
	Limit to transactions that accepted the given object as an input. NOTE: this input filter
	has been deprecated in favor of `affectedObject` which offers an easier to under behavior.

	This filter will be removed with 1.36.0 (2024-10-14), or at least one release after
	`affectedObject` is introduced, whichever is later.
	"""
	inputObject: SuiAddress
	"""
	Limit to transactions that output a versioon of this object. NOTE: this input filter has
	been deprecated in favor of `affectedObject` which offers an easier to understand behavor.

	This filter will be removed with 1.36.0 (2024-10-14), or at least one release after
	`affectedObject` is introduced, whichever is later.
	"""
	changedObject: SuiAddress

		// If the field being traversed is a multiGet query, multiplicity is the number of
		// keys seen in the request.

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

Are you sure you want to change the base?

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

Conversation

stefan-mysten commented Nov 18, 2024 • edited Loading

Description

Test plan

Release notes

vercel bot commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

stefan-mysten commented Nov 18, 2024 •

edited

Loading

vercel bot commented Nov 18, 2024 •

edited

Loading