Skip to content

Commit

Permalink
Improve UrlFormatter (#135)
Browse files Browse the repository at this point in the history
I noticed that `UrlFormatter.CreateString()` does not prefer custom
`IUrlParameter.GetString()` implementations when a type as well
implements `IList`/`IEnumerable`.

Besides that, the PR contains the following additional improvements:

- Recursively call `CreateString()` for collection items
- Remove duplicate enum member handling
- Makes sure that `CreateString()` never returns `null` (there is no
value in distinguishing between `null` and `string.Empty` here)
  • Loading branch information
flobernd authored Nov 19, 2024
1 parent af894be commit 4557279
Showing 1 changed file with 20 additions and 38 deletions.
58 changes: 20 additions & 38 deletions src/Elastic.Transport/Requests/UrlFormatter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@

using System;
using System.Collections;
using System.Runtime.Serialization;
using System.Globalization;
using System.Text;

using Elastic.Transport.Extensions;

namespace Elastic.Transport;

/// <summary>
/// A formatter that can utilize <see cref="ITransportConfiguration" /> to resolve <see cref="IUrlParameter" />'s passed
/// as format arguments. It also handles known string representations for e.g bool/Enums/IEnumerable.
/// as format arguments. It also handles known string representations for e.g. bool/Enums/IEnumerable.
/// </summary>
public sealed class UrlFormatter : IFormatProvider, ICustomFormatter
{
Expand All @@ -39,26 +40,32 @@ public string Format(string? format, object? arg, IFormatProvider? formatProvide
public object? GetFormat(Type formatType) => formatType == typeof(ICustomFormatter) ? this : null;

/// <inheritdoc cref="CreateString(object, ITransportConfiguration)"/>
public string? CreateString(object? value) => CreateString(value, _settings);
public string CreateString(object? value) => CreateString(value, _settings);

/// <summary> Creates a query string representation for <paramref name="value"/> </summary>
public static string? CreateString(object? value, ITransportConfiguration settings) =>
public static string CreateString(object? value, ITransportConfiguration settings) =>
value switch
{
null => null,
null => string.Empty,
string s => s,
string[] ss => string.Join(",", ss),
Enum e => e.GetStringValue(),
bool b => b ? "true" : "false",
DateTimeOffset offset => offset.ToString("o"),
TimeSpan timeSpan => timeSpan.ToTimeUnit(),
// Custom `IUrlParameter.GetString()` implementations should take precedence over collection
// specializations.
IUrlParameter urlParam => urlParam.GetString(settings),
// Special handling to support non-zero based arrays
Array pns => CreateStringFromArray(pns, settings),
// Performance optimization for directly indexable collections
IList pns => CreateStringFromIList(pns, settings),
// Generic implementation for all other collections
IEnumerable pns => CreateStringFromIEnumerable(pns, settings),
_ => ResolveUrlParameterOrDefault(value, settings)
// Generic implementation for `IFormattable` types
IFormattable f => f.ToString(null, CultureInfo.InvariantCulture),
// Last resort fallback
_ => value.ToString() ?? string.Empty
};

private static string CreateStringFromArray(Array value, ITransportConfiguration settings)
Expand All @@ -67,8 +74,9 @@ private static string CreateStringFromArray(Array value, ITransportConfiguration
{
case 0:
return string.Empty;

case 1:
return ResolveUrlParameterOrDefault(value.GetValue(value.GetLowerBound(0)), settings);
return CreateString(value.GetValue(value.GetLowerBound(0)), settings);
}

var sb = new StringBuilder();
Expand All @@ -78,7 +86,7 @@ private static string CreateStringFromArray(Array value, ITransportConfiguration
if (sb.Length != 0)
sb.Append(',');

sb.Append(ResolveUrlParameterOrDefault(value.GetValue(i), settings));
sb.Append(CreateString(value.GetValue(i), settings));
}

return sb.ToString();
Expand All @@ -90,8 +98,9 @@ private static string CreateStringFromIList(IList value, ITransportConfiguration
{
case 0:
return string.Empty;

case 1:
return ResolveUrlParameterOrDefault(value[0], settings);
return CreateString(value[0], settings);
}

var sb = new StringBuilder();
Expand All @@ -101,7 +110,7 @@ private static string CreateStringFromIList(IList value, ITransportConfiguration
if (sb.Length != 0)
sb.Append(',');

sb.Append(ResolveUrlParameterOrDefault(value[i], settings));
sb.Append(CreateString(value[i], settings));
}

return sb.ToString();
Expand All @@ -116,36 +125,9 @@ private static string CreateStringFromIEnumerable(IEnumerable value, ITransportC
if (sb.Length != 0)
sb.Append(',');

sb.Append(ResolveUrlParameterOrDefault(v, settings));
sb.Append(CreateString(v, settings));
}

return sb.ToString();
}

private static string ResolveUrlParameterOrDefault(object? value, ITransportConfiguration settings) =>
value switch
{
null => string.Empty,
IUrlParameter urlParam => urlParam.GetString(settings),
_ => GetEnumMemberName(value) ?? value.ToString() ?? string.Empty
};

private static string? GetEnumMemberName(object value)
{
var type = value.GetType();
if (!type.IsEnum)
return null;

var name = Enum.GetName(type, value);
if (name is null)
return null;

var field = type.GetField(name);
if (field is null)
return null;

return Attribute.GetCustomAttribute(field, typeof(EnumMemberAttribute)) is EnumMemberAttribute attribute
? attribute.Value
: null;
}
}

0 comments on commit 4557279

Please sign in to comment.