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

GraphQL: Fix the long literal passed into the nested variable #26730

Closed
wants to merge 1 commit into from
Closed

GraphQL: Fix the long literal passed into the nested variable #26730

wants to merge 1 commit into from

Conversation

Lucshine
Copy link

This commit fixes a bug in graphql when querying with variables and having nested types in variables.

Expected behaviour

curl 'http://localhost:8545/graphql' --data-raw $'{"query":"query ($filter: FilterCriteria\u0021) { logs(filter: $filter) { transaction { hash } }}","variables":{"filter":{"addresses":["0x1f9840a85d5aF5bf1D1762F925BDADdC4201F984"],"fromBlock":16666666,"toBlock":16666666}}}' 

# {"data":{"logs":[{"transaction":{"hash":"0x6007bf4d384fdcf8c953d3c072bab427cb39dd849c8c59fc4555ee9d259c423f"}},{"transaction":{"hash":"0x94f2ebc630936fe30ebe8861bfa5ca424b8e236aa1f885b8282be40cac760987"}}]}}

Actual behaviour

# {"errors":[{"message":"unexpected type float64 for Long"}],"data":{}}

@rjl493456442
Copy link
Member

I don't get it. Where does float come from?

@Lucshine
Copy link
Author

I don't get it. Where does float come from?

Because the parameters in the variables section are passed in json format, in the golang json library parser, an unquoted numeric literal is always parsed as float64.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Technically, this "makes sense" to me. However, you are adding a non-quoted decoding-format for uint64, and that might not be according to spec.

Typically, anything which can become "very large number" needs to be a quoted string, since javascript only have 53 bit integers natively.

So the implementation looks good to me, but I don't want us to introduce a spec-violation, need to look into that.

@holiman
Copy link
Contributor

holiman commented Feb 20, 2023

According to spec, https://eips.ethereum.org/EIPS/eip-1767

# BigInt is a large integer. Input is accepted as either a JSON number or as a string.
# Strings may be either decimal or 0x-prefixed hexadecimal. Output values are all
# 0x-prefixed hexadecimal.
scalar BigInt
# Long is a 64 bit unsigned integer.
scalar Long
# Block is an Ethereum block.
type Block {
    # Number is the number of this block, starting at 0 for the genesis block.
    number: Long!

Fo the type BigInt, the spec is very explicit about the input/output. For the type Long, not so much.

@fjl
Copy link
Contributor

fjl commented Feb 20, 2023

GraphQL requests are parsed here:

if err := json.NewDecoder(r.Body).Decode(&params); err != nil {

Instead of trying to make it work with float64, we could also call UseNumber on the decoder before using it. That would allow arbitrary-size number inputs, though we'd need to handle them at the graphql decoding level.

@s1na
Copy link
Contributor

s1na commented Mar 16, 2023

The problem is not about variables being nested. It's that some arguments have the type hexutil.Uint64 which doesn't accept decimal numbers. I think we should not change this type because it's used elsewhere and rather change those arguments to use graphql.Long and update Long to accept hex. I've done this in #26984.

@holiman
Copy link
Contributor

holiman commented Mar 22, 2023

Closing this in favour of #26984.

(If I misunderstood something, please let me know)

@holiman holiman closed this Mar 22, 2023
@fjl
Copy link
Contributor

fjl commented Mar 22, 2023

@holiman #26984 does not exist

@holiman
Copy link
Contributor

holiman commented Mar 22, 2023 via email

@Lucshine Lucshine deleted the graphql/nested-long-var branch March 24, 2023 02:05
@Lucshine Lucshine restored the graphql/nested-long-var branch March 24, 2023 02:06
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.

5 participants