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.

Sign up for GitHub

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

Jump to bottom

#1288 Query string char missing #1335

Merged
merged 8 commits into from
Sep 22, 2023
Merged

#1288 Query string char missing #1335

merged 8 commits into from
Sep 22, 2023

Conversation

jlukawska
Copy link
Contributor

@jlukawska jlukawska commented Sep 4, 2020 •
edited by raman-m
Loading

Fixes #1288

Proposed Changes

  • the regex that is used to replace placeholder name and value should be used also in the if condition before
  • The last commit fixed one more problem: when a query param was not the first one, it hadn't been removed.

@jlukawska jlukawska mentioned this pull request Mar 10, 2021
Closed
@jlukawska
Copy link
Contributor Author

  • The last commit fixed one more problem: when a query param was not the first one, it hadn't been removed.

@raman-m raman-m changed the title Fix/1288 query string char missing #1288 Query string char missing Jul 17, 2023
@raman-m raman-m changed the base branch from master to develop July 17, 2023 14:11
@raman-m raman-m self-requested a review July 17, 2023 15:57
raman-m
raman-m approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@raman-m raman-m left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlukawska
Hi J!
Thank you for the PR and bug fixing!
Good job! 😍

Also,
Could you try to Sync fork please?
I saw you've merged update develop #7 to update your develop branch.
Sync should allow to remove redundant merge commit from the diff, via rebasing. Sync operation during rebasing should remove this merge commit, I hope. And finally, you will have the same top commit.

@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 raman-m mentioned this pull request Jul 17, 2023
Closed
@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
@raman-m raman-m force-pushed the fix/1288-query-string-char-missing branch from f23ab16 to 69c950a Compare August 9, 2023 15:19
@raman-m raman-m requested review from raman-m and removed request for TomPallister August 9, 2023 15:25
@raman-m
Copy link
Member

raman-m commented Aug 9, 2023

The feature branch has been rebased onto ThreeMammals:develop successfully!
Code review has started...

@yangbocheng As a reporter of the bug #1288 ,
Could you verify this bug fixing solution please?
Thanks!

raman-m
raman-m approved these changes Aug 16, 2023
View reviewed changes
test/Ocelot.AcceptanceTests/RoutingWithQueryStringTests.cs
Comment on lines +284 to +285
.Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. But where is a And-check that the query string was copied?
Where is And-check that expected queryName is in query string?
What is the predicate of "copying"?
Does it mean if copying failed then the step ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) fails too?

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs
}

[Fact]
public void should_replace_query_string_but_leave_non_placeholder_queries_2()
Copy link
Member

@raman-m raman-m Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we cover the user scenario reported in 1288 by this test?

@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 Aug 16, 2023
@raman-m
Copy link
Member

raman-m commented Aug 16, 2023

@TomPallister
This PR is ready to merge.

@raman-m raman-m merged commit ec85b13 into ThreeMammals:develop Sep 22, 2023
2 checks passed
@raman-m raman-m mentioned this pull request Oct 3, 2023
Merged
@raman-m raman-m mentioned this pull request Jan 27, 2024
Closed
@raman-m raman-m mentioned this pull request Jul 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaynaldM RaynaldM RaynaldM approved these changes

@wast wast wast approved these changes

@raman-m raman-m raman-m approved these changes

@binarymash binarymash Awaiting requested review from binarymash

@jps jps Awaiting requested review from jps

@geffzhang geffzhang Awaiting requested review from geffzhang

@philproctor philproctor Awaiting requested review from philproctor

@anktsrkr anktsrkr Awaiting requested review from anktsrkr

@sharpn sharpn Awaiting requested review from sharpn

@abooraja abooraja Awaiting requested review from abooraja

@yangzhongke yangzhongke Awaiting requested review from yangzhongke

@thiagoloureiro thiagoloureiro Awaiting requested review from thiagoloureiro

@TomPallister TomPallister Awaiting requested review from TomPallister

Assignees

@jlukawska jlukawska

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

route -url char missing
4 participants
@jlukawska @raman-m @RaynaldM @wast