Skip to content

Commit 0c22634

Browse files
KrishanthaudayakumaraKrishanthaudayakumara
authored
Merge pull request #14 from Krishanthaudayakumara/feat/srcset-support-pr-reviews
Refactor ImageTagHelper Parameter Extraction and Remove JSON SrcSet Support - Code changes of the PR Review - Refactored AddParametersToResult to use RouteValueDictionary for extracting object properties, improving consistency and maintainability. - Updated ParseUrlParams to use QueryHelpers.ParseQuery for extracting query parameters from URLs, ensuring more robust and reliable parsing. - Refactored GetWidthDescriptor to use TryGetValue instead of ContainsKey for CA1854 compliance, improving performance and code clarity. - Removed JSON string support for SrcSet in ImageTagHelper and updated related tests to reflect this change.
2 parents 104dea2 + 6161068 commit 0c22634

3 files changed

Lines changed: 31 additions & 105 deletions

File tree

‎src/Sitecore.AspNetCore.SDK.RenderingEngine/Extensions/SitecoreFieldExtensions.cs‎

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,12 @@ private static void AddParametersToResult(Dictionary<string, object?> result, ob
9797
}
9898
else
9999
{
100-
PropertyInfo[] properties = parameters.GetType().GetProperties();
101-
foreach (PropertyInfo prop in properties)
100+
RouteValueDictionary routeValues = new RouteValueDictionary(parameters);
101+
foreach (KeyValuePair<string, object?> kvp in routeValues)
102102
{
103-
object? value = prop.GetValue(parameters);
104-
if (!skipNullValues || value != null)
103+
if (!skipNullValues || kvp.Value != null)
105104
{
106-
result[prop.Name] = value;
105+
result[kvp.Key] = kvp.Value;
107106
}
108107
}
109108
}
@@ -177,28 +176,27 @@ private static string GetSitecoreMediaUriWithPreservation(string urlStr, object?
177176
/// <returns>The URL without query parameters.</returns>
178177
private static string ParseUrlParams(string url, Dictionary<string, object?> parameters)
179178
{
180-
if (url.Contains('?'))
179+
if (string.IsNullOrEmpty(url))
181180
{
182-
string[] parts = url.Split('?', 2);
183-
string cleanUrl = parts[0];
184-
string queryString = parts[1];
181+
return url;
182+
}
185183

186-
string[] paramPairs = queryString.Split('&');
187-
foreach (string paramPair in paramPairs)
188-
{
189-
string[] keyValue = paramPair.Split('=', 2);
190-
if (keyValue.Length == 2)
191-
{
192-
string key = HttpUtility.UrlDecode(keyValue[0]);
193-
string value = HttpUtility.UrlDecode(keyValue[1]);
194-
parameters[key] = value;
195-
}
196-
}
184+
int queryIndex = url.IndexOf('?');
185+
if (queryIndex < 0)
186+
{
187+
return url;
188+
}
197189

198-
return cleanUrl;
190+
string cleanUrl = url.Substring(0, queryIndex);
191+
string queryString = url.Substring(queryIndex);
192+
193+
Dictionary<string, Microsoft.Extensions.Primitives.StringValues> parsedQuery = QueryHelpers.ParseQuery(queryString);
194+
foreach (KeyValuePair<string, Microsoft.Extensions.Primitives.StringValues> kvp in parsedQuery)
195+
{
196+
parameters[kvp.Key] = kvp.Value.Count > 0 ? kvp.Value[0] : null;
199197
}
200198

201-
return url;
199+
return cleanUrl;
202200
}
203201

204202
/// <summary>

‎src/Sitecore.AspNetCore.SDK.RenderingEngine/TagHelpers/Fields/ImageTagHelper.cs‎

Lines changed: 11 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public class ImageTagHelper(IEditableChromeRenderer chromeRenderer) : TagHelper
6060

6161
/// <summary>
6262
/// Gets or sets the srcset configurations for responsive images.
63-
/// Supports: object[] (anonymous objects), Dictionary arrays, or JSON string.
63+
/// Supports: object[] (anonymous objects) or Dictionary arrays.
6464
/// Each item should contain width parameters like { mw = 300 }, { w = 100 }.
6565
/// </summary>
6666
public object? SrcSet { get; set; }
@@ -191,21 +191,21 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
191191
if (parameters is Dictionary<string, object> dictionary)
192192
{
193193
// Priority: w > mw > width > maxWidth (matching Content SDK behavior + legacy support)
194-
if (dictionary.ContainsKey("w"))
194+
if (dictionary.TryGetValue("w", out var wValue))
195195
{
196-
width = dictionary["w"]?.ToString();
196+
width = wValue.ToString();
197197
}
198-
else if (dictionary.ContainsKey("mw"))
198+
else if (dictionary.TryGetValue("mw", out var mwValue))
199199
{
200-
width = dictionary["mw"]?.ToString();
200+
width = mwValue.ToString();
201201
}
202-
else if (dictionary.ContainsKey("width"))
202+
else if (dictionary.TryGetValue("width", out var widthValue))
203203
{
204-
width = dictionary["width"]?.ToString();
204+
width = widthValue.ToString();
205205
}
206-
else if (dictionary.ContainsKey("maxWidth"))
206+
else if (dictionary.TryGetValue("maxWidth", out var maxWidthValue))
207207
{
208-
width = dictionary["maxWidth"]?.ToString();
208+
width = maxWidthValue.ToString();
209209
}
210210
}
211211
else
@@ -219,33 +219,14 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
219219
?? TryParseParameter(parameters, "maxWidth", properties);
220220
}
221221

