Skip to content

Direct return of response by middleware seems not possible when using a condition #34

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

Closed
holtkamp opened this issue Sep 4, 2024 · 1 comment
Assignees

Comments

@holtkamp
Copy link
Contributor

holtkamp commented Sep 4, 2024

I have a middleware class which checks for the existence of a cache entry and either:

  1. cache entry is found, assign the content to the response object and return the response object directly / do not invoke the remaining middleware
  2. cache entry is not found, invoke the remaining middleware

Something like this:

class MyCacheMiddleware implements MiddlewareInterface
{
    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) : ResponseInterface
    {
        if($content = $this->getCacheContent($request)){
            $responseFactory = new ResponseFactory();
            $response = $responseFactory->createResponse();
            $response->getBody()->write($content);
            return $response; // No need to invoke remaining middleware: return response directly
        }
    }

    return $handler->handle($request); // Invoke remaining middleware
}

This works properly when adding it to Harmony using:

$harmony->addMiddleware(new MyCacheMiddleware());
$harmony->addMiddleware(new MyNextMiddleware1());
$harmony->addMiddleware(new MyNextMiddlewareN());

However when using a condition, it seems that the remaining middleware is always invoked, even when the response was returned directly:

$harmony->addCondition(new SomeCondition(), fn (Harmony $harmony) => $harmony->addMiddleware(new MyCacheMiddleware()));
$harmony->addMiddleware(new MyNextMiddleware1()); //Always invoked
$harmony->addMiddleware(new MyNextMiddlewareN()); //Always invoked

This also breaks the AuthenticationMiddleware in the documented example, since the returned response is not detected/used properly:

    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
    {
        // Return Error 401 "Unauthorized" if the provided API key doesn't match the expected one
        if ($request->getHeader("x-api-key") !== [$this->apiKey]) {
            return $this->errorResponsePrototype->withStatus(401); //Will not have effect?
        }

        // Invoke the remaining middleware if authentication was successful
        return $handler->handle($request);
    }

Now my question: is this known (undocumented) behaviour, or is it a bug? I suppose the latter, but I might be wrong.

Analysis
I think it is caused by the way the condition functionality is implemented. In case it evaluates to true, a new Harmony instance is created and executed, after which the remaining middleware is always processed:

$this->handle($this->request);

Possible solution
A solution might be to add remaining middleware to the new Harmony instance when a condition evaluates to true, something like:

//When condition evaluates to true:
$harmony = new Harmony($this->request, $this->response);
$callable($harmony, $this->request);
//Add remaining middleware of the current Harmony instance to the new/branched Harmony instance
for($i = $this->currentMiddleware + 1; $i< count($this->middleware); $i++){
    $harmony->middleware[] = $this->middleware[$i];
}
$harmony->run();

$this->request = $harmony->request;
$this->response = $harmony->response;

//$this->handle($this->request); //Not needed anymore

Not sure whether this works properly with nested conditions and such?

@kocsismate ?

@kocsismate
Copy link
Member

Hi @holtkamp,

Thank you for the report, I'll have a look in the coming days :)

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

No branches or pull requests

2 participants