Open Victor-Savu opened 4 years ago
Could have sworn we had an enhancement request for this alrteady but can't find it. I agree this would be very nice to have.
As a workaround you can make your own macro or derive that spits out a #[juniper::graphql_object]
for you with the boilerplate included.
@LegNeato Thanks for the quick reply! I quickly went through all of the open items before posting just because I was really expecting to see this issue :) I hope I didn't miss anything and if I did I'm sorry.
I will have a look at how to achieve that with my own macro for now. Would it be useful to leave this issue open and see if anyone else may want this feature?
Yep!
I tried looking into rolling my own macro for generating the necessary fields and found a few things that I wanted to share in case you or someone else has an idea for how to best approach this.
The problem I ran into was that I could not solve this problem with a derive macro. In order to have since derive macros only apply to either the struct
item or to the impl
item so it would be impossible to get information about both without using a function-like procedural macro. I opened a draft PR #592 with an implementation that demonstrates the results.
@LegNeato The PR is just to get an idea of whether you and the other juniper maintainers would be open to this direction.
I had an idea how to just make #[derive(GraphQLObject)]
and #[graphql_object])
work together, but the implementation depends on rust-lang/rfcs#1210 (or rather on #![feature(specialization)]
in nightly).
Here is the general design: We can have two intermediate traits:
GraphQLData
derrived from the struct
/enum
by #[derive(GraphQLObject)]
; andGraphQLImpl
the implementation of which is generated from the impl
block by #[graphql_object]
.The two traits would each specify similar methods as the GraphQLType
. We can then have a default implementations of GraphQLType based on objects that implement GraphQLData
and a specialization for those that implement GraphQLImpl
. For coherence reasons, we need GraphQLImpl to require GraphQLData:
trait GraphQLImpl : GraphQLData {
...
}
Here is a mock implementation in the rust playground%0A%20%20%20%20%7D%0A%7D%0A%0A%2F%2F%20---------%20%3C%2Fjuniper%3E%20---------%0A%0A%2F%2F%20---------%20%3Cuser-code%3E%20---------%0A%0A%2F%2F%20%23%5Bderive(GraphQLObject)%5D%0Astruct%20X%20%7B%0A%20%20%20%20x%3A%20usize%2C%0A%7D%0A%0Astruct%20Y%20%7B%0A%20%20%20%20y%3A%20usize%2C%0A%7D%0A%0Aimpl%20GraphQLData%20for%20Y%20%7B%7D%0A%0A%2F%2F%20%23%5Bgraphql_object%5D%0Aimpl%20Y%20%7B%0A%20%20%20%20fn%20yarrow(%26self)%20-%3E%20String%20%7B%0A%20%20%20%20%20%20%20%20format!(%22%7B%7D%20flower%7B%7D%22%2C%20self.y%2C%20if%20self.y%20%3D%3D%201%20%7B%20%22%22%20%7D%20else%20%7B%20%22s%22%20%7D)%0A%20%20%20%20%7D%0A%7D%0A%0A%2F%2F%20%23%5Bderive(GraphQLObject)%5D%0Astruct%20Z%20%7B%0A%20%20%20%20z%3A%20usize%2C%0A%7D%0A%0A%2F%2F%20%23%5Bgraphql_object%5D%0Aimpl%20Z%20%7B%0A%20%20%20%20fn%20zoo(%26self)%20-%3E%20String%20%7B%0A%20%20%20%20%20%20%20%20%22zebras%22.to_owned()%0A%20%20%20%20%7D%0A%7D%0A%0Afn%20main()%20%7B%0A%20%20%20%20let%20x%20%3D%20X%20%7B%20x%3A%205%20%7D%3B%0A%20%20%20%20let%20y%20%3D%20Y%20%7B%20y%3A%20153%20%7D%3B%0A%20%20%20%20let%20z%20%3D%20Z%20%7B%20z%3A%202%20%7D%3B%0A%0A%20%20%20%20println!(%22%23%20x%20%7B%7B%20x%3A%20'5'%20%7D%7D%5Cn%22)%3B%0A%20%20%20%20println!(%22x%5B'x'%5D%3A%20%7B%3A%3F%7D%22%2C%20x.resolve(%22x%22))%3B%0A%20%20%20%20println!(%22x%5B'y'%5D%3A%20%7B%3A%3F%7D%22%2C%20x.resolve(%22y%22))%3B%0A%0A%20%20%20%20println!(%22%5Cn%20---%20~%20---%5Cn%22)%3B%0A%0A%20%20%20%20println!(%22%23%20y%20%7B%7B%20yarrow%3A%20'153%20flowers'%20%7D%7D%5Cn%22)%3B%0A%20%20%20%20println!(%22y%5B'yarrow'%5D%3A%20%7B%3A%3F%7D%22%2C%20y.resolve(%22yarrow%22))%3B%0A%20%20%20%20println!(%22y%5B'y'%5D%3A%20%7B%3A%3F%7D%22%2C%20y.resolve(%22y%22))%3B%0A%0A%20%20%20%20println!(%22%5Cn%20---%20~%20---%5Cn%22)%3B%0A%0A%20%20%20%20println!(%22%23%20z%20%7B%7B%20z%3A%20'2'%2C%20zoo%3A%20'zebras'%20%7D%7D%5Cn%22)%3B%0A%20%20%20%20println!(%22z%5B'z'%5D%3A%20%7B%3A%3F%7D%22%2C%20z.resolve(%22z%22))%3B%0A%20%20%20%20println!(%22z%5B'zoo'%5D%3A%20%7B%3A%3F%7D%22%2C%20z.resolve(%22zoo%22))%3B%0A%20%20%20%20println!(%22z%5B'y'%5D%3A%20%7B%3A%3F%7D%22%2C%20z.resolve(%22y%22))%3B%0A%7D%0A%0A%2F%2F%20---------%20%3C%2Fuser-code%3E%20---------%0A%0A%2F%2F%20---------%20%3Cgenerated%3E%20---------%0A%0A%2F%2F%20by%20%23%5Bderive(GraphQLObject)%5D%0Aimpl%20GraphQLData%20for%20X%20%7B%0A%20%20%20%20fn%20data_resolve(%26self%2C%20field%3A%20%26str)%20-%3E%20Option%3CString%3E%20%7B%0A%20%20%20%20%20%20%20%20if%20field%20%3D%3D%20%22x%22%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20Some(format!(%22%7B%7D%22%2C%20self.x))%0A%20%20%20%20%20%20%20%20%7D%20else%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20None%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%7D%0A%0A%2F%2F%20by%20%23%5Bgraphql_object%5D%0Aimpl%20GraphQLImpl%20for%20Y%20%7B%0A%20%20%20%20fn%20impl_resolve(%26self%2C%20field%3A%20%26str)%20-%3E%20Option%3CString%3E%20%7B%0A%20%20%20%20%20%20%20%20if%20field%20%3D%3D%20%22yarrow%22%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20Some(format!(%22%7B%7D%22%2C%20self.yarrow()))%0A%20%20%20%20%20%20%20%20%7D%20else%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20None%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%7D%0A%0A%2F%2F%20by%20%23%5Bderive(GraphQLObject)%5D%0Aimpl%20GraphQLData%20for%20Z%20%7B%0A%20%20%20%20fn%20data_resolve(%26self%2C%20field%3A%20%26str)%20-%3E%20Option%3CString%3E%20%7B%0A%20%20%20%20%20%20%20%20if%20field%20%3D%3D%20%22z%22%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20Some(format!(%22%7B%7D%22%2C%20self.z))%0A%20%20%20%20%20%20%20%20%7D%20else%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20None%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%7D%0A%0A%2F%2F%20by%20%23%5Bgraphql_object%5D%0Aimpl%20GraphQLImpl%20for%20Z%20%7B%0A%20%20%20%20fn%20impl_resolve(%26self%2C%20field%3A%20%26str)%20-%3E%20Option%3CString%3E%20%7B%0A%20%20%20%20%20%20%20%20if%20field%20%3D%3D%20%22zoo%22%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20Some(format!(%22%7B%7D%22%2C%20self.zoo()))%0A%20%20%20%20%20%20%20%20%7D%20else%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20None%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%7D%0A%0A%2F%2F%20---------%20%3C%2Fgenerated%3E%20---------%0A).
The downside of the design in my comment above is that impl GraphQLData
becomes necessary even for cases where users don't want to expose some (or any) of the fields in a struct
. Also, it is needed for rust enums as well. A hopefully convenient way to solve this is to provide default implementations for the methods in GraphQLData
such that the user would only need to write:
struct Y { y: usize } // don't want to expose Y::y
impl GraphQLData for MyType {}
to satisfy the condition and to hide all the members in MyType
. This option is shown in the playground example as well.
Another possibility is to use handler functions. The user defines structures which contain data and handler fields. For each data fields code is generated. A Handler field references a function, similar to Serde. However, this could require to remove non-async code first, because we can not distinguish between async and non-async functions.
#[derive(juniper::GraphQLObject)]
struct Foo {
name: String
#[graphql(Handler = foo::computed)]
computed: String,
}
mod foo {
fn computed(&self) -> String {
...
}
}
Maybe this approach fits better, but I am not sure.
@jmpunkt This sounds great as well! The design space for this problem seems bountiful.
However, this could require to remove non-async code first, because we can not distinguish between async and non-async functions.
I think if we can tolerate a bit more typing, we can specify when a handler is synchronous. The default could be async:
#[derive(juniper::GraphQLObject)]
struct Foo {
name: String
#[graphql(Handler = foo::computed, synchronous = true)]
computed: String,
#[graphql(Handler = foo::requested)]
requested: String,
}
mod foo {
fn computed(&self) -> String {
...
}
async fn requested(&self) -> String {
...
}
}
A current implementation which compiles (nothing tested) looks like the following:
#[derive(GraphQLObject, Debug, PartialEq)]
pub struct HandlerFieldObj {
regular_field: bool,
#[graphql(Handler = handler_field_obj::skipped, noasync)]
skipped: i32,
#[graphql(Handler = handler_field_obj::skipped_result, noasync)]
skipped_result: i32,
#[graphql(Handler = handler_field_obj::skipped_async)]
skipped_async: i32,
#[graphql(Handler = handler_field_obj::skipped_async_result)]
skipped_async_result: i32,
}
mod handler_field_obj {
pub fn skipped(obj: &super::HandlerFieldObj, ctx: &()) -> i32 {
0
}
pub fn skipped_result(obj: &super::HandlerFieldObj, ctx: &()) -> juniper::FieldResult<i32> {
Ok(0)
}
pub async fn skipped_async(obj: &super::HandlerFieldObj, ctx: &()) -> i32 {
0
}
pub async fn skipped_async_result(
obj: &super::HandlerFieldObj,
ctx: &(),
) -> juniper::FieldResult<i32> {
Ok(0)
}
}
A main different to the examples before, is that it is required to have a context and to use the type of the object rather than self
.
To fix this issue, an impl macro for this special case is required. This impl macro provides self
binding and context handling. Therefore, the mod block would look very similar to an impl block. The macro would transform the function into same format as the example, but would use a similar input as the impl macro.
@jmpunkt I'm impressed and surprised. Was this possible all along? I thought I went over the book with a comb and couldn't find the graphql attribute macro on struct fields taking a Handler. Is this documented somewhere and I totally missed it?
No it is not possible with the code on the master branch. Due to the structure of Juniper, it does not require that much changes to support this behavior.
The sentence
A current implementation which compiles (nothing tested) looks like the following
refers to my testing branch.
I am slightly disagree the proposed approach above. For instance, I am using diesel
to query the database:
#[derive(Debug, Clone, PartialEq, Queryable, Identifiable)]
pub struct User {
pub id: i32,
pub first_name: String,
pub last_name: String,
// pub full_name: String,
}
#[juniper::graphql_object]
impl User {
pub fn full_name(&self) -> String {
format!("{} {}", self.first_name, self.last_name)
}
}
In the above example, I won't want to put the full_name
field in User struct as there is no such column in the users
table. As a workaround, I have to define another struct that doesn't contain the full_name
field for diesel
, which really just delegating the boilerplates...
I did not think of that. Since the original idea was to reduce the boilerplate, what about this?
#[derive(Debug, Clone, PartialEq, Queryable, Identifiable)]
pub struct User {
pub id: i32,
pub first_name: String,
pub last_name: String,
}
#[juniper::graphql_object(id: i32, first_name: String, last_name: String)]
impl User {
pub fn full_name(&self) -> String {
format!("{} {}", self.first_name, self.last_name)
}
}
We define trivial fields inside graphql_object. The macro provides trivial implementations for the specified fields.
Explicitly defining trivial fields also works.
Or I am not sure if it's possible that the macro provides trivial implementations for all fields in the User struct by default (like #[derive( GraphQLObject)]
), unless a field has #[graphql(skip)]
. And methods in impl User
are treated as field resolvers, which can be additional to (or overriding) the provided trivial implementations.
I had been using type-graphql with TypeScript, which has very similar experience like this.
It would be awesome if we can combine with this feature request (#647).
The overall problem with this feature is that a macro can only see the underlying AST, e.g., impl macros can only see the impl block. There is now way for the #[juniper::graphql_object]
to know what fields, e.g., User
has.
My last statement is maybe not 100% true. We could get information from structs if we generate code which provide these information. We define a trait JuniperObjectInfo
which can hold field information of a struct. The trait is implemented by the proc macro GraphQLObjectInfo
. Instead of implementing the usual Juniper logic, only the trait with the necessary field information is generated. Finally we specify that #[graphql_object]
must use the previous defined information in JuniperObjectInfo
. During the code generation the following line is generated <#obj as JuniperObjectInfo>::fields()
which returns the previously defined fields. The proc marco now has all necessary information to generate code which includes fields not defined inside the impl block. If #[graphql(partial)]
is not specified, then only fields listed inside the impl block are used.
Notice that #[derive(GraphQLObjectInfo)]
has the same attributes as #[derive(GraphQLObject)]
. The only difference between these macros is the generated code.
#[derive(juniper::GraphQLObjectInfo)]
struct Obj {
field: String,
}
impl juniper::JuniperObjectInfo for Obj {
fn fields() -> &'static [Field] {
&[ /* generated by macro */ ]
}
}
#[juniper::graphql_object]
#[grapqhl(partial)]
impl User {
pub fn full_name(&self) -> String {
format!("{} {}", self.first_name, self.last_name)
}
}
Not sure if this works.
@zhenwenc Thank you for your interest in this issue! I was wondering if you had a look at my PR #592 for an alternative syntax (admittedly, a bit of a stretch as far as syntactical changes go). Thanks!
@Victor-Savu Thanks! I did had a look the PR, its indeed a very nice idea! But I think it requires a bit too much changes on the current API. I do like the current way of defining field resolvers with #[juniper::graphql_object]
macro, as its pretty close to my previous experiences (such as type-graphql), therefore I would prefer as minimal changes to the current API as possible.
I think the idea of having a JuniperObjectInfo
trait to hold the field info for a struct is great. @jmpunkt Thank you for the great idea!
We can even make the changes compatible with the current behaviour if #[juniper::graphql_object]
only generates trivial implementations for all fields when explicitly required, such as enable it with #[grapqhl(auto)]
attribute.
#[juniper::graphql_object]
#[grapqhl(auto)]
impl User {
pub fn full_name(&self) -> String {
format!("{} {}", self.first_name, self.last_name)
}
}
Gonna work on a P.O.C with my very limited rust knowledge to see if its possible.
Cheers!
Looks like the approach indeed works!! Here is my POC.
One can opt-in this feature by using #[juniper::graphql_object(derive_fields)]
attribute on the impl block, and derive([GraphQLObjectInfo])
on the target struct is required.
#[derive(GraphQLObjectInfo, Debug, PartialEq)] // <-- here
#[graphql(scalar = DefaultScalarValue)]
struct Obj {
regular_field: bool,
#[graphql(
name = "renamedField",
description = "descr",
deprecated = "field deprecation"
)]
c: i32,
}
#[juniper::graphql_object(
name = "MyObj",
description = "obj descr",
scalar = DefaultScalarValue,
derive_fields // <-- here
)]
impl Obj {
fn custom_field(&self) -> &str {
"custom field"
}
}
The derived GraphQLTypeInfo
looks like this:
impl juniper::GraphQLTypeInfo<DefaultScalarValue> for Obj {
type Context = ();
type TypeInfo = ();
fn fields<'r>(
info: &Self::TypeInfo,
registry: &mut juniper::Registry<'r, DefaultScalarValue>,
) -> Vec<juniper::meta::Field<'r, DefaultScalarValue>>
where
DefaultScalarValue: 'r,
{
<[_]>::into_vec(box [
registry.field_convert::<bool, _, Self::Context>("regularField", info),
registry
.field_convert::<i32, _, Self::Context>("renamedField", info)
.description("descr")
.deprecated(Some("field deprecation")),
])
}
#[allow(unused_variables)]
#[allow(unused_mut)]
fn resolve_field(
self: &Self,
_info: &(),
field: &str,
args: &juniper::Arguments<DefaultScalarValue>,
executor: &juniper::Executor<Self::Context, DefaultScalarValue>,
) -> juniper::ExecutionResult<DefaultScalarValue> {
match field {
"regularField" => {
let res = (|| &self.regular_field)();
juniper::IntoResolvable::into(res, executor.context()).and_then(|res| match res
{
Some((ctx, r)) => executor.replaced_context(ctx).resolve_with_ctx(&(), &r),
None => Ok(juniper::Value::null()),
})
}
"renamedField" => {
let res = (|| &self.c)();
juniper::IntoResolvable::into(res, executor.context()).and_then(|res| match res
{
Some((ctx, r)) => executor.replaced_context(ctx).resolve_with_ctx(&(), &r),
None => Ok(juniper::Value::null()),
})
}
_ => {
{
::std::rt::begin_panic_fmt(&::core::fmt::Arguments::new_v1(
&["Field ", " not found on type "],
&match (
&field,
&<Self as juniper::GraphQLType<DefaultScalarValue>>::name(_info),
) {
(arg0, arg1) => [
::core::fmt::ArgumentV1::new(arg0, ::core::fmt::Display::fmt),
::core::fmt::ArgumentV1::new(arg1, ::core::fmt::Debug::fmt),
],
},
))
};
}
}
}
}
And the generated GraphQLTypeAsync
looks like this:
impl juniper::GraphQLTypeAsync<DefaultScalarValue> for Obj
where
DefaultScalarValue: Send + Sync,
Self: Send + Sync,
{
fn resolve_field_async<'b>(
&'b self,
info: &'b Self::TypeInfo,
field: &'b str,
args: &'b juniper::Arguments<DefaultScalarValue>,
executor: &'b juniper::Executor<Self::Context, DefaultScalarValue>,
) -> juniper::BoxFuture<'b, juniper::ExecutionResult<DefaultScalarValue>>
where
DefaultScalarValue: Send + Sync,
{
use futures::future;
use juniper::GraphQLType;
match field {
"customField" => {
let res: &str = (|| "custom field")();
let res2 = juniper::IntoResolvable::into(res, executor.context());
let f = async move {
match res2 {
Ok(Some((ctx, r))) => {
let sub = executor.replaced_context(ctx);
sub.resolve_with_ctx_async(&(), &r).await
}
Ok(None) => Ok(juniper::Value::null()),
Err(e) => Err(e),
}
};
use futures::future;
future::FutureExt::boxed(f)
}
_ => {
let v = <Self as juniper::GraphQLTypeInfo<DefaultScalarValue>>::resolve_field(
self, info, field, args, executor,
);
future::FutureExt::boxed(future::ready(v))
}
}
}
}
Feels like it requires a lot of changes, but most of them are really just minor refactoring to be able to reuse the existing functions.
Please let me know hows my implementation and if there is any better approach.
Thanks!
Looks good.
Currently it is not clear what happens on duplicate fields. I would assume that the registry (https://github.com/zhenwenc/juniper/blob/4cdf13396c8c68841d08bb8319cca4055c217980/juniper_codegen/src/util/mod.rs#L888) overrides if the field is already defined. In my opinion silently overriding is not the best approach. We should test upfront if there are possible duplicates and abort (hard error). The user would skip or remove the field in the #[derive(GraphQLObjectInfo)]
.
Thanks @jmpunkt !
Yes, the derived field resolvers act as fallbacks, where user defined resolvers (in impl block
) should take priority. Agree that we should stop the user if there are possible duplicates.
I will wrap up a proper PR with some tests. Cheers!
I thought about the duplicate issue and I think there is no perfect solution. However, I had two ideas in my mind.
A
holds a list of all added fields. The add
functions of the registry would be constrained such that a function are only callable if there is no typeB
in A
. B
represents a single field, thus each fields needs a unique type. So this would probably blow up the compile time and adds complexity to the registry.Maybe someone gets inspired and finds a better solution.
@jmpunkt Thanks for the advice! Could you please review this PR https://github.com/graphql-rust/juniper/pull/652 when available.
At the moment I perform the duplicate fields check on the GraphQLType#meta
method:
https://github.com/graphql-rust/juniper/blob/2e5805fd3892bc2457c4bd34b3adc5cf198f1f3c/juniper_codegen/src/util/mod.rs#L911-L918
which is a runtime check since I couldn't get the struct field information baked in the GraphQLObjectInfo
at compile time when expanding the graphql_object
macro, but I can be wrong. However, I guess the meta
function should only be executed once when creating the graphql schema (RootNode), so the added overhead should be quite minimal 🤞 .
I agree the solution is not perfect though, keen to know if there is any better solution. Cheers!
@jmpunkt @zhenwenc @Victor-Savu
I was thinking about this issue too for quite a while, and here is something to share...
I've thought about the following design:
#[derive(GraphQLObject)] // <-- mandatory
#[graphql(
name = "MyObj",
description = "obj descr",
scalar = DefaultScalarValue,
)]
struct Obj {
regular_field: bool,
#[graphql(
name = "renamedField",
description = "descr",
deprecated = "field deprecation"
)]
c: i32,
#[graphql(ignore)] // or #[graphql(skip)] ?
hidden: bool,
}
#[graphql_object(scalar = DefaultScalarValue)] // <- optional
impl Obj {
fn custom_field(&self) -> &str {
"custom field"
}
fn hidden(&self) -> bool {
self.hidden
}
}
So, the impl Obj {}
block is always optional, while the type itself should have #[derive(GraphQLObject)]
on it.
As we cannot access from #[derive(GraphQLObject)]
proc macro the tokens fed into #[graphql_object]
macro, we need some mechanism to register things separately and then reuse them in one place. It seems that inventory
crate is widely used exactly for that purpose, so we can: generate separate glue code for resolvers as usual functions outside impl GraphQLType
block, and then, in the generated #[derive(GraphQLObject)]
code, just inventory::iter()
over the all resolvers.
This way, we can have even multiple #[graphql_object] impl Obj { }
blocks.
As already has been described above in the discussion, the 2 questions arise here:
inventory::iter()
doesn't provide any order guarantees at all, so we will end up having them randomly swapped every time. The only viable solution I've figured out here, is to use BTreeMap
in the code generated for fields()
method, so have all the fields alphabetically sorted always. I haven't figured out the way to preserve natural declaration order.rustc
will complain about duplication. We can even use those modules naturally, by putting the generated resolver code inside and not polute the current module (something like gql_obj_Obj_hidden::resolve()
).In addition, I think it would be nice to preserve impl Obj {}
blocks clean after macro expansion, and reused "as is" in the generated code (at the moment they are just a fiction, not real methods). This will allow for library users to avoid confusions like "where are my rustdocs?".
Thanks for the input. I did not know that the inventory
crate exists. To get this clear, we would implement similar functionality (as in inventory
) for our registry? Such that we can use the declarative macros for our registry? As a result, the resolver code generate by the #[derive(GraphQLObject)]
would only call the collect
macro (similar to inventory
). Possible problems with the design is compatibility with earlier version since it requires the derive macro for each object. Another problem could be the use of declarative macros in the code generation process (not sure if is possible to generate code which contains declarative macros).
Some notes on your other ideas
@jmpunkt
To get this clear, we would implement similar functionality (as in inventory) for our registry? Such that we can use the declarative macros for our registry?
No. inventory
is a tricky thing: it allows to register things "statically" across crates (AFAIK, registration happens before main()
is executed), while collect them in runtime anytime we want (after main()
started).
I doubt juniper
's registry works this way, nor I see any advantages to piggyback one on another in any way.
What I've meant is that macro expansion contains something like inventory::submit! { ObjResolvers{ /* info about field and resolver fn */ } }
for each field and inventory::collect!(ObjResolvers);
on #[derive(GraphQLObject)]
expansion. And then, when generating code for filling up registry:
fn fields<'r>(
info: &Self::TypeInfo,
registry: &mut juniper::Registry<'r, DefaultScalarValue>,
) -> Vec<juniper::meta::Field<'r, DefaultScalarValue>>
where
DefaultScalarValue: 'r,
{
for field in inventory::iter::<ObjResolvers> {
// fill up registry with info
}
}
Possible problems with the design is compatibility with earlier version since it requires the derive macro for each object.
This way there won't be any compatibility problems, as inventory
will be used for each type separately, and if you're not using derive
you may not use inventory
for your implementation at all.
The only incompatibility is that to codegen implementation you should always have #[derive(GraphQLObject)]
on your type. But this is intended by this design to uniform the stuff.
Maybe you can open an issue for the "preserve impl Obj {}" feature. Sounds useful but for now I can not imagine how this should look like.
Hmmm.... I've thought about something like this:
// Preserved code that has been fed into `#[graphql_object]` macro.
impl Obj {
pub fn full_name(&self) -> String {
format!("{} {}", self.first_name, self.last_name)
}
}
// Generated code.
mod gql_Obj_fullName {
use super::Obj;
fn resolve(obj: &Obj, /* unified parameters to call resolver */) -> Result</* unified possible result */> {
// extract required for call here
let res = Obj::full_name(obj);
// wrap into required result
}
}
// Generated code.
inventory::submit! {
__ObjResolver {
name: "fullName",
// and other metadata here...
resolve_fn: gql_Obj_fullName::resolve,
}
}
This way the original code is untouched, while we generate all the required machinery around.
Thanks for the clarifications and examples. Overall I really like the design since it is more "like" Rust. Not sure how well this works at the end.
The only incompatibility is that to codegen implementation you should always have #[derive(GraphQLObject)] on your type. But this is intended by this design to uniform the stuff.
I would like to know what @LegNeato thinks about the whole situation.
A small side note. In terms of unification, there is a similar issue (https://github.com/graphql-rust/juniper/pull/631#issuecomment-620824702). The PR did not unify the interface since it would require a lot of changes in the tests. However, if we are planing to uniform Juniper, then we should enforce this.
@jmpunkt I've read the discussion and I'm a bit more convinced to allow resolvers without &self
.
Things like
Therefore, introducing a
&self
makes it clear that the function requires an underlying object.
are quite vital. And supporting them along seems not to be a big deal.
I always prefer best possible user experience.
@tyranron Thanks for your great ideas!
It's definitely desired to have this check in compile time, while not increasing compilation time a lot. I believe this can be solved by generating a unique sub-module for each field, so if we have duplication, rustc will complain about duplication.
Sounds awesome! I didn't thought of that before.. if I understand correctly, we can generate a dummy struct
for each field (eg: __juniper_Obj_fullName
), similar to the markers. Is that the right way to do?
Beside, I actually prefer enforcing &self
for field resolver functions, not only increase the consistency but also make it more obvious that it is a field resolver
for a GraphQLObject
instead of an entry point (resolvers for Query / Mutation / Subscription) to the graph.
@zhenwenc
if I understand correctly, we can generate a dummy struct for each field (eg: __juniper_Obj_fullName), similar to the markers
This can be a struct too, but I see a bit more use in generating modules. See the second example above to understand why.
Beside, I actually prefer enforcing &self for field resolver functions, not only increase the consistency
But what if field doesn't really requires &self
? What if it's something like associated constant or totally context-dependent thing (like query depth quota for debuging stuff)? In this case &self
shows relevance which is really not there. Also, &self
may disallow at all any possible constant evaluation for fields.
but also make it more obvious that it is a
field resolver
for aGraphQLObject
.
In my practice having #[graphql_object]
on top of impl
block is quite enought.
But, if we want more distinction, I think there is a reason to consider the following variant, and drop #[graphql_object]
macro altogether:
#[derive(GraphQLObject)]
#[graphql(
name = "MyObj",
description = "obj descr",
scalar = DefaultScalarValue,
)]
struct Obj {
regular_field: bool,
#[graphql(
name = "renamedField",
description = "descr",
deprecated = "field deprecation"
)]
c: i32,
#[graphql(ignore)]
hidden: bool,
}
impl Obj {
#[graphql_field(scalar = DefaultScalarValue)]
const fn const_field() -> &str {
"const field"
}
#[graphql_field(scalar = DefaultScalarValue)]
fn hidden(&self) -> bool {
self.hidden
}
fn not_a_field(&self) {
panic!("I'm not a field!")
}
}
I noticed that one use-case is not covered by this design. The "old" design was quite handy in situation where the struct is private. Instead of deriving the GraphQLType it was possible to "implement" it, thus we do not have to wrap the type. In situations where types come from external crates, GraphQLType can be implemented without changes in the external crate. The design allows some kind of layered implementations. For example, someone create a database model and provides two API's, one of them is GraphQL. The other API does not know about Juniper, thus does not require the implemented traits.
Not sure how common this design is. Maybe impl
blocks should still be able to generate code?
Regarding the optional &self
. Based on your example and the fact that functions should be preserved, &self
must be able to be optional. Otherwise it would be useless to preserve the methods. Originally I understood your first example differently. I assumed the whole impl
block is optional since the derive
marco is doing all the work. But your idea is that if there is directive at the top of the impl, then all functions are fields, otherwise only marked functions are fields?!
@jmpunkt
But your idea is that if there is directive at the top of the impl, then all functions are fields, otherwise only marked functions are fields?!
Yes. I'm even in favor of leaving the second variant only. To have a computable field, we mark a concrete method rather than an impl
block.
Hmmm... thinking about it a bit more shows a downside of this way, as we cannot know about Obj
type implicitly in this case (macro won't be provided with that kind of info). So, will be made to do this:
impl Obj {
#[graphql_field(object = Obj, scalar = DefaultScalarValue)]
const fn const_field() -> &str {
"const field"
}
#[graphql_field(object = Obj, scalar = DefaultScalarValue)]
fn hidden(&self) -> bool {
self.hidden
}
fn not_a_field(&self) {
panic!("I'm not a field!")
}
}
Which is not quite ergonomic.
So, maybe the second variant really should be an optional one.
I noticed that one use-case is not covered by this design. The "old" design was quite handy in situation where the struct is private. Instead of deriving the GraphQLType it was possible to "implement" it, thus we do not have to wrap the type.
As I've written above, the described design has nothing to do with it and is fully orthogonal to such issues as the current design is. It doesn't disallow you manual implementations of GraphQLType
, nor disallows generating code for private structs. All it does - just generates GraphQLType
trait implementation in some clever way to unite static fields and computable ones. The fields even doesn't have to be pub
, so you may hide them if you don't want to expose in other APIs, while have been exposed in GraphQL schema along.
In term of generating dummy struct
for each field in object type to check duplicated fields on compile time, I am not quite sure if it works for all cases, for instance:
mod test_1 {
#[derive(juniper::GraphQLObjectInfo)]
#[graphql(scalar = juniper::DefaultScalarValue)]
pub struct Object {
field: i32,
}
/// Generated by `GraphQLObjectInfo` derive macro:
struct __juniper_Object_field;
}
mod test_2 {
use super::test_1::Object;
#[juniper::graphql_object(scalar = juniper::DefaultScalarValue)]
impl Object {
fn field(&self) -> i32 {
10
}
}
/// Generated by `graphql_object` macro:
struct __juniper_Object_field;
}
The above code will compile without any error, since the duplicated __juniper_Object_field
structs are located in different scope.
Beside, I am a bit conservative about using this approach. As a newbie to rust and as a normal user of the Juniper crate (without knowing its implementation details), I might be confused when seeing error message like this:
|
34 | struct __juniper_Object_field;
| ------------------------------ previous definition of the type `__juniper_Object_field` here
35 | struct __juniper_Object_field;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `__juniper_Object_field` redefined here
|
since these structs are generated by the library, instead of pointing me to the root caused of the issue. It would be way more useful if I see hints like this:
Manual declared field resolver "field" for type "Object" had conflicted with auto-derived field resolvers, you may want to skip the field with `#[graphql(skip)]`).
@zhenwenc
The above code will compile without any error, since the duplicated
__juniper_Object_field
structs are located in different scope.It would be way more useful if I see hints like this:
Manual declared field resolver "field" for type "Object" had conflicted with auto-derived field resolvers, you may want to skip the field with `#[graphql(skip)]`).
🤔... unfortunately, Rust's proc macros doesn't give any type level information, they work only with raw syntax tokens. So we're unable to deeply inspect the information about type outside current declaration, and seems to be that we're unable to ensure any uniqueness outside current declaration. So modules really don't work... meh 😞
The only way I see to resolve this, at the moment, is a not nice and bad hack, but it will work well and will allow to acomplish the described above:
We can generate a file during macro expansion, where to register info about fields for each object, and check for duplicates reading this file. It will allow to provide nice error messages, as we control it on macro expansion level. This file will be required only during macro expansion phase, to ensure global uniqueness of generated fields.
Cons of this approach are:
I would not recommend generating files and hoping that the compiler executes the macros in the right order. Randomly breaking compilation is annoying. It looks like Rust can not do what we want to do (maybe in the future?). There are currently three different options to go from here
@jmpunkt
and hoping that the compiler executes the macros in the right order.
Nope, the whole idea is not rely on that. Using the file is a way to achieve that for uniqueness checks, but the downsides are:
target/
dir with some special files (which isn't a big deal, as for me).However, investigating this a bit further, it seems that we don't need use files at all. A global static in proc macro crate will do the job, at least for a while. Need more investigation on a question...
At the end, we can support compile-time checks behind a experimental
feature gate or something like that.
Ah ok you use files for collisions checks only. I read
where to register info about fields for each object
wrong. Thanks for the clarification.
Is there anything special (or limited) regarding the creation of files during the compilation. I am not familiar with the internals during compilation.
Maybe this is a crazy idea, but if it is possible to auto-generate a separate crate then this crate contains our typing information. Instead putting the generate structs/modules in the current path we put them in a separate crate where we have control over the path. Maybe using build scripts to pre-process the source files?
My original intention was introducing a small improvement to the existing #[graphql_object]
macro, therefore I tried to apply minimal changes and avoid any breaking changes in the experimental implementation (https://github.com/graphql-rust/juniper/pull/652). If breaking changes and/or hacky solutions are required to achieve this improvement, I would vote for option 1 (do not implement this feature).
However, in practice, I believe this kind of improvement is necessary as field resolvers for object types are so commonly used and critical in GraphQL (otherwise it doesn't seem like a graph..), that said derive([GraphQLObject])
is simply not enough.
Personally, I don't mind doing runtime check (option 3) as I don't see any truely downside of this approach, especially comparing to the tradeoff for the other approaches..
Hi
We can say that today we cannot have the introspection of the structure, to obtain the fields, along with the possibility of having custom fields?
Is your feature request related to a problem? Please describe. I'm always frustrated when I need to replace:
by
because I lose the nice default field accessor for
name
and I have to manually add it to theimpl
:Describe the solution you'd like I'd like for
#[derive(juniper::GraphQLObject)]
and#[juniper::graphql_object]
to work together so I don't have to add the dummy accessors for every field in theimpl
block. I would imagine it is complicated to achieve this because both macros currently generate the sameimpl
block underneath.Describe alternatives you've considered I couldn't come up with any alternative to this :crying_cat_face:
Additional context This is extra painful when wrapping REST interfaces which have many simple fields (like strings) and a few complex fields (references to other resources).
P.S.:
juniper
is such a great crate! many thanks to all the contributors! :100: