Skip to content

Redesign GraphQLScalar macros #1017

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

Merged
merged 92 commits into from
Feb 28, 2022
Merged

Redesign GraphQLScalar macros #1017

merged 92 commits into from
Feb 28, 2022

Conversation

ilslv
Copy link
Member

@ilslv ilslv commented Jan 14, 2022

Part of #1010

Synopsis

Main goal of this PR is to support generic scalars.

Solution

In addition to this PR tries to unify #[graphql_scalar] and #[derive(GraphQLScalar)] macros. This is done by introducing ability to use custom resolvers:

#[derive(GraphQLScalar)]
#[graphql(to_output_with = Self::to_output)]
struct Scalar(i32);

impl Scalar {
    fn to_output(&self) -> Value {
        Value::scalar(self.0 + 1)
    }
}

But we can't remove #[graphql_scalar] macro entirely because of uncovered use-case of derive macro: types from other crate

#[graphql_scalar(
    with = object_id,
    parse_token = String,
    scalar = LocalScalarValue,
)]
type ObjectId = bson::oid::ObjectId;

mod object_id {
    use super::*;

    pub(super) type Error = String;

    pub(super) fn to_output(v: &ObjectId) -> Value<LocalScalarValue> {
        Value::scalar(v.to_hex())
    }

    pub(super) fn from_input(v: &InputValue<LocalScalarValue>) -> Result<ObjectId, Error> {
        v.as_string_value()
            .ok_or_else(|| format!("Expected `String`, found: {}", v))
            .and_then(|s| {
                ObjectId::parse_str(s).map_err(|e| format!("Failed to parse `ObjectId`: {}", e))
            })
    }
}

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed

@ilslv ilslv changed the title Draft: Redesign GraphQLScalar macro Draft: Redesign GraphQLScalar and graphql_scalar macros Feb 4, 2022
@ilslv ilslv changed the title Draft: Redesign GraphQLScalar and graphql_scalar macros Redesign GraphQLScalar and graphql_scalar macros Feb 4, 2022
@ilslv
Copy link
Member Author

ilslv commented Feb 4, 2022

FCM

- Redesign `#[derive(GraphQLScalar)]` and `#[graphql_scalar]` macros (#1017)
  - `#[derive(GraphQLScalar)]`
    - support generic scalars
    - support structs with single named field
    - support for overriding resolvers
  - `#[graphql_scalar]`
    - use on type aliases in case `#[derive(GraphQLScalar)]` isn't applicable because of orphan rules

@ilslv ilslv marked this pull request as ready for review February 4, 2022 10:40
@ilslv
Copy link
Member Author

ilslv commented Feb 4, 2022

@tyranron at the end I've decided not to split PRs for derive and attribute macros, because built-in scalar implementation are looking better with both macros present:

#[derive(Clone, Debug, Eq, GraphQLScalar, PartialEq, Serialize, Deserialize)]
#[graphql(with = id, parse_token(String, i32))]
pub struct ID(String);

mod id {
    use super::*;

    pub(super) type Error = String;

    pub(super) fn to_output<S: ScalarValue>(v: &ID) -> Value<S> {
        Value::scalar(v.0.clone())
    }

    pub(super) fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<ID, Error> {
        v.as_string_value()
            .map(str::to_owned)
            .or_else(|| v.as_int_value().map(|i| i.to_string()))
            .map(ID)
            .ok_or_else(|| format!("Expected `String` or `Int`, found: {}", v))
    }
}

#[graphql_scalar(with = int)]
type Int = i32;

mod int {
    use super::*;

    pub(super) type Error = String;

    pub(super) fn to_output<S: ScalarValue>(v: &Int) -> Value<S> {
        Value::scalar(*v)
    }

    pub(super) fn from_input<S: ScalarValue>(v: &InputValue<S>) -> Result<Int, Error> {
        v.as_int_value()
            .ok_or_else(|| format!("Expected `Int`, found: {}", v))
    }

    pub(super) fn parse_token<S: ScalarValue>(value: ScalarToken<'_>) -> ParseScalarResult<'_, S> {
        if let ScalarToken::Int(v) = value {
            v.parse()
                .map_err(|_| ParseError::UnexpectedToken(Token::Scalar(value)))
                .map(|s: i32| s.into())
        } else {
            Err(ParseError::UnexpectedToken(Token::Scalar(value)))
        }
    }
}

@ilslv ilslv marked this pull request as draft February 4, 2022 13:32
@ilslv ilslv changed the title Redesign GraphQLScalar and graphql_scalar macros Redesign GraphQLScalar macros Feb 17, 2022
@ilslv ilslv mentioned this pull request Feb 17, 2022
15 tasks
# Conflicts:
#	docs/book/content/types/scalars.md
#	integration_tests/juniper_tests/src/codegen/mod.rs
#	integration_tests/juniper_tests/src/codegen/scalar_attr_derive_input.rs
#	juniper/src/executor_tests/introspection/mod.rs
#	juniper/src/executor_tests/variables.rs
#	juniper/src/lib.rs
#	juniper/src/types/scalars.rs
#	juniper_codegen/src/graphql_scalar/attr.rs
#	juniper_codegen/src/graphql_scalar/mod.rs
#	juniper_codegen/src/lib.rs
#	juniper_codegen/src/result.rs
@ilslv ilslv marked this pull request as ready for review February 25, 2022 07:27
@ilslv ilslv requested a review from tyranron February 25, 2022 08:02
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv great work, thanks! 🍻

let attr = Attr::from_attrs("graphql", &ast.attrs)?;

let field = match (
attr.to_output.as_deref().cloned(),
Copy link
Member

Choose a reason for hiding this comment

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

This whole match is quite a repeatable pattern. It'd better be deduced into a separate method.

@tyranron tyranron merged commit 0ebd19a into master Feb 28, 2022
@tyranron tyranron deleted the new-derive-scalar branch February 28, 2022 09:34
@rimutaka
Copy link

@ilslv , @tyranron , how final is this change? I saw some rust-docs updates in the diffs, but no book updates. What is the plan with that? I'm happy to help there.

@tyranron
Copy link
Member

@rimutaka the current Book isn't ideal, but should be up-to-date with these changes on the current master. Please, re-check it.

@tyranron tyranron added this to the 0.16.0 milestone Apr 13, 2022
@tyranron tyranron mentioned this pull request Jun 1, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface) semver::breaking Breaking change in terms of SemVer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants