-
Notifications
You must be signed in to change notification settings - Fork 158
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
Comments
Maybe a simple fix would be to define ID as something like this instead of simply 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
I could do a PR to implement that if this is a valid solution (but I'm not sure). |
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. |
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. |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Hey, I opened #476 which should fix this in a backwards compatible way. |
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 asError("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 withstruct User { id:i32}
).The text was updated successfully, but these errors were encountered: