Skip to content

#1288 Query string char missing #1335

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

Merged
raman-m merged 8 commits into from
Sep 22, 2023
Merged

#1288 Query string char missing #1335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c56d079
fix char missing when query string contains the placeholder name and …
jlukawska Sep 4, 2020
5cb55ec
fix the regex problem with removing a query param when it is not the …
jlukawska Mar 12, 2021
1b43f19
Remove and Sort Usings
raman-m Jul 17, 2023
b8d748e
CA1845 Use span-based 'string.Concat' and 'AsSpan' instead of 'Substr…
raman-m Jul 17, 2023
b147b43
IDE0057 Substring can be simplified.
raman-m Jul 17, 2023
d531718
IDE0057 Substring can be simplified.
raman-m Jul 17, 2023
69c950a
IDE0042 Variable declaration can be deconstructed.
raman-m Jul 17, 2023
48e65da
Merge branch 'develop' into fix/1288-query-string-char-missing
raman-m Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 27 additions & 34 deletions src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,16 @@
using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

using Ocelot.Configuration;

using Ocelot.DownstreamRouteFinder.UrlMatcher;

using Ocelot.Logging;

using Microsoft.AspNetCore.Http;

using Ocelot.Middleware;
using Ocelot.Request.Middleware;

using Ocelot.Responses;

using Ocelot.DownstreamUrlCreator.UrlTemplateReplacer;

using Microsoft.AspNetCore.Http;
using Ocelot.Configuration;
using Ocelot.DownstreamRouteFinder.UrlMatcher;
using Ocelot.DownstreamUrlCreator.UrlTemplateReplacer;
using Ocelot.Logging;
using Ocelot.Middleware;
using Ocelot.Request.Middleware;
using Ocelot.Responses;
using Ocelot.Values;
using System;
using System.Collections.Generic;
using System.Text.RegularExpressions;
using System.Threading.Tasks;

