Generate code as part of "composer install"

chillu commented 3 years ago

This makes it consistent with the vendor-expose approach, and ensures that both new and upgrading devs have a good experience (especially when they don't read upgrading guides).

As a reminder, there's the following ways of generating GraphQL code. Only the

Explicitly through sake dev/graphql/build on dev envs and committing the result to the codebase. Multi server safe.
Implicitly through dev/build (on dev or production runtime envs). Not multi server safe.
Explicitly Through sake dev/graphql/build on production runtime envs. Not multi server safe.
Implicitly through /graphql handlers (with SilverStripe\GraphQL\Controller::$autobuildSchema defaulting to true). Not multi server safe.

We propose adding a new default method:

On composer install and composer update. That only works for GraphQL configuration added/updated through modules, you'd still need sake dev/graphql/build to develop your own APIs. But the advantage here is that these commands already run as part of deployment processes, and likely would transparently include the new .graphql/ folder.

Note: Our own platforms run composer install --no-scripts && composer vendor-expose for security reasons, so we'll still need to add a new command to those.

Inclusion Options

There are various options on how to introduce this new logic

Inclusion Option 1: Require silverstripe/graphql-composer-plugin in silverstripe/graphql. This means everyone will run GraphQL code generation on composer install and composer update by default when upgrading to CMS 4.9, unless they opt out via SS_GRAPHQL_COMPOSER_BUILD_SCHEMAS=''. The opt-out would only be required if the generated GraphQL code is checked in to the codebase (ganky), or if there are alternative ways to generate it during deployment or at runtime in production.

Inclusion Option 2: Require silverstripe/graphql-composer-plugin in silverstripe/installer. This means only new installs will get it by default, and we'd strongly recommend opt-in to any sites upgrading to CMS 4.9. A bit more "room for error" in this new implementation, but it'll lead to lots of confusion when the "runtime fallbacks" described above kick in and behave inconsistently between servers.

Inclusion Option 3: Add this logic to the silverstripe/recipe-plugin instead of a new composer plugin, which is already required by framework. In this case, we need to make the code generation a no-op in case graphql isn't installed.

Tasks

[ ] Decide on a inclusion option (see above) and create pull request for it
[ ] Prefer database-less run via sake dev/graphql/build
[ ] Implement Databaseless Option 4: Move CoreKernel->validateDatabase() into DB::get_conn()
[ ] Add NullDatabase to SilverStripe\GraphQL\Dev\DevelopmentAdmin (add more specific friendly error to instance)
[ ] Throw less specific exception ("using database, no config found") in DB::get_conn(). Friendly error pointing to install docs.
[ ] Remove redirectToInstaller() logic, doesn't work in 99% of installs anyway (not running https://github.com/silverstripe/silverstripe-installer-wizard)
[ ] Remove custom error message from NullDatabase, throw custom exception and catch it in GraphQL build instead
[ ] Shim $_SERVER in GraphQL plugin to avoid error handling issues with HTTPOutputHandler outside of cli-script.php
[x] Decide on using NullDatabase vs. http://github.com/tractorcow/silverstripe-proxy-db
[x] Fix "undefined SERVER_NAME" due to HTTPOutputHandler handling CLI requests (and assuming HTTP state set)
[ ] Document composer install approach on docs.ss.org (if applicable)
[ ] Document database-less GraphQL resolver workarounds on docs.ss.org

Pull Requests

Testing this feature

Shortcut to set up a project for this (mostly for my own reference):

composer create-project silverstripe/installer 4.x-dev test
cd test
composer config repositories.framework vcs https://github.com/open-sausages/silverstripe-framework.git
composer config repositories.versioned vcs https://github.com/open-sausages/silverstripe-versioned.git
composer config repositories.graphql vcs https://github.com/open-sausages/silverstripe-graphql.git
composer require silverstripe/framework:'dev-pulls/nulldatabase as 4.x-dev' silverstripe/graphql:'dev-pulls/explicit-model-class as 4.x-dev' silverstripe/versioned:'dev-pulls/avoid-queries-on-build as 1.x-dev'
composer require silverstripe/graphql-composer-plugin:dev-pulls/remove-sake-dep

Now you can simulate a composer install run:

composer run-script post-install-cmd

There's more validation about operating this change on Silverstripe's own platforms in an internal ticket

chillu commented 3 years ago

From https://getcomposer.org/doc/articles/scripts.md

Note: Only scripts defined in the root package's composer.json are executed. If a dependency of the root package specifies its own scripts, Composer does not execute those additional scripts.

https://getcomposer.org/doc/articles/plugins.md

Plugin packages are automatically loaded as soon as they are installed and will be loaded when Composer starts up if they are found in the current project's list of installed packages. You may pass the --no-plugins option to Composer commands to disable all installed plugins. This may be particularly helpful if any of the plugins causes errors and you wish to update or uninstall it.

We don't require our current plugins (silverstripe/vendor-plugin and silverstripe/recipe-plugin) in projects root composer.json, which is proof that we can solve GraphQL the same way - as long as composer install is executed without --no-plugins

chillu commented 3 years ago

Update on this: There's a composer plugin now. The problem is that executing through sake and even just booting the kernel requires a database connection, which means we can't run this a build environment (e.g. CI) that's separate from the runtime environment - see WIP pull request.

michalkleiner commented 3 years ago

I think the 'Inclusion Option 1' makes the most sense, it seems to be the closest to the module that actually works with it. Installer or recipe-plugin are too far from the problem from my perspective.

You've already mentioned the edge case where the generation will need to be compatible with codebases where the generated code was already committed to the repo. I'd also check how it behaves on fresh installs with pre-existing project code defining custom types.

chillu commented 3 years ago

Had a long chat with Aaron, and we've tried to isolate the need for a database-less execution from GraphQL code gen a bit, which required choosing between a few different options for core behaviour.

Overview

At its core, Silverstripe is a web application framework, and since the ORM is bundled into the "core" the assumption of a valid database connection is pretty strong. The database connection is established lazy (when DB::get_conn() is first called, usually as part of executing a query. But the configuration for this connection is validated on Kernel->boot().

There are uncommon but still valid scenarios where a database connection is not available:

GraphQL code generation on deployment rather than runtime environments (details). This triggered the discussion here.
Config and class manifest generation as part of a deployment package, to speed up instances with this deployment package coming into service. This is especially relevant for auto scaling new instances, where every second counts
Code validation and introspection tooling built into framework (e.g. validating YAML config as part of CI)
Speed up unit tests which don't require a database (although given the lazy connection already in place these gains will be marginal)

There is a WIP pull request for adding a NullDatabase to framework, a fake that throws exceptions if the database is used. In the first scenario (GraphQL code gen) it can point at docs to describe why querying is a bad idea in this context. It also relies on a DatabaselessKernel to avoid boot failures due to lack of valid database config. These classes are marked as @internal, and are used in a GraphQL composer plugin, rather than in cli-script.php. Ideally we could enable cli-script.php (sake) to opt out of database usage as well.

Databaseless Options

Databaseless Option 1: Make CoreKernel bootstrap methods composable

This would be the most pluggable (for custom cli-script.php and index.php). But it also exposes core internals: We couldn't add a new Bootstrapper to core afterwards. It would also extend the public API surface, and break custom Kernel implementations if we enforced the new API via the Kernel interface.

class CoreKernel
{

    protected $bootstrappers = [
        SilverStripe\Core\Bootstrapper\PHP::class,
        SilverStripe\Core\Bootstrapper\Manifest::class,
        SilverStripe\Core\Bootstrapper\DatabaseConfig::class,
        // ...
    ];  

    public function setBootstrappers(array $bs)
    {
        $this->bootstrappers = $bs;
    }
}

Databaseless Option 2: Add behaviour modifiers to CoreKernel

Big API surface increase, but retains our flexibility to alter boot logic in core afterwards (unlike Option 1).

class CoreKernel
{
    protected $bootDatabase = true;

    public function boot()
    {
        $this->bootPHP();

        if ($this->bootDatabase) {
            $this->bootDatabase()
        }
    }

}

Databaseless Option 3: Subclass CoreKernel

Special purpose kernel, which worked when we just used it in the GraphQL composer plugin. It's messier now that we need it in cli-script.php.

class DatabaselessKernel extends CoreKernel
{
    public function boot()
    {
        $this->bootPHP();
        // leave out $this->bootDatabase()
        // ...
    }
}

Databaseless Option 4: Lazy database validation

Move validateDatabase out of CoreKernel into DB, and perform config validation on DB::get_conn() instead. This method avoids explicitly signaling the intent to boot without a database (e.g. sake dev/graphql/build --no-database), and doesn't require changes on cli-script.php or index.php.

This will force us to also remove the non-functional redirectToInstaller() call, which is acceptable because the installer-wizard is broken and unsupported anyway.

GraphQL Code Gen Behaviour

With any of the options proposed the framework can boot without requiring a database, or failing on a missing database config (until the first query). We also need to call DB::set_conn(new NullDatabase()) in SilverStripe\GraphQL\Dev\DevelopmentAdmin. This ensures that we can throw specific error messages when the database is used as part of the GraphQL code gen, regardless of how this was triggered (composer plugin vs. sake).

Conclusion

Databaseless Option 4, since it avoids API changes as well as changes to highlevel usage (such as sake --no-database). I've added a few more tasks to the issue description to work through the implementation.

silverstripe / silverstripe-graphql