Skip to content

Macro for InputValue creation #996

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 23 commits into from
Nov 26, 2021
Merged

Macro for InputValue creation #996

merged 23 commits into from
Nov 26, 2021

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Nov 23, 2021

Resolves #503

Synopsis

We have a graphql_value!() macro which allows Values creation in a convenient way.

We don't have similar for InputValues and Variables, however. Which makes test cases unergonomic, harder to write and read.

Solution

Introduce graphql_input_value!() and graphql_vars!() macros for that purpose.

Additionally

While graphql_value!() exists, it has some inconveniences at the moment:

  • It accepts None for null values. It would be better to follow serde_json::json!() macro here and accept nulls too instead of None.
  • It doesn't play well with accepting expressions. In some cases it just fails.

To cover those needs, it is worth to look at how serde_json::json!() is implemented and repeat the simplified version for our needs, both for graphql_input_value!() and graphql_value!() macros.

@tyranron tyranron added enhancement Improvement of existing features or bugfix semver::breaking Breaking change in terms of SemVer labels Nov 23, 2021
@tyranron
Copy link
Member Author

@ilslv it would be better also to move macros definitions into their own modules.

@ilslv
Copy link
Member

ilslv commented Nov 24, 2021

@tyranron graphql_input_value! should be able to encode InputValue::Variable and InputValue::Enum. GraphQL uses $ prefix to indicate variable: example. But rust declarative macros can't match $, so we could possibly use something like # or @, but it will be hard to remember and it doesn't solve InputValue::Enum. So using let and enum prefixes makes most sense to me:

graphql_input_value!({
    "key": 123,
    "var": let variable,
    "enum": enum ENUM,
    "arr": [123, let another, enum OTHER]
});

@tyranron
Copy link
Member Author

tyranron commented Nov 24, 2021

@ilslv it's seems that for enums there should be just an ident usage, if we want to comply with GraphQL syntax.

So, we have the following options here:

  1. Either use the variant you've proposed, which is unintuitive and compliant with GraphQL syntax, but it's macro_rules so good IDE assist.
  2. Or implement via proc macro and support syntax for variables as $var and enum values as UPPERCASED_IDENT, but then loose IDE assist.
  3. Or throw out support for variables and enums and don't bother at all.
  4. Or throw out support for variables only while stringify!(ident) should work for enum values, so we still may use macro_rules for that, and preserve IDE assist.

Considering that this feature is primarily for internal usage, what do you prefer?

@ilslv
Copy link
Member

ilslv commented Nov 24, 2021

@tyranron great idea with ident, completely missed that.
I think we should stick with declarative macros, because of IDE support and much simpler implementation. It may be less robust, but I don't think it has to be, considering usage.
I lean towards stringify!(ident) for enums and #var for variables. It gives us most amount of flexibility and if this macro will become more popular than we expect, we'll be able to rewrite it into proc macro with $var and least amount of breaking changes.

@tyranron
Copy link
Member Author

@ilslv ok, let's do this way then.

#var for variables

Maybe @var then? # is usually associated with comments or meta.

@ilslv
Copy link
Member

ilslv commented Nov 25, 2021

FCM

Implement `graphql_input_value!` and `graphql_vars!` macros (#996, #503)

- add `From` impls to `InputValue`, that mirror `Value` impls to provide better support for `Option` handling
- support expressions in `graphql_value!` macro
- use `null` in addition to `None` to create `Value::Null` in `graphql_value!` macro to mirror `serde_json::json!`

@tyranron this PR is ready for review. I've left support for creating Value::Null with None because this feature is part of support for better handling of Options:

#[test]
fn option_support() {
    let val = Some(42);
    assert_eq!(graphql_value!(None), V::Null);
    assert_eq!(graphql_value!(null), V::Null);
    assert_eq!(graphql_value!(Some(42)), V::scalar(42));
    assert_eq!(graphql_value!(val), V::scalar(42));
}

@tyranron tyranron removed the semver::breaking Breaking change in terms of SemVer label Nov 25, 2021
Copy link
Member Author

@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 despite the described nitpickts, you've done a brilliant work with macros implementation. Thanks!

({}) => {{ ::std::collections::HashMap::<::std::string::String, _>::new() }};

({ $($map:tt)* }) => {{
let mut object = ::std::collections::HashMap::<::std::string::String, _>::new();
Copy link
Member Author

Choose a reason for hiding this comment

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

It's better to reuse the type alias here, referring to it as to an opaque type. This will allow us to unlikely mess with the macro once the implemantion of Variables changes.

@@ -743,7 +743,7 @@ query Hero($episode: Episode) {

if let crate::ast::Definition::Operation(ref op) = docs[0] {
let mut vars = Variables::default();
vars.insert("episode".into(), InputValue::Enum("JEDI".into()));
vars.insert("episode".into(), graphql_input_value!(JEDI));
Copy link
Member Author

Choose a reason for hiding this comment

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

We better use graphql_vars!() here.

fn scalar() {
let val = 42;
assert_eq!(
graphql_vars!({ "key": 123 }),
Copy link
Member Author

Choose a reason for hiding this comment

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

As far as we always have a mapping here, we may strip redundant braces and have only graphql_vars! { "key": 123 } like indexmap! does.

@@ -235,7 +235,7 @@ async fn querying_long_variable() {
"query q($test: Long!){ longWithArg(longArg: $test) }",
vec![(
"test".to_owned(),
InputValue::Scalar(MyScalarValue::Long(i64::from(i32::MAX) + 42)),
graphql_input_value!(i64::from(i32::MAX) + 42),
Copy link
Member Author

Choose a reason for hiding this comment

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

That's very strange why CI has passed, but running cargo test -p juniper_tests --all-features locally fails to compile with the following error:

error[E0277]: the trait bound `InputValue<_>: From<i64>` is not satisfied
   --> integration_tests/juniper_tests/src/custom_scalar.rs:238:13
    |
238 |             graphql_input_value!(i64::from(i32::MAX) + 42),
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<i64>` is not implemented for `InputValue<_>`
    |
    = help: the following implementations were found:
              <InputValue<S> as From<&'a str>>
              <InputValue<S> as From<bool>>
              <InputValue<S> as From<f64>>
              <InputValue<S> as From<i32>>
            and 2 others
note: required by `std::convert::From::from`
   --> ~/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/convert/mod.rs:371:5
    |
371 |     fn from(_: T) -> Self;
    |     ^^^^^^^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `graphql_input_value` (in Nightly builds, run with -Z macro-backtrace for more info)

It would be nicer to have transparent conversion or some kind forwarding wrapper here, because custom scalars may have additional From impls to the ones you've provided for InputValue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be quite a rabbit hole with a wrapper, cause due to lack of specialization we cannot structurally propagate in Option<_>, and so it won't work with expressions inside macro.

The easiest way will be just use InputValue::scalar(i64) when calling the macro, without any additions.

@tyranron tyranron merged commit acde85a into master Nov 26, 2021
@tyranron tyranron deleted the input-value-macro branch November 26, 2021 16:54
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support experssions when using juniper::graphql_value
2 participants