222-
if (width != null && int.TryParse(width, out int widthValue) && widthValue <= 0)
222+
if (width != null && int.TryParse(width, out int widthValueInt) && widthValueInt <= 0)
223223
{
224224
return null;
225225
}
226226

227227
return width != null ? $"{width}w" : null;
228228
}
229229

230-
private static object[]? ParseJsonSrcSet(object srcSetValue)
231-
{
232-
if (srcSetValue is string jsonString)
233-
{
234-
try
235-
{
236-
// We need to use Dictionary<string, object>[] to ensure proper deserialization of JSON objects into dictionaries that our GetWidthDescriptor method can handle
237-
return JsonSerializer.Deserialize<Dictionary<string, object>[]>(jsonString, JsonLayoutServiceSerializer.GetDefaultSerializerOptions());
238-
}
239-
catch (Exception ex)
240-
{
241-
// JSON parsing failed - this is a programming error, invalid JSON was passed
242-
throw new InvalidOperationException($"Failed to parse srcset JSON: {jsonString}", ex);
243-
}
244-
}
245-
246-
return [srcSetValue];
247-
}
248-
249230
private TagBuilder GenerateImage(ImageField imageField, TagHelperOutput output)
250231
{
251232
Image image = imageField.Value;
@@ -375,18 +356,7 @@ private string GenerateSrcSetAttribute(ImageField imageField)
375356
return string.Empty;
376357
}
377358

378-
object[]? parsedSrcSet;
379-
380-
if (SrcSet is object[] objectArray)
381-
{
382-
parsedSrcSet = objectArray;
383-
}
384-
else
385-
{
386-
parsedSrcSet = ParseJsonSrcSet(SrcSet);
387-
}
388-
389-
if (parsedSrcSet == null || parsedSrcSet.Length == 0)
359+
if (SrcSet is not object[] parsedSrcSet || parsedSrcSet.Length == 0)
390360
{
391361
return string.Empty;
392362
}

‎tests/Sitecore.AspNetCore.SDK.RenderingEngine.Tests/TagHelpers/Fields/ImageTagHelperFixture.cs‎

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -861,29 +861,6 @@ public void Process_EditableImageWithSrcSet_MergesSrcSetIntoEditableMarkup(
861861
sizesValue.Should().Be("(min-width: 768px) 600px, 300px");
862862
}
863863

864-
[Theory]
865-
[AutoNSubstituteData]
866-
public void Process_SrcSetWithJsonString_GeneratesSrcSetAttribute(
867-
ImageTagHelper sut,
868-
TagHelperContext tagHelperContext,
869-
TagHelperOutput tagHelperOutput)
870-
{
871-
// Arrange
872-
tagHelperOutput.TagName = "img";
873-
sut.For = GetModelExpression(new ImageField(_image));
874-
sut.SrcSet = "[{\"mw\": 500}, {\"mw\": 250}]";
875-
876-
// Act
877-
sut.Process(tagHelperContext, tagHelperOutput);
878-
879-
// Assert
880-
tagHelperOutput.Attributes.Should().ContainSingle(a => a.Name == "srcset");
881-
string srcSetValue = tagHelperOutput.Attributes["srcset"].Value.ToString()!;
882-
883-
// Full string comparison
884-
srcSetValue.Should().Be("http://styleguide/-/jssmedia/styleguide/data/media/img/sc_logo.png?iar=0&hash=F313AD90AE547CAB09277E42509E289B&mw=500 500w, http://styleguide/-/jssmedia/styleguide/data/media/img/sc_logo.png?iar=0&hash=F313AD90AE547CAB09277E42509E289B&mw=250 250w");
885-
}
886-
887864
[Theory]
888865
[AutoNSubstituteData]
889866
public void Process_SrcSetWithMixedParameterTypes_GeneratesSrcSetAttribute(
@@ -915,25 +892,6 @@ public void Process_SrcSetWithMixedParameterTypes_GeneratesSrcSetAttribute(
915892
srcsetValue.Should().Be("http://styleguide/-/jssmedia/styleguide/data/media/img/sc_logo.png?iar=0&amp;hash=F313AD90AE547CAB09277E42509E289B&amp;w=800 800w, http://styleguide/-/jssmedia/styleguide/data/media/img/sc_logo.png?iar=0&amp;hash=F313AD90AE547CAB09277E42509E289B&amp;mw=400 400w");
916893
}
917894

918-
[Theory]
919-
[AutoNSubstituteData]
920-
public void Process_SrcSetWithInvalidJsonString_ThrowsInvalidOperationException(
921-
ImageTagHelper sut,
922-
TagHelperContext tagHelperContext,
923-
TagHelperOutput tagHelperOutput)
924-
{
925-
// Arrange
926-
tagHelperOutput.TagName = "img";
927-
sut.For = GetModelExpression(new ImageField(_image));
928-
sut.SrcSet = "invalid json string";
929-
930-
// Act & Assert
931-
Action act = () => sut.Process(tagHelperContext, tagHelperOutput);
932-
act.Should().Throw<InvalidOperationException>()
933-
.WithMessage("Failed to parse srcset JSON: invalid json string*")
934-
.WithInnerException<System.Text.Json.JsonException>();
935-
}
936-
937895
[Theory]
938896
[AutoNSubstituteData]
939897
public void Process_SrcSetWithImageParamsConflict_SrcSetParametersArePreferred(

0 commit comments

Comments
 (0)