thecodingmachine / graphqlite

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

Upgrading to 7.0.0 ends up with `dummyQuery`, `dummyMutation` and `dummySubscription` placeholders only in the Schema #696

Closed josefsabl closed 2 months ago

josefsabl commented 2 months ago

Hello, after upgrading to 7.0.0 (from 6.2.3) the thing stops working and I only get those dummy placeholders. I don't use any integration with frameworks, however I am using Slim Framework to provide middleware functionality "above" the GraphQL (e.g. for processing authentication headers) and GraphQLite is the handler of the Slims middleware stack.

The only BC break mentioned in docs is SchemaFactory::setClassNameMapper being renamed to SchemaFactory::setFinder. I don't call SchemaFactory::setClassNameMapper anywhere so I don't think I sould be affected.

I also noticed slight change in documentation example where ->addControllerNamespace('App\\Controllers\\') is replace with ->addControllerNamespace('App\\Controllers'). I checked and I am using it without the trailing backslash. Schema and type namespaces are same in my case.

This is my code dumbed down to a minimal version that works in 6.2.3 and doesn't work in 7.0.0:

        $schemaFactory = (new \TheCodingMachine\GraphQLite\SchemaFactory(
            $cache,
            $container,
        ))
            ->addControllerNamespace("MyApp\\Graph\\Schema")
            ->addTypeNamespace("MyApp\\Graph\\Schema");

        $graphQlServer = new \GraphQL\Server\StandardServer([
            'schema' => $schemaFactory->createSchema(),
            'context' => new \TheCodingMachine\GraphQLite\Context\Context(),
        ]);

        $requestHandler = new class($graphQlServer) implements \Psr\Http\Server\RequestHandlerInterface {
            public function __construct(
                private readonly \GraphQL\Server\StandardServer $server
            ) {
            }

            public function handle(
                \Psr\Http\Message\ServerRequestInterface $request
            ): \Psr\Http\Message\ResponseInterface {
                $result = $this->server
                    ->processPsrRequest(
                        $request,
                        (new \Slim\Psr7\Factory\ResponseFactory())->createResponse(),
                        new \Slim\Psr7\Stream(\Safe\fopen('php://temp', 'w'))
                    );

                if ($result instanceof \Psr\Http\Message\ResponseInterface) {
                    return $result;
                }

                throw new \LogicException('Unexpected result of GraphQL server encountered: `' . get_class($result) . '`');
            }
        };

        $middlewareDispatcher = new \Slim\MiddlewareDispatcher(
            $requestHandler
        );

        $middlewareDispatcher
            ->addMiddleware(new \Slim\Middleware\BodyParsingMiddleware())
        ;

        (new \Slim\ResponseEmitter())->emit(
            $middlewareDispatcher
                ->handle(
                    \Slim\Factory\ServerRequestCreatorFactory::create()->createServerRequestFromGlobals()
                )
        );

This is the full log from composer update:

  - Removing thecodingmachine/class-explorer (v1.1.0)   - Removing mouf/classname-mapper (v1.0.2)   - Upgrading symfony/cache (v6.4.10 => v7.1.3): Extracting archive   - Upgrading symfony/expression-language (v6.4.8 => v7.1.1): Extracting archive   - Installing kcs/class-finder (0.4.0): Extracting archive   - Upgrading thecodingmachine/graphqlite (v6.2.3 => v7.0.0): Extracting archive

My PSR Container is very thin custom adapter for Nette\DI\Container that is in production for years in many projects.

My PSR Cache is also very thin adapter for Nette\Caching\Cache. There used to be problems in history but more with performance and I don't think this might be the case. I also purged the cache 😉.

What might be wrong? Any help appreciated ♥.

oojacoboo commented 2 months ago

Well, clearly there is some class mapping issues going on. By default, the new class mapper uses the Composer autoload. Is your namespace autoloaded by Composer? If not, add it to your composer.json, or use the SchemaFactory::setFinder method to set a Finder instance that correctly loads the classes.

If that's handled correctly, you're going to need to manually debug the class loading to find out what's being loaded. Ultimately though, the new class-finder isn't finding your namespace it seems.

