Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Don't use OnSendingHeaders #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JoachimC
Copy link

As we've decided to terminate the middleware stack and initiate creating the response, we shouldn't use OnSendingHeaders. Particularly to set the status code as middleware further down the stack can't see what response we've decided to return.

I appreciate this might be a breaking change for some people but I think it's the 'right' answer - so hopefully you might consider accepting this change.

…ing the response, we shouldn't use OnSendingHeaders. Particularly to set the status code as middleware further down the stack can't see what response we've decided to return.
@JoachimC JoachimC changed the title Don't use OnSettingHeaders Don't use OnSendingHeaders Mar 10, 2017
@stefanprodan
Copy link
Owner

To avoid a breaking change you could add an optional param to the middleware constructor and use a if to exclude the OnSendingHeaders section.

@JoachimC
Copy link
Author

Hi, thanks for considering my PR.

To me that seems like a shame - to introduce the complexity of a flag to switch a behaviour that I don't believe is correct. To me the current behaviour feels like a bug - and this is a fix to that bug.

So maybe this change could wait for a major release - where breaking changes might be allowable?

@lodicolo
Copy link

lodicolo commented Dec 4, 2019

I will concede that making this change breaks any code reliant on the last-minute mutation of the response, but...

The currently implemented behavior:

  • does not match that of the ThrottlingHandler which has an immediate mutation
  • mutates the response outside of the middleware's execution scope, preventing any middleware that runs after the fact from making accurate decisions based on the response and any further mutations would be discarded

Based on those two things, to me it appears as though the currently released implementation is invalid and a bug, and that the last-minute mutation behavior just should not be permitted (and can be achieved by attaching this middleware last if it is truly desired).

In short -- I agree with JoachimC.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants