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

#653 Adding null check for downstreamResponse #1334

Merged
merged 5 commits into from
Sep 30, 2023
Merged

#653 Adding null check for downstreamResponse #1334

merged 5 commits into from
Sep 30, 2023

Conversation

ben-bartholomew
Copy link
Contributor

@ben-bartholomew ben-bartholomew commented Sep 4, 2020

Fixes #653

Null reference when pipeline terminates early

Proposed Changes

  • In ResponderMiddleware.cs added a check for null downstreamResponse to avoid getting a null ref exception by passing it to SetResponseOnHttpContext
  • Added a unit test to make sure SetResponseOnHttpContext does not get called in that case
  • Added an acceptance test to make sure that when the pipeline terminates early nothing blows up

@raman-m raman-m changed the title Fix for #653 / Adding null check for downstreamResponse #653 Adding null check for downstreamResponse Jul 17, 2023
@raman-m raman-m changed the base branch from master to develop July 17, 2023 14:41
@raman-m raman-m self-requested a review July 17, 2023 14:49
raman-m
raman-m previously approved these changes Jul 17, 2023
@raman-m raman-m added bug Identified as a potential bug accepted Bug or feature would be accepted as a PR or is being worked on labels Jul 17, 2023
@raman-m
Copy link
Member

raman-m commented Jul 17, 2023

@ben-bartholomew Hi Ben!
Thank you for the fix! 😻
Also, Could you Sync fork please? So, new develop branch will occur with all top commits!
More here: PR 1

@TomPallister Hey Tom!
A new peace of code requires your merging, please! 😉

@raman-m raman-m requested a review from TomPallister July 17, 2023 15:08
@ben-bartholomew
Copy link
Contributor Author

@ben-bartholomew Hi Ben! Thank you for the fix! 😻 Also, Could you Sync fork please? So, new develop branch will occur with all top commits! More here: PR 1

Done, thanks.

@raman-m
Copy link
Member

raman-m commented Jul 17, 2023

@ben-bartholomew
Thanks for merging!

Have you tried to Sync fork? Is the button enabled?
I don't see develop branch in your fork...

@ben-bartholomew
Copy link
Contributor Author

ben-bartholomew commented Jul 17, 2023

@raman-m

I used the sync fork button actually, and that seemed to auto-merge the PR as well.

I'm no GitHub expert but what's the goal is of getting develop to show up in my fork? This PR is coming from a separate bugfix branch, and you merged develop back into it so the bugfix branch should be up to date.

Everything seems good to go here to me, not sure what is missing.

Current state:

image

@raman-m
Copy link
Member

raman-m commented Jul 17, 2023

@ben-bartholomew commented on Jul 17, 2023:

I'm no GitHub expert but what's the goal is of getting develop to show up in my fork?

To be able to create feature branch from updated default branch which is develop in head repo.
But your default branch is master, because repo was forked before changing def branch from master to develop.
And you will be able to create from master only.
And you will need to resync to develop branch after a PR creation if the target is develop.

I see, Sync fork button works for current default branch, probably.
To fix that, in future, after PR merging you have to remove this fork, and re-fork once again to get new def branch which is develop one.


Update 1

Another user has reported to me a successful syn operation for all branches just today:
https://github.com/Burgyn/Ocelot/branches
And now develop is default branch in his forked repo! 👍

I have an idea!

aka life hack!

Could you create develop branch from your updated master, and after that make develop branch as default one in repo settings.
And after that try to sync fork once again please!
Let me know the results please!

@ben-bartholomew
Copy link
Contributor Author

To fix that, in future, after PR merging you have to remove this fork, and re-fork once again to get new def branch which is develop one.

I understand now. I assumed this was about changes necessary to get this PR merged since the discussion is happening in the PR thread, and not about getting my fork into some other desired state for potential future PRs.

Let me know the results please!

Easiest way to accomplish this IMO:

git remote add upstream https://github.com/ThreeMammals/Ocelot.git // add original repo as upstream remote
git fetch upstream                                                 // pull changes from original repo
git checkout -b develop upstream/develop                           // pull down develop branch locally
git push -u origin develop                                         // push develop branch to fork

Worked for me. Then downstream repos can set develop as their default branch in GitHub if they want.

@raman-m
Copy link
Member

raman-m commented Jul 18, 2023

@ben-bartholomew commented on July 17, 2023:

git remote add upstream https://github.com/ThreeMammals/Ocelot.git // add original repo as upstream remote
git fetch upstream                                                 // pull changes from original repo
git checkout -b develop upstream/develop                           // pull down develop branch locally
git push -u origin develop                                         // push develop branch to fork

Worked for me. Then downstream repos can set develop as their default branch in GitHub if they want.

Now I see develop branch in your fork, and it is default. Perfect!
Thanks for the script!
I will definitely keep this script for other users to recover their outdated repos.

@raman-m
Copy link
Member

raman-m commented Aug 8, 2023

@yilezhu As a bug reporter,

Could you review the code and verify this bug fix please?

@raman-m raman-m added needs feedback Issue is waiting on feedback before acceptance and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Aug 8, 2023
RaynaldM
RaynaldM previously approved these changes Aug 9, 2023
@raman-m raman-m merged commit e950cf2 into ThreeMammals:develop Sep 30, 2023
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs feedback Issue is waiting on feedback before acceptance labels Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An exception is thrown when the middleware is rewritten and then blocked back.
3 participants