Skip to content

Conversation

Westie
Copy link

@Westie Westie commented Oct 24, 2023

I'm working on a project that involves autodocumenting API endpoints, as well as manually documenting endpoints that are not procedurally generated.

The purpose of this PR is to introduce a new middleware - RouteInformationMiddleware - that allows a developer to have a bit more control over the RouteInformation object.

In addition, as part of this PR, I would also like to propose that we can now optionally pass the RouteInformation instance to any operation builder.

Tests and doc changes supplied.

Furthermore: the code in #55, this has component middleware layer only running if there are already components assigned.

I personally think that behaviour is incorrect but I'd love your thoughts @vyuldashev and @nandi95

@vyuldashev
Copy link
Owner

Hi. I like the code, looks clean. I have two questions:

  1. What are the real world use cases of route middleware?
  2. RoutesBuilder breaks API, so a new major tag would be required. But I see no reason for that, we can still have this functionality in PathsBuilder and not bump the version.

@Westie
Copy link
Author

Westie commented Oct 24, 2023

Hi. I like the code, looks clean. I have two questions:

Thanks! I've been running a heavily hacked together version of this locally for a while now, but I've only just now got around to thinking about how to upstream things.

  1. What are the real world use cases of route middleware?

I'm currently using a third party Laravel library in quite a few projects that allows me to essentially abstract out how models, relationships and ultimately API endpoints/routes are defined - in what are essentially config files.

The project is https://laraveljsonapi.io

As information about what is considered an acceptable request body, expected response body, relationships etc is already well known, and more importantly, is very strictly defined - we can avoid the effort of manually describing each model, relationship, endpoint etc and through the use of Laravel Magic™, automatically generate this documentation. Very useful when working on projects with anywhere from 10 to 300 distinctive programmatically defined endpoints(!) to audit what's actually available (and if it can be pruned!)

A side bonus of this is that it creates time to write the docs for endpoints that aren't using that JSON:API package.

  1. RoutesBuilder breaks API, so a new major tag would be required. But I see no reason for that, we can still have this functionality in PathsBuilder and not bump the version.

I had a feeling this would be raised as a concern, however what I expected would have been raised was me changing the PathsBuilder::build method to have an arity of 3, as opposed to an arity of 2

The reason why I did this, ultimately, was an attempt to keep consistency with how the Generator class handles middlewares:

I'm struggling to find a way to pivot from config, to a route builder/collector with a way of passing middlewares across.

The only way I can think, would be to do something like:

<?php

namespace Vyuldashev\LaravelOpenApi\Builders;

class PathsBuilder
{
    ...

    public function build(string $collection, array $middlewares): array {
        $middlewares = collect($middlewares);

        $pathMiddlewares = $middlewares->filter(fn ($middleware) => is_a($middleware, PathMiddleware::class, true));
        $routeBuilderMiddlewares = $middlewares->filter(fn ($middleware) => is_a($middleware, RouteInformationMiddleware::class, true));

        return $this->routes($routeBuilderMiddlewares)
            ->filter(static function (RouteInformation $routeInformation) {
                ...
            })
            ->map(static function (RouteInformation $item) use ($routeBuilderMiddlewares) {
                foreach ($pathMiddlewares as $middleware) {
                    app($middleware)->before($item);
                }
                return $item;
            });
    }
}

...and pass in config like:

$config = [
    'collections' => [
        'default' => [
            'middlewares' => [
                'paths' => [
                    \Path\To\MyPathMiddleware::class,
                    \Path\To\MyRouteInformationMiddleware::class,
                ],
            ],
        ],
    ],
];

If you're okay with that, this would ensure 100% method compatibility, just makes things slightly less clearer.

@Westie
Copy link
Author

Westie commented Oct 24, 2023

PS: will probably change RouteInformationMiddleware to RouteBuilderMiddleware, but I'll do that when I've heard your thoughts on whether or not you're happy with two different middleware types being sent into the same flow

@Westie
Copy link
Author

Westie commented Oct 25, 2023

Update: I've misunderstood how Vyuldashev\LaravelOpenApi\Contracts\ComponentMiddleware works. I think to proceed we'll need to adjust the code so that flow also can support two middlewares types as well

@Westie Westie changed the title Addition of new middleware, RouteInformationMiddleware! Addition of new middleware, RoutesBuilderMiddleware! Oct 30, 2023
@Westie
Copy link
Author

Westie commented Oct 30, 2023

@vyuldashev I've changed how middlewares work internally, following the notes issued in the documentation. If users of this library have followed documentation correctly, then their middlewares will continue to work.

I have now made it so that multiple types of middlewares can be thrown into a builder and only the correct flow will be evaluated.

In addition I have made it so that there's no changes to any public methods, the only prototype change is a protected function which shouldn't be relied upon for prototype compatibility.

I will be very appreciative of your thoughts RE: these changes.

@Westie
Copy link
Author

Westie commented Nov 16, 2023

@vyuldashev I was wondering if you had the chance to review these changes?

@vyuldashev
Copy link
Owner

@Westie Hello, will definitely review soon, thanks.

@Westie
Copy link
Author

Westie commented Feb 20, 2024

Hi @vyuldashev, have you had a chance to review this PR yet? :-)

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