thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
554 stars 95 forks source link

Security attribute on extended type #653

Open LubosRemplik opened 6 months ago

LubosRemplik commented 6 months ago

I am getting error when trying to use Security attribute on extended type class.

I have mutation class

#[Type]
final class RealtyMuation {
    // some methods
}

Then I have extended class, with Security attribute usage

#[ExtendType(RealtyMutation::class)]
final class RealtyCheckupMutation {
    #[Field]
    #[Security(
        expression: "is_granted('CAN_EDIT_REALTY', realtyId)",
        failWith: new AccessDeniedException(),
    )]
    public funtion startTechnicalInspection(RealtyMutation $mutation, UuidInterface $realtyId) 
    {
    }
}

Which gives me error

array_combine(): Argument #1 ($keys) and argument #2 ($values) must have the same number of elements

here https://github.com/thecodingmachine/graphqlite/blob/b63eaec119e6bf01146e5ca74795b79d98fbc97e/src/Middlewares/SecurityFieldMiddleware.php#L118

Version: v5.0.3

oojacoboo commented 6 months ago

Hmm, well, for starters, a muation class would typically be in your "controller" layer and isn't a "type" per se. I'm not really sure why you'd do this. Have you tried extending the RealtyMutation in PHP using extends? ExtendType is really there to allow you to extend your model, so as to not pollute the domain with GraphQL API related logic.

LubosRemplik commented 6 months ago

Well, the controller looks like this

final class RealtyMutationController
{
    public function __construct(
        private readonly RealtyMutation $realtyMutation,
    ) {
    }

    #[Mutation]
    public function realty(): RealtyMutation
    {
        return $this->realtyMutation;
    }
}

And we wanted to use ExtendType to simply divide long type classes into several logical ones.

But as far as you say above and from example I see here https://graphqlite.thecodingmachine.io/docs/extend-type the ExtendType is there only for Query, no Mutation?

Well it worked well until I had to add #[Security] attribute there and seems there are some related issues here too

oojacoboo commented 6 months ago

I'm still failing to see the value in using the ExtendType in a controller design. Why can't you use PHP to handle your logical separations?

LubosRemplik commented 6 months ago

OK, ignore it then, as I said it worked well and it was nice way to split long type classes, but I can create extra controllers for them and use as separate type.

Thanks, feel free to close, but still it looks like there is a bug with method getVariables at SecurityFieldMiddleware class since i got similar problem as in another 2 mentioned issues, what you think?

oojacoboo commented 6 months ago

I just read back over your examples again and noticed that you're calling your classes Mutations, but then annotating them as types. I don't understand this naming/design choice and that's the root of my confusion. You might want to rethink that. A mutation is a field, not a type. To call a class WhateverMutation is really misleading. I was assuming those were controllers for the actual mutations.

I see the issue now, the ExtendType injects the original type object as an argument into the method, but the SecurityFieldMiddleware isn't aware or expecting that. Happy to accept a PR on this.