Also, if this is the case, we should probably add a validation check to see if the namespace exists. I think it'd probably be okay to throw an exception if a namespace provided doesn't have any classes found. I'm guessing this is quietly ignored currently.

josefsabl commented 2 months ago

Please, don't close it yet. I have the debugging in my TODO. Indeed all my classes are autoloaded by Composer (PSR-4). I want to Xdebug it but didn't have time.

oojacoboo commented 1 month ago

Feel free to reply back with details and we can reopen it.

josefsabl commented 1 week ago

So it turned out that the bundled class-finder doesn't look into composers fallback directories.

The obvious solution was to change:

  "autoload": {
    "psr-4": {
      "": "src/",
      "SomeNamespace\\": "packages/"
    }
  },

to

  "autoload": {
    "psr-4": {
      "": "src/",
      "MyApp\\Graph\\Schema": "src/MyApp/Graph/Schema",
      "SomeNamespace\\": "packages/"
    }
  },

Maybe it could be mentioned in the docs (unless the https://github.com/alekitto/class-finder/issues/22 is fixed).

I realize there is this:

  "autoload": {
    "psr-4": {
      "App\\": "src/"
    }
  }

But I humbly believe it is not obvious enough. Looks almost exactly like:

  "autoload": {
    "psr-4": {
      "": "src/"
    }
  }

and obviously APP is only an example.

Thanks. Problem solved on my end. Have a nice day :-)

oojacoboo commented 1 week ago

GraphQLite's class-finder implementation now uses Composer's autoload. So yea, whatever you define in your Composer config is going to be what's available to GraphQLite. I don't think there is much more to explain here. But if you'd like to improve the docs, a PR would be welcomed and appreciated. IMO, Composer's autoload functionality should be pretty straightforward and understood by most developers. And GraphQLite leveraging that autoload, and not trying to handle it some other way, is ideal.

josefsabl commented 1 week ago

Oh no, my classes ARE autoloaded by Composer. They are, however autoloaded by method, that is not implemented by class-finder. Fallback PSR4 directory.

josefsabl commented 1 week ago

Looks like class-finder has been fixed already 😅.

https://github.com/alekitto/class-finder/issues/22

oojacoboo commented 6 days ago

Oh, I see. Please submit a PR after testing to bump the class-finder package version.

josefsabl commented 5 days ago

Wow, during testing the fix for class-finder I found out that the class-finder includes all the files in the directories. This is crazy side effect to say at least.

I understand that mixing php scripts and classes is a bad practice. I have sort of a template written in php somewhere in the app and its contents started appearing in all the API responses 😆.

The problem didn't show up until the composer fallback directory got implemented.

josefsabl commented 5 days ago

Finally I decided to implement my own finder since there are problems with Kcs and there is straightforward way for me in my application.

But man, the Filter interface of the SchemaFactory::setFinder's parameter is complicated 👀 !

And you only use the iterator and inNamespace feature (which is hardly useful in custom finder as it can be easily managed there) 😭 .

Not to mention that it is from third party package I did not explicitly install. What happens if you decide to replace Kcs for some reason?

This should be pretty much it:

namespace TheCodingMachine\GraphQLite;

interface Finder {
  public function getClasses(): iterable;
}

PS: I plan to use Nette\RobotLoader to find classes. Maybe you should take a look too 😉.

josefsabl commented 2 days ago

Oh, I see. Please submit a PR after testing to bump the class-finder package version.

I don't think class-finder needs bumping. The fix has been released in 0.5.4. GraphQLite requires ^0.5.1 which will install the fixed version.

People for whom the version 0.5.1 works will continue with no change, new installs will have 0.5.4.

oojacoboo commented 1 day ago

@josefsabl adding the setFinder is a new feature that was added to allow for customization of Kcs mostly. It wasn't really fully intended to support other class finder implementations. However, from a design perspective, it probably should have been. If you'd like to improve on this with a managed interface and adapter for Kcs, that'd be something we can consider merging. It'd obviously be a BC break at this point, but I think that's fine - most people will be using the default finder implementation, and this is a newer addition as well.