Skip to content

Fix failure on REST requests with $filter and $orderby parameters #3664

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

Open
RubenCerna2079 wants to merge 5 commits into
base: main
Choose a base branch
from
+37 −1
Open

Fix failure on REST requests with $filter and $orderby parameters #3664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a09d06
Fix bug
RubenCerna2079 Jun 12, 2026
024e0b0
Fix bug
RubenCerna2079 Jun 12, 2026
a5e72f4
Add testing
RubenCerna2079 Jun 12, 2026
f83f122
Address comments by copilot
RubenCerna2079 Jun 12, 2026
d2eee0d
Address comments based on copilot
RubenCerna2079 Jun 12, 2026
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
3 changes: 2 additions & 1 deletion src/Core/Services/RestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ RequestValidator requestValidator
DatabaseObject dbObject = sqlMetadataProvider.EntityToDatabaseObject[entityName];

QueryString? query = GetHttpContext().Request.QueryString;
string queryString = query is null ? string.Empty : GetHttpContext().Request.QueryString.ToString();
string queryString = query is null ? string.Empty : GetHttpContext().Request.QueryString.ToString()
.Replace("?%24", "?$").Replace("&%24", "&$"); //Add replacement in order to ensure '$' sign is present as expected only for the case where it is after a '?' or '&' signs.

@souvikghosh04 souvikghosh04 Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this be parsed using already available libraries instead of manually? we have something like this below context.RawQueryString = queryString; context.ParsedQueryString = HttpUtility.ParseQueryString(queryString); or using Microsoft.AspNetCore.WebUtilities.QueryHelpers.ParseQuery()

@souvikghosh04 souvikghosh04 Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be solved/fixed at root level to ensure downstream usage of the querystring is already valid.

@souvikghosh04 souvikghosh04 Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the trailing comment on RestService.cs:75 says what the code does but not why $ arrives URL-encoded in the first place, nor what PR #3080 changed that broke it. future readers will look at the Replace chain and have no idea whether they can remove it.


// Read the request body early so it can be used for downstream processing.
string requestBody = string.Empty;
Expand Down
35 changes: 35 additions & 0 deletions src/Service.Tests/Configuration/ConfigurationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4224,6 +4224,41 @@ public async Task OpenApi_InteractiveSwaggerUI(
}
}

/// <summary>
/// End to end test that validates that REST requests with OData query
/// options $filter and $orderby succeed to ensure no regression can occur.
/// </summary>
[TestMethod]
[TestCategory(TestCategory.MSSQL)]
public async Task TestForRestRequestsWithFilterAndOrderbyParameters()
{
// The configuration file is constructed by merging hard-coded JSON strings to simulate the scenario where users manually edit the
// configuration file (instead of using CLI).
string configJson = TestHelper.AddPropertiesToJson(TestHelper.BASE_CONFIG, BOOK_ENTITY_JSON);
Assert.IsTrue(RuntimeConfigLoader.TryParseConfig(
configJson,
out RuntimeConfig deserializedConfig,
replacementSettings: new(),
connectionString: GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL)));
string configFileName = "custom-config.json";
File.WriteAllText(configFileName, deserializedConfig.ToJson());
string[] args = new[]
{
$"--ConfigFileName={configFileName}"
};

using (TestServer server = new(Program.CreateWebHostBuilder(args)))
using (HttpClient client = server.CreateClient())
{
// Act
using HttpRequestMessage restRequest = new(HttpMethod.Get, "/api/Book?$orderby=id desc&$filter=publisher_id eq 1234");

@souvikghosh04 souvikghosh04 Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this explicitly tests the %24. the httpclient would internally normalize this, so this can pass with or without the fix. you might need to explicitly pass the encoded values in URI "/api/Book?%24orderby=id%20desc&%24filter=publisher_id%20eq%201234") and then do proper assertions.

@RubenCerna2079 RubenCerna2079 Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, the whole point of the test is to make an end-to-end scenario with values that a user would use. Adding the encoded values directly would defeat that purpose and would not help us catch any possible regressions in the future. DAB should be able to encode these values by itself and not throw any errors.

@souvikghosh04 souvikghosh04 Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the intent is fair but partially justifiable. this test doesn't hit the fix's code path. HttpClient won't percent-encode $ (it's a legal sub-delimiter per RFC 3986), so Request.QueryString never contains %24 here, the Replace calls never fire. you can verify by reverting the fix and confirming the test still passes.

DAB is the server. it doesn't encode. it decodes what arrives on the wire. The regression is specifically when a client sends %24filter. the test must send the encoded form.

Suggestion: keep both as [DataRow] inputs: the current URI as a smoke test, plus a %24-encoded variant as the actual regression guard.

using HttpResponseMessage restResponse = await client.SendAsync(restRequest);

// Assert - Verify REST response
Assert.AreEqual(HttpStatusCode.OK, restResponse.StatusCode, "REST request to auto-generated entity should succeed");

@souvikghosh04 souvikghosh04 Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this only checks HttpStatusCode.OK. a regression where $filter is silently dropped would still return 200 with an unfiltered list. can we also assert that the response body contains the rows you expect (publisher_id == 1234 only) and in the order requested.

}
}

/// <summary>
/// Test different loglevel values that are avaliable by deserializing RuntimeConfig with specified LogLevel
/// and checks if value exists properly inside the deserialized RuntimeConfig.
Expand Down