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

Deserializing ID as integer not String #455

Open
ValouBambou opened this issue Aug 21, 2023 · 4 comments · May be fixed by #476
Open

Deserializing ID as integer not String #455

ValouBambou opened this issue Aug 21, 2023 · 4 comments · May be fixed by #476

Comments

@ValouBambou
Copy link

ValouBambou commented Aug 21, 2023

Hi, I'm using an API where ID are integers, not string. Thus, the generated code by the macro is using type ID = String. And it leads to Deserialization error such as Error("invalid type: integer `570721`, expected a string", line: 1, column: 45). Is there a way to specify this to the derive macro ? I'm confused because some of the examples in the documentation are using integers, so it may be possible (EDIT: I realized that the example is confusing because of the hidden context with struct User { id:i32}).

@ValouBambou
Copy link
Author

ValouBambou commented Aug 21, 2023

Maybe a simple fix would be to define ID as something like this instead of simply type ID = String in codegen.rs line 56.

enum ID {
    Int(usize),
    String(String),
}

impl From<usize> for ID {
    fn from(value: usize) -> Self {
        ID::Int(value)
    }
}

impl From<String> for ID {
    fn from(value: String) -> Self {
        ID::String(value)
    }
}

// probably also implement ToString trait

Or maybe we could use i128 instead of usize (or even add another variant with i128) since this might be used according to some parts of the Graphql spec:

GraphQL is agnostic to ID format, and serializes to string to ensure consistency across many formats ID could represent, from small auto‐increment numbers, to large 128‐bit random numbers, to base64 encoded values, or string values of a format like GUID.

I could do a PR to implement that if this is a valid solution (but I'm not sure).

@tomhoule
Copy link
Member

I think I remember (but it's a very far away memory) that IDs are defined as strings by the GraphQL spec. We could have a config option for code generation to change that to another type, however.

@Sytten
Copy link

Sytten commented Aug 22, 2023

They are always passed as string as per the spec. You could propose a feature to parse them, but in general I found this is a bad idea.

ID should be opaque for the client and you cannot assume they will have a particular format. Doing otherwise is a risk long term for maintenance. It is also very easy for a provider to provide a "bad" id by error since everything formats to string.

In any case the proposed enum would be a big nono since it would be a breaking change for almost 100% of existing users.

willfindlay added a commit to willfindlay/graphql-client that referenced this issue Mar 9, 2024
Although not strictly to spec, some upstream graphql implementations such as the start.gg
API encode their ID types as integers rather than strings. Prior to this commit,
deserialization would fail in such cases, since graphql_client simply type aliases the ID
type to a String. This commit introduces a simple deserialization helper that enables us
to handle both integers and strings while maintaining backward compatbility for downstream
users.

Fixes: graphql-rust#455

Signed-off-by: William Findlay <will@isovalent.com>
willfindlay added a commit to willfindlay/graphql-client that referenced this issue Mar 9, 2024
Although not strictly to spec, some upstream graphql implementations such as the start.gg
API encode their ID types as integers rather than strings. Prior to this commit,
deserialization would fail in such cases, since graphql_client simply type aliases the ID
type to a String. This commit introduces a simple deserialization helper that enables us
to handle both integers and strings while maintaining backward compatbility for downstream
users.

Fixes: graphql-rust#455

Signed-off-by: William Findlay <william@williamfindlay.com>
willfindlay added a commit to willfindlay/graphql-client that referenced this issue Mar 9, 2024
Although not strictly to spec, some upstream graphql implementations such as the start.gg
API encode their ID types as integers rather than strings. Prior to this commit,
deserialization would fail in such cases, since graphql_client simply type aliases the ID
type to a String. This commit introduces a simple deserialization helper that enables us
to handle both integers and strings while maintaining backward compatbility for downstream
users.

Fixes: graphql-rust#455

Signed-off-by: William Findlay <william@williamfindlay.com>
@willfindlay
Copy link

Hey, I opened #476 which should fix this in a backwards compatible way.

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 a pull request may close this issue.

4 participants