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

Implement custom scalar for u8 type #862

Open
kunjee17 opened this issue Jan 26, 2021 · 19 comments
Open

Implement custom scalar for u8 type #862

kunjee17 opened this issue Jan 26, 2021 · 19 comments
Assignees
Labels
k::documentation Related to project documentation support
Milestone

Comments

@kunjee17
Copy link
Contributor

Might be totally dumb question. But I couldn't make it work.

Here is my code for making Custom Scalar for u8 type.

#[juniper::graphql_scalar(description = "u8")]
impl<S> GraphQLScalar for u8 where S:ScalarValue {
    fn resolve(&self) -> Value {
        Value::scalar(self.to_string())
    }

    fn from_input_value(v : &InputValue) -> Option<u8> {
        v.as_string_value().and_then(|s| s.parse::<u8>().ok())
    }

    fn from_str(value: ScalarToken) -> ParseScalarResult<S> {
        if let ScalarToken::String(value) = value {
            Ok(S::from(value.to_owned()))
        } else {
            Err(ParseError::UnexpectedToken(Token::Scalar(value)))
        }
    }
}

But I m getting error

error[E0210]: type parameter `__S` must be used as the type parameter for some local type (e.g., `MyStruct<__S>`)
 --> server/src/custom_scalars/custom_u8.rs:4:1
  |
4 | #[juniper::graphql_scalar(description = "u8")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `__S` must be used as the type parameter for some local type
  |
  = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
  = note: only traits defined in the current crate can be implemented for a type parameter
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0210]: type parameter `S` must be used as the type parameter for some local type (e.g., `MyStruct<S>`)
 --> server/src/custom_scalars/custom_u8.rs:4:1
  |
4 | #[juniper::graphql_scalar(description = "u8")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type parameter `S` must be used as the type parameter for some local type
  |
  = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
  = note: only traits defined in the current crate can be implemented for a type parameter
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)


I can't figure it out what is wrong in my code. Any help or pointers are more than welcome.

@dzmitry-lahoda
Copy link

have not able to find extended scalars covering Uuid, some forms for chrono time, non negative and positive integers...

@kunjee17
Copy link
Contributor Author

kunjee17 commented Mar 8, 2021

@LegNeato little help over here if possible ? Or point us to direction where we can find something regarding this.

@dyedgreen
Copy link
Contributor

@kunjee17 The issue is that you don’t own neither the type nor the trait, so your not allowed to implement the trait for the type (see the error you’re getting).

You can wrap the u8 in a new-type if you want, something like:

struct U8GraphQL(u8);

implement Deref for U8GraphQL {
type Target = u8;

fn deref(&self) -> &u8 {
  &self.0
}
}

#[juniper::graphql_scalar(description = "u8")]
impl<S> GraphQLScalar for U8GraphQL where S:ScalarValue {
// ...

@kunjee17
Copy link
Contributor Author

@dyedgreen I thought it is allowed because of macros. I did it as given in the documentation.

@dyedgreen
Copy link
Contributor

It's not allowed at the language level, so doing code-gen via macros won't change that unfortunately 😅

@rimutaka
Copy link

@tyranron, @LegNeato , sorry to bother you with this, but would you be open to a PR that implements common Rust types as custom GQL types natively?

The approach suggested by @dyedgreen in the comment above is the best one we have out of the box, but it has a number of problems:

  • we may need to implement additional traits for that wrapper type, e.g. Ser/Deser
  • it may not be possible to change the type from a Rust scalar to wrapper because it is expected to be exactly that in other parts of the code
  • too much boilerplate code

What I would like to do is to implement custom GQL types for native Rust types not supported by GQL inside Juniper. E.g. GqlUsize, GqlU8, GqlI64. Those types will be available out of the box from Juniper for anyone who starts a new GQL project or is flexible with type naming. If someone needs to have GQL type for u8 named / defined exactly the way they want, they would have to implement it themselves as suggested by @dyedgreen here.

Overall, I think it will be a net benefit for Juniper.
I am happy to start working on it right now for my current project and then submit a PR when ready. This is a bit of a showstopper for us adopting Juniper (sounds like an ultimatum, sorry 😀 ).

@tyranron
Copy link
Member

tyranron commented Feb 11, 2022

@rimutaka putting it straight: I'm very skeptical to what you've described.

It's not wise to put every single scalar right into Juniper. Currently (and for the next major release too), we tend to provide only the ones declared by the GraphQL spec. And commonly-used wide-known having-at-least-minimal-spec extensions like https://www.graphql-scalars.dev/docs/scalars, but behind a feature flag only.

u8 and other parties are not commonly used by GraphQL, neither they have any minimal GraphQL spec. So they definitely won't go out-of-the-box. Maybe (I need think more about the exact design) they can be implemented behind a feature flag like scalars-rust or similar.
Even this has enough trickies, like:

