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

DownstreamUrlCreatorMiddleware a bug that caused a parameter loss #473

Closed
lfzm opened this issue Jul 17, 2018 · 10 comments · Fixed by #481
Closed

DownstreamUrlCreatorMiddleware a bug that caused a parameter loss #473

lfzm opened this issue Jul 17, 2018 · 10 comments · Fixed by #481
Labels
bug Identified as a potential bug good first issue Should be pretty easy to do help wanted Not actively being worked on. If you plan to contribute, please drop a note. merged Issue has been merged to dev and is waiting for the next release small effort Likely less than a day of development effort.

Comments

@lfzm
Copy link
Contributor

lfzm commented Jul 17, 2018

Expected Behavior / New Feature

Route conversion
Request address: http://localhost:5000/uc/Authorized/refresh?refreshToken=2288356cfb1338fdc5ff4ca558ec785118dfe1ff2864340937da8226863ff66d

Actual Behavior / Motivation for New Feautre

"Routes": [{
     "UpstreamPathTemplate": "/uc/Authorized/{server}/{action}",
      "UpstreamHttpMethod": [ "Post", "Get" ],
      "DownstreamPathTemplate": "/Authorized/{action}?server={server}"
}]

Steps to Reproduce the Problem

  1. The refreshToken parameter is missing after other routes are converted by DownstreamUrlCreatorMiddleware

2、 Debugging found that the problem occurred in this place

context.DownstreamRequest.Query = GetQueryString(dsPath);

Specifications

  • Version: 8.0
  • Platform: Asp.net core 2.1
  • Subsystem: Windows
@TomPallister
Copy link
Member

@aqa510415008 Which version of Ocelot are you using? I will look at this ASAP. Feels like a bug.

@TomPallister TomPallister added bug Identified as a potential bug help wanted Not actively being worked on. If you plan to contribute, please drop a note. good first issue Should be pretty easy to do small effort Likely less than a day of development effort. labels Jul 17, 2018
@lfzm
Copy link
Contributor Author

lfzm commented Jul 18, 2018

@TomPallister use ocelot 8.0

@lfzm
Copy link
Contributor Author

lfzm commented Jul 18, 2018

This modification can solve my problem, but I don't know if it will cause other problems, I hope to help you.

if (string.IsNullOrEmpty(context.DownstreamRequest.Query))
{
	context.DownstreamRequest.Query = GetQueryString(dsPath);
}
else
{
	context.DownstreamRequest.Query += GetQueryString(dsPath).Replace('?', '&');
}

@TomPallister
Copy link
Member

@aqa510415008 thanks, I can add failing test, make the change and see if all the tests still pass!

@lfzm
Copy link
Contributor Author

lfzm commented Jul 20, 2018

@TomPallister Is there any progress on this issue?

@TomPallister
Copy link
Member

@aqa510415008 sorry I haven’t been able to do this yet. Will try ASAP.

TomPallister pushed a commit that referenced this issue Jul 20, 2018
@TomPallister TomPallister added the merged Issue has been merged to dev and is waiting for the next release label Jul 20, 2018
@TomPallister TomPallister reopened this Jul 20, 2018
@lfzm lfzm closed this as completed Jul 21, 2018
@TomPallister
Copy link
Member

@aqa510415008 I will close this when it is in the main nuget package 😄

@TomPallister TomPallister reopened this Jul 21, 2018
@TomPallister
Copy link
Member

Released in version 8.0.1

@lfzm
Copy link
Contributor Author

lfzm commented Jul 21, 2018

@TomPallister Thank you for your efforts

@TomPallister
Copy link
Member

no problem, let me know if it doesnt work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug good first issue Should be pretty easy to do help wanted Not actively being worked on. If you plan to contribute, please drop a note. merged Issue has been merged to dev and is waiting for the next release small effort Likely less than a day of development effort.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants