From 1e5f9e1687e9b54fefb94f87f5914598184f7cb3 Mon Sep 17 00:00:00 2001 From: Hamza Mahmood Date: Mon, 21 Jul 2025 18:17:17 +0500 Subject: [PATCH 1/5] fix(request-parameter): deduplicate parameters using ConcurrentDictionary --- APIMatic.Core.Test/Request/ParameterTests.cs | 53 +++++++++++++++++++ APIMatic.Core/Request/Parameters/Parameter.cs | 34 ++++++------ 2 files changed, 70 insertions(+), 17 deletions(-) create mode 100644 APIMatic.Core.Test/Request/ParameterTests.cs diff --git a/APIMatic.Core.Test/Request/ParameterTests.cs b/APIMatic.Core.Test/Request/ParameterTests.cs new file mode 100644 index 00000000..89e0d98b --- /dev/null +++ b/APIMatic.Core.Test/Request/ParameterTests.cs @@ -0,0 +1,53 @@ +using System.Threading.Tasks; +using APIMatic.Core.Request; +using APIMatic.Core.Request.Parameters; +using NUnit.Framework; + +namespace APIMatic.Core.Test.Request +{ + [TestFixture] + public class ParameterTests : TestBase + { + const string ServerUrl = "https://parameter.api.server.com"; + + [Test] + public void DuplicateHeaderKeys_Should_UseLastOneInRequestBuilder() + { + // Arrange + var parameters = new Parameter.Builder(); + + parameters + .Header(h => h.Setup("Authorization", "Basic OLD").Required()) + .Header(h => h.Setup("Authorization", "Basic NEW").Required()); + + var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); + + // Act + parameters.Validate().Apply(requestBuilder); + + // Assert + Assert.That(requestBuilder.headersParameters.TryGetValue("Authorization", out var value), Is.True); + Assert.That(value, Is.EqualTo("Basic NEW")); + } + + [Test] + public void DuplicateQueryKeys_Should_UseLastOneInRequestBuilder() + { + // Arrange + var parameters = new Parameter.Builder(); + + parameters + .Query(q => q.Setup("status", "pending").Required()) + .Query(q => q.Setup("status", "approved").Required()); + + var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); + + // Act + parameters.Validate().Apply(requestBuilder); + + // Assert + Assert.That(requestBuilder.queryParameters.TryGetValue("status", out var value), Is.True); + Assert.That(value, Is.EqualTo("approved")); + } + } +} diff --git a/APIMatic.Core/Request/Parameters/Parameter.cs b/APIMatic.Core/Request/Parameters/Parameter.cs index c6aa03e9..456a6557 100644 --- a/APIMatic.Core/Request/Parameters/Parameter.cs +++ b/APIMatic.Core/Request/Parameters/Parameter.cs @@ -5,9 +5,6 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; -using APIMatic.Core.Utilities; -using Microsoft.Json.Pointer; -using Newtonsoft.Json.Linq; namespace APIMatic.Core.Request.Parameters { @@ -22,9 +19,11 @@ public abstract class Parameter private Func valueSerializer = value => value; protected bool validated = false; protected string typeName; - + private string GetName() => key == "" ? typeName : key; + private string IdentifierKey => string.IsNullOrEmpty(key) ? $"_{nameof(Parameter)}_{Guid.NewGuid()}" : key; + public Parameter Setup(string key, object value) { this.key = key; @@ -80,7 +79,8 @@ internal virtual void Validate() /// public class Builder { - private readonly ConcurrentBag parameters = new ConcurrentBag(); + private readonly ConcurrentDictionary _parameters = + new ConcurrentDictionary(); internal Builder() { } @@ -93,7 +93,7 @@ public Builder Template(Action _template) { var template = new TemplateParam(); _template(template); - parameters.Add(template); + _parameters[template.IdentifierKey] = template; return this; } @@ -106,7 +106,7 @@ public Builder Header(Action _header) { var header = new HeaderParam(); _header(header); - parameters.Add(header); + _parameters[header.IdentifierKey] = header; return this; } @@ -120,7 +120,7 @@ public Builder AdditionalHeaders(Action _headers) { var headers = new AdditionalHeaderParams(); _headers(headers); - parameters.Add(headers); + _parameters[headers.IdentifierKey] = headers; return this; } @@ -133,7 +133,7 @@ public Builder Query(Action _query) { var query = new QueryParam(); _query(query); - parameters.Add(query); + _parameters[query.IdentifierKey] = query; return this; } @@ -146,7 +146,7 @@ public Builder AdditionalQueries(Action _queries) { var queries = new AdditionalQueryParams(); _queries(queries); - parameters.Add(queries); + _parameters[queries.IdentifierKey] = queries; return this; } @@ -159,7 +159,7 @@ public Builder Form(Action _form) { var form = new FormParam(); _form(form); - parameters.Add(form); + _parameters[form.IdentifierKey] = form; return this; } @@ -172,7 +172,7 @@ public Builder AdditionalForms(Action _forms) { var forms = new AdditionalFormParams(); _forms(forms); - parameters.Add(forms); + _parameters[forms.IdentifierKey] = forms; return this; } @@ -185,7 +185,7 @@ public Builder Body(Action _body) { var body = new BodyParam(); _body(body); - parameters.Add(body); + _parameters[body.IdentifierKey] = body; return this; } @@ -196,11 +196,11 @@ public Builder Body(Action _body) internal Builder Validate() { var missingArgErrors = new List(); - foreach (var p in parameters) + foreach (var p in _parameters) { try { - p.Validate(); + p.Value.Validate(); } catch (ArgumentNullException exp) { @@ -220,9 +220,9 @@ internal Builder Validate() /// internal void Apply(RequestBuilder requestBuilder) { - foreach (var p in parameters) + foreach (var p in _parameters) { - p.Apply(requestBuilder); + p.Value.Apply(requestBuilder); } } } From 98d99916b9a13e81cb65787cfc6aa28cdbd0c0a3 Mon Sep 17 00:00:00 2001 From: Hamza Mahmood Date: Mon, 21 Jul 2025 18:21:25 +0500 Subject: [PATCH 2/5] remove unsued import --- APIMatic.Core.Test/Request/ParameterTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/APIMatic.Core.Test/Request/ParameterTests.cs b/APIMatic.Core.Test/Request/ParameterTests.cs index 89e0d98b..e62b7267 100644 --- a/APIMatic.Core.Test/Request/ParameterTests.cs +++ b/APIMatic.Core.Test/Request/ParameterTests.cs @@ -1,5 +1,4 @@ -using System.Threading.Tasks; -using APIMatic.Core.Request; +using APIMatic.Core.Request; using APIMatic.Core.Request.Parameters; using NUnit.Framework; From 501011452023e4e85b39cbd756e878a937b2e7c2 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Tue, 22 Jul 2025 10:53:48 +0500 Subject: [PATCH 3/5] temp: disable coverage report uploading --- .github/workflows/test.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c813d329..65c3dc20 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -38,11 +38,11 @@ jobs: if: ${{ matrix.os == 'ubuntu-22.04' && matrix.dotnet == '7.0.x' }} run: dotnet test APIMatic.Core.Test/APIMatic.Core.Test.csproj -p:CollectCoverage=true -p:CoverletOutputFormat=lcov - - name: Upload coverage report - if: ${{ matrix.os == 'ubuntu-22.04' && matrix.dotnet == '7.0.x' && github.actor != 'dependabot[bot]' }} - uses: paambaati/codeclimate-action@v3.0.0 - env: - CC_TEST_REPORTER_ID: ${{ secrets.CODE_CLIMATE_KEY }} - with: - coverageLocations: | - ${{github.workspace}}/APIMatic.Core.Test/coverage.info:lcov + # - name: Upload coverage report + # if: ${{ matrix.os == 'ubuntu-22.04' && matrix.dotnet == '7.0.x' && github.actor != 'dependabot[bot]' }} + # uses: paambaati/codeclimate-action@v3.0.0 + # env: + # CC_TEST_REPORTER_ID: ${{ secrets.CODE_CLIMATE_KEY }} + # with: + # coverageLocations: | + # ${{github.workspace}}/APIMatic.Core.Test/coverage.info:lcov From 2a5d70afe35486320a36eb9f5cea87da758b6675 Mon Sep 17 00:00:00 2001 From: Hamza Mahmood Date: Tue, 22 Jul 2025 12:06:32 +0500 Subject: [PATCH 4/5] add unit test for null parameter key handling --- APIMatic.Core.Test/Request/ParameterTests.cs | 32 +++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/APIMatic.Core.Test/Request/ParameterTests.cs b/APIMatic.Core.Test/Request/ParameterTests.cs index e62b7267..a88ba78c 100644 --- a/APIMatic.Core.Test/Request/ParameterTests.cs +++ b/APIMatic.Core.Test/Request/ParameterTests.cs @@ -1,4 +1,6 @@ -using APIMatic.Core.Request; +using System.Collections.Generic; +using System.Linq; +using APIMatic.Core.Request; using APIMatic.Core.Request.Parameters; using NUnit.Framework; @@ -48,5 +50,33 @@ public void DuplicateQueryKeys_Should_UseLastOneInRequestBuilder() Assert.That(requestBuilder.queryParameters.TryGetValue("status", out var value), Is.True); Assert.That(value, Is.EqualTo("approved")); } + + [Test] + public void AdditionalForms_WhenKeyIsNull_ShouldAssignInnerFieldsCorrectly() + { + // Arrange + var parameters = new Parameter.Builder(); + var fieldParameters = new Dictionary + { + ["inner_field"] = "inner_field", + }; + + parameters + .AdditionalForms(additionalForms => additionalForms.Setup(fieldParameters)); + + var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); + + // Act + parameters.Validate().Apply(requestBuilder); + + // Assert + var formParam = requestBuilder.formParameters.FirstOrDefault(p => p.Key == "inner_field"); + + Assert.Multiple(() => + { + Assert.That(formParam, Is.Not.Null, "Expected form parameter with key 'inner_field'"); + Assert.That(formParam.Value, Is.EqualTo("inner_field"), "Expected value mismatch for 'inner_field'"); + }); + } } } From 4d246637c9148b28bec8ab38ef197b0be0dcc985 Mon Sep 17 00:00:00 2001 From: Hamza Mahmood Date: Tue, 22 Jul 2025 15:04:06 +0500 Subject: [PATCH 5/5] fix(parameter-builder): enforce parameter ordering to respect precedence for duplicate keys in additional form, query, and header parameters --- APIMatic.Core.Test/Request/ParameterTests.cs | 131 ++++++++++++++++-- APIMatic.Core/Request/Parameters/Parameter.cs | 38 +++-- 2 files changed, 145 insertions(+), 24 deletions(-) diff --git a/APIMatic.Core.Test/Request/ParameterTests.cs b/APIMatic.Core.Test/Request/ParameterTests.cs index a88ba78c..17375118 100644 --- a/APIMatic.Core.Test/Request/ParameterTests.cs +++ b/APIMatic.Core.Test/Request/ParameterTests.cs @@ -10,7 +10,48 @@ namespace APIMatic.Core.Test.Request public class ParameterTests : TestBase { const string ServerUrl = "https://parameter.api.server.com"; - + + private static readonly Dictionary QueryParams = new Dictionary + { + ["field"] = "query_value", + ["field1"] = "query_value1", + ["field2"] = "query_value2", + ["field3"] = "query_value3", + }; + + private static readonly Dictionary AdditionalQueryParams = new Dictionary + { + ["field"] = "additional_query_value", + ["field1"] = "additional_query_value1", + ["field2"] = "additional_query_value2", + ["field3"] = "additional_query_value3", + }; + + [Test] + public void AdditionalForms_WhenKeyIsNull_ShouldAssignInnerFieldsCorrectly() + { + // Arrange + var parameters = new Parameter.Builder(); + var fieldParameters = new Dictionary { ["field"] = "field", }; + + parameters + .AdditionalForms(additionalForms => additionalForms.Setup(fieldParameters)); + + var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); + + // Act + parameters.Validate().Apply(requestBuilder); + + // Assert + var formParam = requestBuilder.formParameters.FirstOrDefault(p => p.Key == "field"); + + Assert.Multiple(() => + { + Assert.That(formParam, Is.Not.Null, "Expected form parameter with key 'field'"); + Assert.That(formParam.Value, Is.EqualTo("field"), "Expected value mismatch for 'field'"); + }); + } + [Test] public void DuplicateHeaderKeys_Should_UseLastOneInRequestBuilder() { @@ -18,7 +59,12 @@ public void DuplicateHeaderKeys_Should_UseLastOneInRequestBuilder() var parameters = new Parameter.Builder(); parameters - .Header(h => h.Setup("Authorization", "Basic OLD").Required()) + .Header(h => h.Setup("Authorization", "Basic OLD1").Required()) + .Header(h => h.Setup("Authorization", "Basic OLD2").Required()) + .Header(h => h.Setup("Authorization", "Basic OLD3").Required()) + .Header(h => h.Setup("Authorization", "Basic OLD4").Required()) + .Header(h => h.Setup("Authorization", "Basic OLD5").Required()) + .Header(h => h.Setup("Authorization", "Basic OLD6").Required()) .Header(h => h.Setup("Authorization", "Basic NEW").Required()); var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); @@ -30,7 +76,7 @@ public void DuplicateHeaderKeys_Should_UseLastOneInRequestBuilder() Assert.That(requestBuilder.headersParameters.TryGetValue("Authorization", out var value), Is.True); Assert.That(value, Is.EqualTo("Basic NEW")); } - + [Test] public void DuplicateQueryKeys_Should_UseLastOneInRequestBuilder() { @@ -50,19 +96,17 @@ public void DuplicateQueryKeys_Should_UseLastOneInRequestBuilder() Assert.That(requestBuilder.queryParameters.TryGetValue("status", out var value), Is.True); Assert.That(value, Is.EqualTo("approved")); } - + [Test] - public void AdditionalForms_WhenKeyIsNull_ShouldAssignInnerFieldsCorrectly() + public void DuplicateFormKey_WhenSetInAdditionalFormsAndForm_ShouldRetainBothWithSameKey() { // Arrange var parameters = new Parameter.Builder(); - var fieldParameters = new Dictionary - { - ["inner_field"] = "inner_field", - }; - + var fieldParameters = new Dictionary { ["field"] = "additional_form_value", }; + parameters - .AdditionalForms(additionalForms => additionalForms.Setup(fieldParameters)); + .AdditionalForms(additionalForms => additionalForms.Setup(fieldParameters)) + .Form(f => f.Setup("field", "form_value")); var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); @@ -70,12 +114,71 @@ public void AdditionalForms_WhenKeyIsNull_ShouldAssignInnerFieldsCorrectly() parameters.Validate().Apply(requestBuilder); // Assert - var formParam = requestBuilder.formParameters.FirstOrDefault(p => p.Key == "inner_field"); + var formParams = requestBuilder.formParameters + .Where(p => p.Key == "field") + .ToList(); Assert.Multiple(() => { - Assert.That(formParam, Is.Not.Null, "Expected form parameter with key 'inner_field'"); - Assert.That(formParam.Value, Is.EqualTo("inner_field"), "Expected value mismatch for 'inner_field'"); + Assert.That(formParams.Count, Is.EqualTo(2), "Expected 2 form parameters with the key 'field'"); + Assert.That(formParams.Any(p => Equals(p.Value, "form_value")), Is.True, + "Expected 'form_value' to be present"); + Assert.That(formParams.Any(p => Equals(p.Value, "additional_form_value")), Is.True, + "Expected 'additional_form_value' to be present"); + }); + } + + [Test] + public void When_QueryAddedAfterAdditionalQueries_ShouldRetainQueryValues() + { + // Arrange + var parameters = new Parameter.Builder(); + parameters.AdditionalQueries(q => q.Setup(AdditionalQueryParams)); + foreach (var (key, value) in QueryParams) + parameters.Query(q => q.Setup(key, value)); + + var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); + + // Act + parameters.Validate().Apply(requestBuilder); + + // Assert + Assert.Multiple(() => + { + foreach (var (key, expectedValue) in QueryParams) + { + Assert.That(requestBuilder.queryParameters.TryGetValue(key, out var actualValue), Is.True, + $"Expected query parameter with key '{key}'"); + Assert.That(actualValue, Is.EqualTo(expectedValue), + $"Expected value for key '{key}' to be '{expectedValue}', but was '{actualValue}'"); + } + }); + } + + [Test] + public void When_AdditionalQueriesAddedAfterQuery_ShouldRetainAdditionalQueryValues() + { + // Arrange + var parameters = new Parameter.Builder(); + foreach (var (key, value) in QueryParams) + parameters.Query(q => q.Setup(key, value)); + parameters.AdditionalQueries(q => q.Setup(AdditionalQueryParams)); + + var requestBuilder = new RequestBuilder(LazyGlobalConfiguration.Value, ServerUrl); + + // Act + parameters.Validate().Apply(requestBuilder); + + // Assert + Assert.Multiple(() => + { + foreach (var (key, expectedValue) in AdditionalQueryParams) + { + Assert.That(requestBuilder.queryParameters.TryGetValue(key, out var actualValue), Is.True, + $"Expected query parameter with key '{key}'"); + Assert.That(actualValue, Is.EqualTo(expectedValue), + $"Expected value for key '{key}' to be '{expectedValue}', but was '{actualValue}'"); + } }); } } diff --git a/APIMatic.Core/Request/Parameters/Parameter.cs b/APIMatic.Core/Request/Parameters/Parameter.cs index 456a6557..863b6a0c 100644 --- a/APIMatic.Core/Request/Parameters/Parameter.cs +++ b/APIMatic.Core/Request/Parameters/Parameter.cs @@ -81,6 +81,8 @@ public class Builder { private readonly ConcurrentDictionary _parameters = new ConcurrentDictionary(); + private readonly ConcurrentQueue _insertionOrder = new ConcurrentQueue(); + private readonly object _sync = new object(); internal Builder() { } @@ -93,7 +95,7 @@ public Builder Template(Action _template) { var template = new TemplateParam(); _template(template); - _parameters[template.IdentifierKey] = template; + AddOrUpdate(template); return this; } @@ -106,7 +108,7 @@ public Builder Header(Action _header) { var header = new HeaderParam(); _header(header); - _parameters[header.IdentifierKey] = header; + AddOrUpdate(header); return this; } @@ -120,7 +122,7 @@ public Builder AdditionalHeaders(Action _headers) { var headers = new AdditionalHeaderParams(); _headers(headers); - _parameters[headers.IdentifierKey] = headers; + AddOrUpdate(headers); return this; } @@ -133,7 +135,7 @@ public Builder Query(Action _query) { var query = new QueryParam(); _query(query); - _parameters[query.IdentifierKey] = query; + AddOrUpdate(query); return this; } @@ -146,7 +148,7 @@ public Builder AdditionalQueries(Action _queries) { var queries = new AdditionalQueryParams(); _queries(queries); - _parameters[queries.IdentifierKey] = queries; + AddOrUpdate(queries); return this; } @@ -159,7 +161,7 @@ public Builder Form(Action _form) { var form = new FormParam(); _form(form); - _parameters[form.IdentifierKey] = form; + AddOrUpdate(form); return this; } @@ -172,7 +174,7 @@ public Builder AdditionalForms(Action _forms) { var forms = new AdditionalFormParams(); _forms(forms); - _parameters[forms.IdentifierKey] = forms; + AddOrUpdate(forms); return this; } @@ -185,7 +187,7 @@ public Builder Body(Action _body) { var body = new BodyParam(); _body(body); - _parameters[body.IdentifierKey] = body; + AddOrUpdate(body); return this; } @@ -220,9 +222,25 @@ internal Builder Validate() /// internal void Apply(RequestBuilder requestBuilder) { - foreach (var p in _parameters) + foreach (var p in _insertionOrder) { - p.Value.Apply(requestBuilder); + _parameters[p].Apply(requestBuilder); + } + } + + private void AddOrUpdate(Parameter param) + { + var key = param.IdentifierKey; + + lock (_sync) + { + // Only record order on first insertion + if (!_parameters.ContainsKey(key)) + { + _insertionOrder.Enqueue(key); + } + + _parameters[key] = param; } } }