  • u64 and u128 have problems parsing from JSON in JavaScript (and similar). This should be either documented clearly (which isn't ideal, doesn't prevent from accidential use), or use Strings under-the-hood (far from ideal too). So both tradeoffs are quite bad to propose them as a general solution.
  • naming should be aligned well with i32 reserved for just Int.

I still prefer to keep it "user makes a newtype with the desired semantics, if he needs it". The downsides you've described doesn't look that bad to me (worthing to put things into Juniper).

  • we may need to implement additional traits for that wrapper type, e.g. Ser/Deser

That's a common newtype pattern in Rust. We do use it a lot, for example, as we do have a lot of custom scalars. Usually, this looks like:

#[derive(AsRef, Clone, Debug, Display, Eq, Into, PartialEq)]
#[as_ref(forward)]
pub struct UserName(String);

Regarding the Serialize/Deserialize exactly, you don't need the ones to use the type as GraphQL scalar in juniper.

  • it may not be possible to change the type from a Rust scalar to wrapper because it is expected to be exactly that in other parts of the code

When you build the schema, you control the in/out parts of the program. So for what you've described it's enough to have From/Into implementations to convert the type before passing it into the part where you don't control it. Thanksfully to derive_more this may be done with as little boilerplate as possible.

  • too much boilerplate code

You only need to define you custom GraphQL scalars once and use .from()/.into() at the in/out side. Doesn't sound like too much. Quite an usual way to deal with orphan rules in Rust.

@LegNeato @ilslv would like to hear your thoughts on it as well.

@ilslv
Copy link
Member

ilslv commented Feb 11, 2022

@rimutaka

I'm very skeptical to what you've described.

I pretty much agree. Working with numbers in Rust maybe painful sometimes, but it provides more safety guarantees and makes you handle edge-cases explicitly. I don't think that erroring or panicing on those edge cases is the way to do it in Rust. Definitely not out of the box.

feature flag like scalars-rust or similar

This approach can be appealing in case there was a common community-agreed spec for custom scalars with naming and all that. But from what I can tell specs like graphql-scalars don't have it. I think the reason behind this is that other languages don't treat numbers like Rust does, so enforcing non-native way of doing numbers may become painful for front-end interacting with this crate.


Also, the thing is preventing you from implementing GraphQLScalar on u8 are orphan rules. But there is a way to avoid this without newtyping, by providing local ScalarValue implementation (I'm currently working on making procedural macro for it more pleasant to use).

@rimutaka
Copy link

@ilslv , did you say that implementing this trait may get me out of newtyping?

    fmt::Debug
    + fmt::Display
    + PartialEq
    + Clone
    + DeserializeOwned
    + Serialize
    + From<String>
    + From<bool>
    + From<i32>
    + From<f64>
    + 'static
{ ... }

It looks doable. I'm just not sure about 'static. Will give it a try after some sleep. If you have an implementation example handy it would help. No pressure. Thanks for the idea! :)

@ilslv
Copy link
Member

ilslv commented Feb 11, 2022

@rimutaka yep, there is an example inside crates integration tests:

#[graphql_scalar(name = "Long")]
impl GraphQLScalar for i64 {
fn resolve(&self) -> Value {
Value::scalar(*self)
}
fn from_input_value(v: &InputValue) -> Result<i64, String> {
v.as_scalar_value::<i64>()
.copied()
.ok_or_else(|| format!("Expected `MyScalarValue::Long`, found: {}", v))
}
fn from_str<'a>(value: ScalarToken<'a>) -> ParseScalarResult<'a, MyScalarValue> {
if let ScalarToken::Int(v) = value {
v.parse()
.map_err(|_| ParseError::UnexpectedToken(Token::Scalar(value)))
.map(|s: i64| s.into())
} else {
Err(ParseError::UnexpectedToken(Token::Scalar(value)))
}
}
}

@rimutaka
Copy link

@ilslv , thanks for the link, sir! I managed to make it compile with

