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

Add custom generator support to open api spec #912

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

olivernybroe
Copy link
Contributor

Hey 👋
This PR aims to add support for custom generators in Open API spec generation logic.

Problem

It is currently not possible to extend the generation of the Open API Spec. So if you add custom data into the custom field in the endpoint object, you cannot get it into the Open API Spec.

For my specific example, we had added a custom field for showing what scopes are required. It was easy to change the blade templates to show this custom field, but the spec file did not include it.

Possible solutions

There are many ways to solve this issue of course, and I am gonna list some of them and why I choose this direction.

Add OAuth support

As part of Open API spec, scopes are to be used together with OAuth. This problem can therefore be solved by adding a new authentication section for oauth to the package and hardcode into Open API spec generation the oauth values.

This is a great solution and could be tied directly to passport for easily getting the information, as it's fair to assume that people are prob. using passport for oauth setups.

Making Open API Spec generation extendable

By allowing users to extend the generation, Scribe does not need to keep the support for these specific custom fields people might have. My specific example is prob. just one of many scenarios of custom fields people would like to override.

Chosen solution

The reason why I went with custom generators support was because it seemed much more flexible and would allow for Passport support to still be created, but now how that logic isolated. It also allows for Passport support to be created as a third party package, reducing the maintenance of it from Scribe itself.

I Hope you like the concept and are willing to accept it. Totally understandable if not. This is also a breaking change in the OpenApiSpecWriter class as it removes a bunch of methods that were protected before, which users might have overriden. Not entirely sure what your policy/definition for breaking changes are.

Copy link
Contributor

@shalvah shalvah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. So what would expected use look like? Can you add some docs to the abstract class (as I assume users would be meant to extend from that), and the config file?

@olivernybroe
Copy link
Contributor Author

Hey 👋

Yeah of course, I have updated the class to include some docs in it.

An expected use case could for example be if you wanted to add a parameter to the components section, you could add docs like the following: (Added it as a test case also now)

class ComponentsOpenApiGenerator extends OpenApiGenerator
{
    public function root(array $root, array $groupedEndpoints): array
    {
        $parameters = Arr::get($root, 'components.parameters', []);
        $parameters = array_merge($parameters, [
            'slugParam' => [
                'in' => 'path',
                'name' => 'slug',
                'description' => 'The slug of the organization.',
                'example' => 'acme-corp',
                'required' => true,
                'schema' => [
                    'type' => 'string',
                ],
            ],
        ]);
        $root['components']['parameters'] = $parameters;

        return $root;
    }

    public function pathParameters(array $parameters, array $endpoints, array $urlParameters): array
    {
        $parameters['slug'] = ['$ref' =>  "#/components/parameters/slugParam"];

        return $parameters;
    }
}

and then in the config do

'openapi' => [
    ...
    'generators' => [
        ComponentsOpenApiGenerator::class,
    ],
],

@shalvah
Copy link
Contributor

shalvah commented Nov 20, 2024

I like the overall intention, but I'mma need a moment; I need to buy into the concept of generators here and plan a release strategy.

@olivernybroe
Copy link
Contributor Author

Yeah of course :) No rush at all. Just wanted to pitch the idea. Thank you for taking it into consideration

For now we are just replacing the OpenApiWriter in the container.

@shalvah
Copy link
Contributor

shalvah commented Dec 11, 2024

Heads-up: I'm currently planning the next major release, which will include this. But need to be in the hospital for a few weeks, so this may be on hold until next year.

@olivernybroe
Copy link
Contributor Author

Heads-up: I'm currently planning the next major release, which will include this. But need to be in the hospital for a few weeks, so this may be on hold until next year.

Thank you for informing me, glad to hear that you are planning to include this in next major release. Good luck with your hospital visit.

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

Successfully merging this pull request may close these issues.

2 participants