Skip to content

Commit

Permalink
.Net: Fix for RestApiOperation.BuildQueryString (#6662)
Browse files Browse the repository at this point in the history
### Motivation and Context

We have noticed few cases where API calls were failing. Sample response:


"errors":{"since_create_time":["The value
'2024-05-01T00:00:00\u002B00:00' is not
valid."],"before_create_time":["The value
'2024-05-31T23:59:59\u002B00:00' is not valid."]}}


We are passing the value correctly to SK as 2024-05-01T00:00:00+00:00
but then it’s getting transformed to '2024-05-01T00:00:00\u002B00:00 in
[this code
here](https://github.com/microsoft/semantic-kernel/blob/f1f53f4d9db6aa8f963a5c6a1053101774a0b49e/dotnet/src/Functions/Functions.OpenApi/Serialization/OpenApiTypeConverter.cs#L30).

Then in this particular API these arguments are passed in the URL. The
final URL is this:
/campaigns?before_create_time=2024-05-31T23%3a59%3a59%5cu002B00%3a00&since_create_time=2024-05-01T00%3a00%3a00%5cu002B00%3a00

Now, I believe the problem is that everything would work just fine if
these values were passed as the request payload to the API, where API
would deserialize it correctly. But, since these are passed in URL, the
value is never deserialized and stays broken.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
  • Loading branch information
markwallace-microsoft committed Jun 11, 2024
1 parent c39eb1a commit af875b6
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ public static string Serialize(RestApiOperationParameter parameter, JsonNode arg
Verify.NotNull(parameter);
Verify.NotNull(argument);

if (parameter.Style != RestApiOperationParameterStyle.Form)
var style = parameter.Style ?? RestApiOperationParameterStyle.Form;
if (style != RestApiOperationParameterStyle.Form)
{
throw new NotSupportedException($"Unsupported Rest API operation parameter style '{parameter.Style}' for parameter '{parameter.Name}'");
}
Expand All @@ -35,7 +36,13 @@ public static string Serialize(RestApiOperationParameter parameter, JsonNode arg
return SerializeArrayParameter(parameter, argument);
}

// Handling parameters of primitive and removing extra quotes added by the JsonValue for string values.
// Handling parameters where the underlying value is already a string.
if (argument is JsonValue jsonValue && jsonValue.TryGetValue(out string? value))
{
return $"{parameter.Name}={HttpUtility.UrlEncode(value)}";
}

// Handling parameters of any arbitrary type by using JSON format without enclosing quotes.
return $"{parameter.Name}={HttpUtility.UrlEncode(argument.ToString().Trim('"'))}";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,112 @@ public void ShouldBuildResourceUrlWithoutQueryString()
Assert.Equal($"{fakeHostUrlOverride}/fake-path-value/", url.OriginalString);
}

[Fact]
public void ItShouldBuildQueryString()
{
// Arrange
var parameters = new List<RestApiOperationParameter> {
new(
name: "since_create_time",
type: "string",
isRequired: false,
expand: false,
location: RestApiOperationParameterLocation.Query),
new(
name: "before_create_time",
type: "string",
isRequired: false,
expand: false,
location: RestApiOperationParameterLocation.Query),
};

var sut = new RestApiOperation(
"fake_id",
new Uri("https://fake-random-test-host"),
"fake-path/",
HttpMethod.Get,
"fake_description",
parameters);

var arguments = new Dictionary<string, object?>
{
{ "since_create_time", "2024-01-01T00:00:00+00:00" },
{ "before_create_time", "2024-05-01T00:00:00+00:00" },
};

// Act
var queryString = sut.BuildQueryString(arguments);

// Assert
Assert.Equal("since_create_time=2024-01-01T00%3A00%3A00%2B00%3A00&before_create_time=2024-05-01T00%3A00%3A00%2B00%3A00", queryString, ignoreCase: true);
}

[Fact]
public void ItShouldBuildQueryStringWithQuotes()
{
// Arrange
var parameters = new List<RestApiOperationParameter> {
new(
name: "has_quotes",
type: "string",
isRequired: false,
expand: false,
location: RestApiOperationParameterLocation.Query)
};

var sut = new RestApiOperation(
"fake_id",
new Uri("https://fake-random-test-host"),
"fake-path/",
HttpMethod.Get,
"fake_description",
parameters);

var arguments = new Dictionary<string, object?>
{
{ "has_quotes", "\"Quoted Value\"" },
};

// Act
var queryString = sut.BuildQueryString(arguments);

// Assert
Assert.Equal("has_quotes=%22Quoted+Value%22", queryString, ignoreCase: true);
}

[Fact]
public void ItShouldBuildQueryStringForArray()
{
// Arrange
var parameters = new List<RestApiOperationParameter> {
new(
name: "times",
type: "array",
isRequired: false,
expand: false,
location: RestApiOperationParameterLocation.Query),
};

var sut = new RestApiOperation(
"fake_id",
new Uri("https://fake-random-test-host"),
"fake-path/",
HttpMethod.Get,
"fake_description",
parameters);

var arguments = new Dictionary<string, object?>
{
{ "times", new string[] { "2024-01-01T00:00:00+00:00", "2024-05-01T00:00:00+00:00" } },
};

// Act
var queryString = sut.BuildQueryString(arguments);

// Assert
Assert.Equal("times=2024-01-01T00%3A00%3A00%2B00%3A00,2024-05-01T00%3A00%3A00%2B00%3A00", queryString, ignoreCase: true);
}

[Fact]
public void ItShouldRenderHeaderValuesFromArguments()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void ItShouldCreateParameterForPrimitiveValue()
}

[Fact]
public void ItShouldCreateParameterForStringValue()
public void ItShouldCreateParameterForDateTimeValue()
{
// Arrange
var parameter = new RestApiOperationParameter(
Expand All @@ -95,6 +95,28 @@ public void ItShouldCreateParameterForStringValue()
Assert.Equal("id=2023-12-06T11%3a53%3a36Z", result);
}

[Theory]
[InlineData("2024-01-01T00:00:00+00:00", "2024-01-01T00%3a00%3a00%2b00%3a00")]
public void ItShouldCreateParameterForStringValue(string value, string encodedValue)
{
// Arrange
var parameter = new RestApiOperationParameter(
name: "id",
type: "string",
isRequired: true,
expand: false,
location: RestApiOperationParameterLocation.Query,
style: RestApiOperationParameterStyle.Form);

// Act
var result = FormStyleParameterSerializer.Serialize(parameter, JsonValue.Create(value));

// Assert
Assert.NotNull(result);

Assert.Equal($"id={encodedValue}", result);
}

[Theory]
[InlineData(":", "%3a")]
[InlineData("/", "%2f")]
Expand Down

0 comments on commit af875b6

Please sign in to comment.