#[graphql_object(scalar = MyScalarValue)]
impl TestType {
    fn long_field() -> i64 {
        i64::from(i32::MAX) + 1
    }
}

as in your example, but it falls over GraphQLObject in

#[derive(Debug, Deserialize, GraphQLObject)]
struct MyStruct {
    pub num: i64,
}

with a long list of missing implementations:

the trait bound `i64: GraphQLValue<__S>` is not satisfied
required because of the requirements on the impl of `IntoResolvable<'_, __S, i64, ()>` for `i64`rustc[E0277](https://doc.rust-lang.org/error-index.html#E0277)
[main.rs(24, 43): ]()consider extending the `where` bound, but there might be an alternative better way to express this requirement: `, i64: GraphQLValue<__S>`
the trait bound `i64: IsOutputType<__S>` is not satisfied
the trait `IsOutputType<__S>` is not implemented for `i64`rustc[E0277](https://doc.rust-lang.org/error-index.html#E0277)
[main.rs(24, 43): ]()consider extending the `where` bound, but there might be an alternative better way to express this requirement: `, i64: IsOutputType<__S>`
...

All scalar examples I could find had #[graphql_object(scalar = MyScalarValue)] for impl, not GraphQLObject for struct.

Is it possible to make i64 work with GraphQLObject? What am I missing?

@ilslv
Copy link
Member

ilslv commented Feb 12, 2022

@rimutaka yes, this is possible, but looks like not documented well enough unfortunately. All derive macros use #[graphql(...)] attributes that should pretty much mirror attribute macros. So the solution to your problem should be resolved by adding #[graphql(scalar = MyScalarValue)] like this:

#[derive(GraphQLObject)]
#[graphql(scalar = DefaultScalarValue)]
struct Human {
id: &'static str,
}

@rimutaka
Copy link

@ilslv, @tyranron, thanks a lot for the great product and your help. It was a steep learning curve (for me), but I finally got it working end-to-end. Would you like a PR with examples and doc updates for this topic?

@tyranron
Copy link
Member

@rimutaka thanks for the effort with docs, but, at the moment, we make quite enough breaking changes to the macro system and semantics, so there is no point to dig the docs now, as they will need total rewrite anyway, once the chages settle.

@tyranron tyranron added the k::documentation Related to project documentation label Feb 14, 2022
@tyranron
Copy link
Member

I leave this issue open for a while as a reminder for new docs to describe this case.

@rimutaka
Copy link

I'm trying to update my custom scalar implementation for u64 to work with the new #[graphql_scalar ...] macro as described in
https://graphql-rust.github.io/juniper/master/types/scalars.html#using-foreign-types-as-scalars.
The minimal example in the guide is a bit confusing. May be it was because I was trying to upgrade from the earlier version or it could be just me.

What got me unstuck was this example https://github.com/graphql-rust/juniper/blob/master/integration_tests/juniper_tests/src/codegen/scalar_value_derive.rs. Consider adding a link to that file from the guide. I'm happy to make a PR with it as a separate example.

@ilslv
Copy link
Member

ilslv commented Mar 14, 2022

@rimutaka can you please describe what exactly was confusing about the book? Is it wording Local 'ScalarValue' implementation.?

@rimutaka
Copy link

Yes, that's the one. I starting adapting the custom implementation I had, but wasn't sure what changed there. It looked like you completely removed GraphQLScalarValue. The change log was still referring to it as if it's in use. Small things like that.

Long story short, that integration example worked as-is and took me just a few minutes to merge with my custom code hence my suggestion to link to it. I wouldn't rush to make changes. It could be just me being a bit obtuse :)

@ilslv
Copy link
Member

ilslv commented Mar 14, 2022

@rimutaka

It looked like you completely removed GraphQLScalarValue

Yes, because old GraphQLScalarValue was corresponding to 2 different features: deriving ScalarValue on enums and implementing custom scalar on structs. Now it's 2 different derive macros ScalarValue and GraphQLScalar which are more feature-rich. Thanks for your feedback, I'll expand that part of the book to better cover the reasoning behind design decisions.

@tyranron tyranron self-assigned this Jun 1, 2022
@tyranron tyranron added this to the 0.16.0 milestone Jun 1, 2022
@tyranron tyranron modified the milestones: 0.16.0, 0.17.0 Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k::documentation Related to project documentation support
Projects
None yet
Development

No branches or pull requests

6 participants