Add support for route parameters

[alancolant]

Correction find schema when prefix contains parameters

Summary

I add the ability to set custom parameters to route prefix, like:

        'prefix'           => 'graphql/{game_slug}',

This fix use $request->route()->uri() instead of $request->getPathInfo() to fix the schemaName detection to always return null due to Str::startsWith

• …

---

Type of change:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

• Existing tests have been adapted and/or new tests have been added
• Add a CHANGELOG.md entry
• Update the README.md
• Code style has been fixed via composer fix-style

[mfn]

Based on the source code change and the description given, I still don't understand the purpose.

Please provide more examples and include a test showcasing the use-case.

[alancolant]

Ok, let's imagine I want a dynamic URL parameter at my Graphql entry point.

I'll start by setting the prefix at the config level to "graphql/{my_parameter}" like this:

return [
    ...
    'route'      => [
        // The prefix for routes; do NOT use a leading slash!
        'prefix'           => 'graphql/{my_parameter}',
        ...
    ],
    ...
]

The route generated will therefore make it possible to retrieve this parameter in this way in my queries:

$myParameter = request()->route()->parameter('my_parameter')

This works for the current version's default schema.

But in case I have several schemas, a "public" and a "private".

The controller tries to detect the schema using

if (!Str::startsWith($path, $routePrefix)) {
    return null;
}

In case the URL is "/graphql/parameter_arbitrary_value/private"

// When $path is retrieved using $request->getPathInfo(), the condition become

if (!Str::startsWith('/graphql/parameter_arbitrary_value/private', '/graphql/{my_parameter}')) { 
    return null; // Always return null and default schema is applied
}


//But, when $path is retrieved using $request->route()->uri(), the condition become
if (!Str::startsWith('/graphql/{my_parameter}/private', '/graphql/{my_parameter}')) { 
    return null;
}

//Everything works fine

[mfn]

Thanks for the clarification, that helps a lot!

Can you please add a test for the PR showing the change in action?

It will also help me figure which approach is the best.

Thanks

[alancolant]

Here, 2 tests have been added, one for the default schema and a second for the custom schema.

[alancolant]

I think a better approach would be to declare one and only one graphql route:

$prefix/{schemaName?}

And in your controller use

$schemaName = $request->route()->parameter('schemaName',$config->get('graphql.default_schema', 'default'));
//Instead of:
$routePrefix = $config->get('graphql.route.prefix', 'graphql');
$schemaName = $this->findSchemaNameInRequest($request, "/$routePrefix") ?: $config->get('graphql.default_schema', 'default');

But this is probably going to be a breaking change,
since there will no longer be the possibility to retrieve routes named 'graphql.custom' using route('graphql.custom'), but it will have to use route('graphql',['schemaName'=>'custom']) instead

[mfn]

💥 😄

I'm not opposed to bump a major version for a breaking change, if the improves are worth it.

The routing code has undergone some major change in recent years (if you're invested in this PR, I encourage you to check out the route.php history and the related changes in the controller) and received a LOT of duck tape fixes, so we really need to be very considerate about any changes here.

I already had the feeling this won't be an easy PR but now I'm confident it's not. I'm on vacation for a good part of the summer months and I need focus time to wrap my head around this before proceeding. I'll revisit once I've time, but that will likely take months :/

[mfn]

One side effect here I noticed that in \Rebing\GraphQL\Tests\Unit\EmptyRoutePrefixTest::testEmptyRoutePrefix the $path will turn into // (the test didn't break, I just noticed it and we should avoid this).

But this is without considering your other suggestion.

Feel free to go wild on the routing code and propose further commits with follow-up changes, as long as everything is covered with tests (they help me immensely figuring out where to go with this).

[mfn]

(I moved the PR in draft mode until we've consensus, I hope this is fine with you)

[alancolant]

Alright I am fine with it.
I will on my side try to implement the suggestion above.
I will just fix the problem you have detected for the moment.
Good holidays

[alancolant]

To avoid breaking changes, I have reworked the routes.php file so that the specified routes remain independent: graphql, graphql.custom, graphql.default.
However, instead of using a parameter at the route level, I have opted to automatically instantiate a middleware that injects the schemaName variable into $request->server for each route.

I find this approach more flexible as it allows us to eventually inject other information that can be retrieved in any controller using $request->server('graphql.*').

I also took the opportunity to declare and move the route declaration into the GraphQL.php class within a parseRoute function.

All these changes have been tested, and some tests have been modified to verify the presence of the GraphQLHttpMiddleware for each generated route.

I have also removed the findSchemaNameInRequest function from the controller since we no longer need to detect this name from the URL.

Looking forward to your feedback on this proposal.

---

I also think it could be interesting in a future major version to define a GraphQLContext class that will be automatically injected into all Queries.

In this case, for dynamic URLs, it will be possible to retrieve these parameters directly by using something like:

public function resolve($root, $args, GraphQLContext $context, ResolveInfo $resolveInfo, Closure $getSelectFields): LengthAwarePaginator
{
  $routeParameter = $context->getRouteParameters()['name_of_param'];
  $routeParameter = $context->getRouteParameter('name_of_param');
}