namespace Ocelot.DownstreamUrlCreator.Middleware
{
Expand Down Expand Up @@ -66,11 +58,11 @@ public async Task Invoke(HttpContext httpContext)

if (ServiceFabricRequest(internalConfiguration, downstreamRoute))
{
var pathAndQuery = CreateServiceFabricUri(downstreamRequest, downstreamRoute, templatePlaceholderNameAndValues, response);
var (path, query) = CreateServiceFabricUri(downstreamRequest, downstreamRoute, templatePlaceholderNameAndValues, response);

//todo check this works again hope there is a test..
downstreamRequest.AbsolutePath = pathAndQuery.Path;
downstreamRequest.Query = pathAndQuery.Query;
downstreamRequest.AbsolutePath = path;
downstreamRequest.Query = query;
}
else
{
Expand Down Expand Up @@ -108,31 +100,32 @@ private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(Downst
{
var name = nAndV.Name.Replace("{", string.Empty).Replace("}", string.Empty);

if (downstreamRequest.Query.Contains(name) &&
downstreamRequest.Query.Contains(nAndV.Value))
{
var questionMarkOrAmpersand = downstreamRequest.Query.IndexOf(name, StringComparison.Ordinal);
downstreamRequest.Query = downstreamRequest.Query.Remove(questionMarkOrAmpersand - 1, 1);
var rgx = new Regex($@"\b{name}={nAndV.Value}\b");

var rgx = new Regex($@"\b{name}={nAndV.Value}\b");
if (rgx.IsMatch(downstreamRequest.Query))
{
var questionMarkOrAmpersand = downstreamRequest.Query.IndexOf(name, StringComparison.Ordinal);
downstreamRequest.Query = rgx.Replace(downstreamRequest.Query, string.Empty);
downstreamRequest.Query = downstreamRequest.Query.Remove(questionMarkOrAmpersand - 1, 1);

if (!string.IsNullOrEmpty(downstreamRequest.Query))
{
downstreamRequest.Query = '?' + downstreamRequest.Query.Substring(1);
downstreamRequest.Query = string.Concat("?", downstreamRequest.Query.AsSpan(1));
}
}
}
}

private static string GetPath(DownstreamPath dsPath)
{
return dsPath.Value.Substring(0, dsPath.Value.IndexOf('?', StringComparison.Ordinal));
{
int length = dsPath.Value.IndexOf('?', StringComparison.Ordinal);
return dsPath.Value[..length];
}

private static string GetQueryString(DownstreamPath dsPath)
{
return dsPath.Value.Substring(dsPath.Value.IndexOf('?', StringComparison.Ordinal));
{
int startIndex = dsPath.Value.IndexOf('?', StringComparison.Ordinal);
return dsPath.Value[startIndex..];
}

private static bool ContainsQueryString(DownstreamPath dsPath)
Expand Down
51 changes: 44 additions & 7 deletions test/Ocelot.AcceptanceTests/RoutingWithQueryStringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
using Microsoft.AspNetCore.Http;
using Ocelot.Configuration.File;
using System;
using System.Collections.Generic;
using System.Net;

using Ocelot.Configuration.File;

using Microsoft.AspNetCore.Http;

using TestStack.BDDfy;

using Xunit;

namespace Ocelot.AcceptanceTests
Expand Down Expand Up @@ -247,8 +243,49 @@ public void should_return_response_200_with_query_string_upstream_template_multi
.Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
.BDDfy();
}
}

// to reproduce 1288: query string should contain the placeholder name and value
[Fact]
public void should_copy_query_string_to_downstream_path_issue_1288()
{
var idName = "id";
var idValue = "3";
var queryName = idName + "1";
var queryValue = "2" + idValue + "12";
var port = RandomPortFinder.GetRandomPort();

var configuration = new FileConfiguration
{
Routes = new List<FileRoute>
{
new FileRoute
{
DownstreamPathTemplate = $"/cpx/t1/{{{idName}}}",
DownstreamScheme = "http",
DownstreamHostAndPorts = new List<FileHostAndPort>
{
new FileHostAndPort
{
Host = "localhost",
Port = port,
},
},
UpstreamPathTemplate = $"/safe/{{{idName}}}",
UpstreamHttpMethod = new List<string> { "Get" },
},
},
};

this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", $"/cpx/t1/{idValue}", $"?{queryName}={queryValue}", 200, "Hello from Laura"))
.And(x => _steps.GivenThereIsAConfiguration(configuration))
.And(x => _steps.GivenOcelotIsRunning())
.When(x => _steps.WhenIGetUrlOnTheApiGateway($"/safe/{idValue}?{queryName}={queryValue}"))
.Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
Comment on lines +284 to +285
Copy link
Copy Markdown
Member

@raman-m raman-m Aug 16, 2023

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?

.BDDfy();
}

private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, string queryString, int statusCode, string responseBody)
{
_serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, basePath, async context =>
Expand Down
54 changes: 39 additions & 15 deletions test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Threading.Tasks;

using Microsoft.AspNetCore.Http;

using Microsoft.AspNetCore.Http;
using Moq;

using Ocelot.Configuration;
using Ocelot.Configuration.Builder;
using Ocelot.DownstreamRouteFinder;
Expand All @@ -17,15 +10,14 @@
using Ocelot.Logging;
using Ocelot.Middleware;
using Ocelot.Request.Middleware;

using Ocelot.Responses;

using Ocelot.Values;
using Shouldly;

using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Threading.Tasks;
using TestStack.BDDfy;

using Ocelot.Values;

using Xunit;

namespace Ocelot.UnitTests.DownstreamUrlCreator
Expand Down Expand Up @@ -137,7 +129,39 @@ public void should_replace_query_string_but_leave_non_placeholder_queries()
.WithDownstreamRoute(downstreamRoute)
.WithUpstreamHttpMethod(new List<string> { "Get" })
.Build())))
.And(x => x.GivenTheDownstreamRequestUriIs("http://localhost:5000/api/subscriptions/1/updates?unitId=2&productId=2"))
.And(x => x.GivenTheDownstreamRequestUriIs("http://localhost:5000/api/subscriptions/1/updates?unitId=2&productId=2")) // unitId is the first
.And(x => GivenTheServiceProviderConfigIs(config))
.And(x => x.GivenTheUrlReplacerWillReturn("api/units/1/2/updates"))
.When(x => x.WhenICallTheMiddleware())
.Then(x => x.ThenTheDownstreamRequestUriIs("https://localhost:5000/api/units/1/2/updates?productId=2"))
.And(x => ThenTheQueryStringIs("?productId=2"))
.BDDfy();
}

[Fact]
public void should_replace_query_string_but_leave_non_placeholder_queries_2()
Copy link
Copy Markdown
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?

{
var downstreamRoute = new DownstreamRouteBuilder()
.WithDownstreamPathTemplate("/api/units/{subscriptionId}/{unitId}/updates")
.WithUpstreamHttpMethod(new List<string> { "Get" })
.WithDownstreamScheme("https")
.Build();

var config = new ServiceProviderConfigurationBuilder()
.Build();

this.Given(x => x.GivenTheDownStreamRouteIs(
new DownstreamRouteHolder(
new List<PlaceholderNameAndValue>
{
new PlaceholderNameAndValue("{subscriptionId}", "1"),
new PlaceholderNameAndValue("{unitId}", "2"),
},
new RouteBuilder()
.WithDownstreamRoute(downstreamRoute)
.WithUpstreamHttpMethod(new List<string> { "Get" })
.Build())))
.And(x => x.GivenTheDownstreamRequestUriIs("http://localhost:5000/api/subscriptions/1/updates?productId=2&unitId=2")) // unitId is the second
.And(x => GivenTheServiceProviderConfigIs(config))
.And(x => x.GivenTheUrlReplacerWillReturn("api/units/1/2/updates"))
.When(x => x.WhenICallTheMiddleware())
Expand Down