graphql-rust / juniper

GraphQL server library for Rust
Other
5.71k stars 425 forks source link

Implement custom scalar for u8 type #862

Open kunjee17 opened 3 years ago

kunjee17 commented 3 years ago

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 commented 3 years ago

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

kunjee17 commented 3 years ago

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

dyedgreen commented 3 years ago

@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 commented 3 years ago

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

dyedgreen commented 3 years ago

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

rimutaka commented 2 years ago

@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:

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 commented 2 years ago

@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:

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

https://github.com/graphql-rust/juniper/blob/3a70403aba7a74652036b74f1db43fc0cfb6bb55/integration_tests/juniper_tests/src/custom_scalar.rs#L135-L156

rimutaka commented 2 years ago

@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 commented 2 years ago

@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:

https://github.com/graphql-rust/juniper/blob/3a70403aba7a74652036b74f1db43fc0cfb6bb55/integration_tests/juniper_tests/src/codegen/object_derive.rs#L823-L827

rimutaka commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

rimutaka commented 2 years ago

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 commented 2 years ago

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

rimutaka commented 2 years ago

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 commented 2 years ago

@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.