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

Security attribute on extended type #653

Open
LubosRemplik opened this issue Feb 5, 2024 · 5 comments
Open

Security attribute on extended type #653

LubosRemplik opened this issue Feb 5, 2024 · 5 comments

Comments

@LubosRemplik
Copy link

LubosRemplik commented Feb 5, 2024

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

$argsByName = array_combine($argsName, $args);

Version: v5.0.3

@oojacoboo
Copy link
Collaborator

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
Copy link
Author

LubosRemplik commented Feb 5, 2024

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

oojacoboo commented Feb 5, 2024

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.

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