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

Nested Input annotated types within Factory defined inputs causes type conflicts #537

Open
oojacoboo opened this issue Dec 17, 2022 · 8 comments

Comments

@oojacoboo
Copy link
Collaborator

oojacoboo commented Dec 17, 2022

See #532.

Currently, factories require a Type annotation on the target class. This is problematic if a field of that class is typed as an Input type. One possible solution here would be to update the factory logic to require an Input attribute. To be compatible with the existing input type mapping, we'd need to add a custom parameter on the Input attribute, to denote that the input type is created with a factory.

I also think we could probably just get rid of the Factory attribute, altogether and have the Input property point to the factory method instead.

I think for most people, a factory isn't needed at all. A constructor on a DTO style Input defined class is more than sufficient.

Consider the following, where factory takes a callable.

#[GraphQLite\Input(factory: [FooInputFactory::class, 'build'])
class FooInput
{
    public function __construct(
        public readonly ID $id,
    ) {}
}

class FooInputFactory
{
    public function build(ID $id): FooInput
    {
        return new FooInput($id);
    }
}
@mdoelker
Copy link
Contributor

mdoelker commented Dec 19, 2022

Can you give a code example to better understand the problem? I have many factories in use that map input types to models (without any annotation) just fine. You mentioned in #532 however that the issue could be linked to using input types within input types, so that might be the problematic combination.

@oojacoboo
Copy link
Collaborator Author

oojacoboo commented Dec 19, 2022

@mdoelker Building off that last example. Here is how it is currently. You cannot mix factories and Input annotated classes together. A factory requires that you annotate the input type class with a Type annotation. Which is really wrong. That should be for an output type, not an input type. And, I haven't checked the schema to check, but it's possible it's also generating an output type for that class as well.

#[GraphQLite\Type)
class FooInput
{
    public function __construct(
        public readonly ID $id,
    ) {}
}

class FooInputFactory
{
    #[GraphQLite\Factory]
    public function build(ID $id): FooInput
    {
        return new FooInput($id);
    }
}

#[GraphQLite\Input)
class BarInput
{
    public function __construct(
        #[GraphQLite\Field]
        public readonly ID $id,

        #[GraphQLite\Field]
        public readonly FooInput $foo,
    ) {}
}

@mdoelker
Copy link
Contributor

mdoelker commented Dec 19, 2022

This might be a misunderstanding of the role of a factory. Returning an Input-annotated class from a factory does not make much sense to me, as the factory itself represents the GraphQL input type. You use one or the other.

Let's imagine your build method would return a Foo class which is a model in your application. Then the parameters of that build method become the fields of the generated input type and its name will be Foo + Input (see https://github.com/thecodingmachine/graphqlite/blob/master/src/NamingStrategy.php#L83). Your mutation just type-hints the model and the factory takes care of creating that instance for you.

// This is our plain old model.
class Foo
{
  public function __construct(
    public readonly string $id,
    public readonly string $value,
  )
  {}
}

// The class holding the factory that represents the GraphQL input type.
// It convert the values from the input type to a Foo instance.
class FooFactory
{
  #[Factory]
  public function createFoo(ID $id, string $value): Foo
  {
    return new Foo((string) $id, $value);
  }
}

// The class mapping a Foo instance to a GraphQL output type.
#[Type(class: Foo::class)]
class FooType
{
  public function getId(Foo $foo): ID
  {
    return new ID($foo->id);
  }

  public function getValue(Foo $foo): string
  {
    return $foo->value;
  } 
}

// The class holding the mutation. The incoming Foo instance is mapped with FooFactory,
// the returned Foo instance is mapped via FooType.
class Controller
{
  #[Mutation]
  public function someMutation(Foo $foo): Foo
  {
    // do something with $foo

    return $foo;
  }
}

Your factory returns an Input-annotated class which is redundant. You should use the FooInput class directly in the method representing your mutation:

class Controller
{
  #[Mutation]
  public function someMutation(FooInput $foo): string
  {
    return $foo->value;
  }
}

@mdoelker
Copy link
Contributor

That said, I'm not sure about the BarInput use case where a field is another input type. I never had the need to do that, but this seems like it should be supported. I will try this later.

@oojacoboo
Copy link
Collaborator Author

@mdoelker That's correct. I removed the #[Field] attribute that was on the FooInput class. That isn't needed.

As for the Type or the Input annotation needing to be on the target class - yea, they're pointless really, as the Factory is what is defining the actual input type. At any rate, requiring a Type annotation, as it currently is, is certainly not ideal.

All of this is a result of factories being the only way you could create input types, initially. The Input annotation didn't exist, so the Type annotation was used. In that context, it makes sense. Technically, both input and output are types. However, as it is now, a Type annotated class is really an output and Input is an input.

But yea, the issue is when you're mixing them with a nested input, which is what happened in our case as we began slowly migrating from factories to using Input annotated classes.

@oojacoboo
Copy link
Collaborator Author

Maybe we should just remove the requirement for a target class to have any annotations at all, and solely rely on the Factory annotated method. That would be a BC friendly change that should resolve the nested input type issue as well.

@mdoelker
Copy link
Contributor

In my example above, which represents how I use factories in my codebase, the target class has no annotations and knows nothing about graphqlite. So that works perfectly well already.

@oojacoboo
Copy link
Collaborator Author

@mdoelker hmm, maybe it was only requiring the annotation because of the nested input then. We've now migrated all our factories over to Input annotated classes and the issue no longer exists. I guess nested inputs really aren't going to be all that common and mixing Input annotated classes with Factory built inputs is also less likely.

We need a testcase to replicate this issue.

@oojacoboo oojacoboo changed the title Review role of factories in creating Input types Nested Input types with Factory defined inputs causes type conflicts Dec 19, 2022
@oojacoboo oojacoboo changed the title Nested Input types with Factory defined inputs causes type conflicts Nested Input annotated types within Factory defined inputs causes type conflicts Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants