Fix resolvable dictionary properties (#7076) (#7078)

* Prefer IReadOnlyDictionary on read only (response) types

* Introduce readonly dictionary converters for Field and IndexName

These ensure keys are santised during lookups so that items can be found correctly using inferred values.

* Prefer IDictionary for properties on writable types

* Update tests

* Fix BOM

* Cleanup IUrlParameter types for consistency

Co-authored-by: Steve Gordon <sgordon@hotmail.co.uk>

Author: github-actions[bot]

src/Elastic.Clients.Elasticsearch/Api/IndexManagement/GetFieldMappingResponse.cs

@@ -2,13 +2,91 @@
 // Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
 // See the LICENSE file in the project root for more information.
 
+using System;
 using System.Collections.Generic;
+using System.Linq.Expressions;
 using System.Text.Json.Serialization;
+using Elastic.Clients.Elasticsearch.Mapping;
 
 namespace Elastic.Clients.Elasticsearch.IndexManagement;
 
 public partial class GetFieldMappingResponse
 {
 	[JsonIgnore]
 	public IReadOnlyDictionary<IndexName, TypeFieldMappings> FieldMappings => BackingDictionary;
+
+	public IProperty? GetProperty(IndexName index, Field field)
+	{
+		if (index is null)
+			throw new ArgumentNullException(nameof(index));
+
+		if (field is null)
+			throw new ArgumentNullException(nameof(field));
+
+		var mappings = MappingsFor(index);
+
+		if (mappings is null)
+			return null;
+
+		if (!mappings.TryGetValue(field, out var fieldMapping) || fieldMapping.Mapping is null)
+			return null;
+
+		return fieldMapping.Mapping.TryGetProperty(PropertyName.FromField(field), out var property) ? property : null;
+	}
+
+	public bool TryGetProperty(IndexName index, Field field, out IProperty property)
+	{
+		property = null;
+
+		if (index is null)
+			throw new ArgumentNullException(nameof(index));
+
+		if (field is null)
+			throw new ArgumentNullException(nameof(field));
+
+		var mappings = MappingsFor(index);
+
+		if (mappings is null)
+			return false;
+
+		if (!mappings.TryGetValue(field, out var fieldMapping) || fieldMapping.Mapping is null)
+			return false;
+
+		if (fieldMapping.Mapping.TryGetProperty(PropertyName.FromField(field), out var matched))
+		{
+			property = matched;
+			return true;
+		}
+
+		return false;
+	}
+
+	public IProperty? PropertyFor<T>(Field field) => PropertyFor<T>(field, null);
+
+	public IProperty? PropertyFor<T>(Field field, IndexName index) =>
+		GetProperty(index ?? Infer.Index<T>(), field);
+
+	public IProperty? PropertyFor<T, TValue>(Expression<Func<T, TValue>> objectPath)
+		where T : class =>
+			GetProperty(Infer.Index<T>(), Infer.Field(objectPath));
+
+	public IProperty? PropertyFor<T, TValue>(Expression<Func<T, TValue>> objectPath, IndexName index)
+		where T : class =>
+			GetProperty(index, Infer.Field(objectPath));
+
+	public IProperty? PropertyFor<T>(Expression<Func<T, object>> objectPath)
+		where T : class =>
+			GetProperty(Infer.Index<T>(), Infer.Field(objectPath));
+
+	public IProperty? PropertyFor<T>(Expression<Func<T, object>> objectPath, IndexName index)
+		where T : class =>
+			GetProperty(index, Infer.Field(objectPath));
+
+	private IReadOnlyDictionary<Field, FieldMapping> MappingsFor(IndexName index)
+	{
+		if (!FieldMappings.TryGetValue(index, out var indexMapping) || indexMapping.Mappings == null)
+			return null;
+
+		return indexMapping.Mappings;
+	}
 }

src/Elastic.Clients.Elasticsearch/Core/Infer/IndexName/IndexName.cs

@@ -45,7 +45,7 @@ private IndexName(string index, Type type, string cluster = null)
 
 	bool IEquatable<IndexName>.Equals(IndexName other) => EqualsMarker(other);
 
-	public string GetString(ITransportConfiguration settings)
+	string IUrlParameter.GetString(ITransportConfiguration settings)
 	{
 		if (settings is not IElasticsearchClientSettings elasticsearchClientSettings)
 			throw new Exception("Tried to pass index name on querystring but it could not be resolved because no Elastic.Clients.Elasticsearch settings are available.");

src/Elastic.Clients.Elasticsearch/Core/Infer/Metric/Metrics.cs

@@ -7,7 +7,7 @@
 
 namespace Elastic.Clients.Elasticsearch;
 
-public class Metrics : IEquatable<Metrics>, IUrlParameter
+public sealed class Metrics : IEquatable<Metrics>, IUrlParameter
 {
 	// TODO: Complete this
 
@@ -27,7 +27,7 @@ public class Metrics : IEquatable<Metrics>, IUrlParameter
 
 	public bool Equals(Metrics other) => Value.Equals(other.Value);
 
-	public string GetString(ITransportConfiguration settings) => string.Empty; // TODO Value.GetStringValue();
+	string IUrlParameter.GetString(ITransportConfiguration settings) => string.Empty; // TODO Value.GetStringValue();
 
 	//public static implicit operator Metrics(IndicesStatsMetric metric) => new Metrics(metric);
 

src/Elastic.Clients.Elasticsearch/Core/Infer/PropertyName/PropertyName.cs

@@ -39,7 +39,7 @@ public PropertyName(PropertyInfo property)
 		_comparisonValue = property;
 		_type = property.DeclaringType;
 	}
-
+	
 	public bool CacheableExpression { get; }
 	public Expression Expression { get; }
 
@@ -78,6 +78,20 @@ public static implicit operator PropertyName(Expression expression) =>
 	public static implicit operator PropertyName(PropertyInfo property) =>
 		property == null ? null : new PropertyName(property);
 
+	internal static PropertyName FromField(Field field)
+	{
+		if (field.Property is not null)
+			return new PropertyName(field.Property);
+
+		if (field.Expression is not null)
+			return new PropertyName(field.Expression);
+
+		if (field.Name is not null)
+			return new PropertyName(field.Name);
+
+		return null;
+	}
+
 	public override int GetHashCode()
 	{
 		unchecked

src/Elastic.Clients.Elasticsearch/Core/Infer/Timestamp/Timestamp.cs

@@ -16,8 +16,11 @@ public sealed class Timestamp : IUrlParameter, IEquatable<Timestamp>
 
 	public bool Equals(Timestamp other) => Value == other.Value;
 
-	// ReSharper disable once ImpureMethodCallOnReadonlyValueField
-	public string GetString(ITransportConfiguration settings) => Value.ToString(CultureInfo.InvariantCulture);
+	string IUrlParameter.GetString(ITransportConfiguration settings) => GetString();
+
+	public override string ToString() => GetString();
+
+	private string GetString() => Value.ToString(CultureInfo.InvariantCulture);
 
 	public static implicit operator Timestamp(DateTimeOffset categoryId) => new(categoryId.ToUnixTimeMilliseconds());
 

src/Elastic.Clients.Elasticsearch/Core/ReadOnlyFieldDictionary.cs

@@ -0,0 +1,56 @@
+// Licensed to Elasticsearch B.V under one or more agreements.
+// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
+// See the LICENSE file in the project root for more information.
+
+using System.Collections;
+using System.Collections.Generic;
+using Elastic.Transport;
+
+namespace Elastic.Clients.Elasticsearch;
+
+/// <summary>
+/// A specialised readonly dictionary for <typeparamref name="TValue"/> data, keyed by <see cref="IndexName"/>.
+/// <para>This supports inferrence enabled lookups by ensuring keys are sanitized when storing the values and when performing lookups.</para>
+/// </summary>
+/// <typeparam name="TValue"></typeparam>
+internal readonly struct ReadOnlyFieldDictionary<TValue> : IReadOnlyDictionary<Field, TValue>
+{
+	private readonly Dictionary<Field, TValue> _backingDictionary;
+	private readonly IElasticsearchClientSettings? _settings;
+
+	public ReadOnlyFieldDictionary()
+	{
+		_backingDictionary = new Dictionary<Field, TValue>(0);
+		_settings = null;
+	}
+
+	internal ReadOnlyFieldDictionary(Dictionary<Field, TValue> source, IElasticsearchClientSettings settings)
+	{
+		_settings = settings;
+
+		// This is an "optimised version" which doesn't cause a second dictionary to be allocated.
+		// Since we expect this to be used only for deserialisation, the keys received will already have been strings,
+		// so no further sanitisation is required.
+
+		if (source == null)
+		{
+			_backingDictionary = new Dictionary<Field, TValue>(0);
+			return;
+		}
+
+		_backingDictionary = source;
+	}
+
+	private string Sanitize(Field key) => _settings is not null ? (key as IUrlParameter)?.GetString(_settings) : string.Empty;
+
+	public TValue this[Field key] => _backingDictionary.TryGetValue(Sanitize(key), out var v) ? v : default;
+	public TValue this[string key] => _backingDictionary.TryGetValue(key, out var v) ? v : default;
+
+	public IEnumerable<Field> Keys => _backingDictionary.Keys;
+	public IEnumerable<TValue> Values => _backingDictionary.Values;
+	public int Count => _backingDictionary.Count;
+	public bool ContainsKey(Field key) => _backingDictionary.ContainsKey(Sanitize(key));
+	public IEnumerator<KeyValuePair<Field, TValue>> GetEnumerator() => _backingDictionary.GetEnumerator();
+	public bool TryGetValue(Field key, out TValue value) => _backingDictionary.TryGetValue(Sanitize(key), out value);
+	IEnumerator IEnumerable.GetEnumerator() => _backingDictionary.GetEnumerator();
+}

src/Elastic.Clients.Elasticsearch/Core/ReadOnlyIndexNameDictionary.cs

@@ -4,6 +4,7 @@
 
 using System.Collections;
 using System.Collections.Generic;
+using Elastic.Transport;
 
 namespace Elastic.Clients.Elasticsearch;
 
@@ -12,7 +13,7 @@ namespace Elastic.Clients.Elasticsearch;
 /// <para>This supports inferrence enabled lookups by ensuring keys are sanitized when storing the values and when performing lookups.</para>
 /// </summary>
 /// <typeparam name="TValue"></typeparam>
-public struct ReadOnlyIndexNameDictionary<TValue> : IReadOnlyDictionary<IndexName, TValue>
+internal readonly struct ReadOnlyIndexNameDictionary<TValue> : IReadOnlyDictionary<IndexName, TValue>
 {
 	private readonly Dictionary<IndexName, TValue> _backingDictionary;
 	private readonly IElasticsearchClientSettings? _settings;
@@ -31,21 +32,16 @@ internal ReadOnlyIndexNameDictionary(Dictionary<IndexName, TValue> source, IElas
 		// Since we expect this to be used only for deserialisation, the keys received will already have been strings,
 		// so no further sanitisation is required.
 
-		//var backingDictionary = new Dictionary<IndexName, TValue>(source.Count);
-
 		if (source == null)
 		{
 			_backingDictionary = new Dictionary<IndexName, TValue>(0);
 			return;
 		}
 
-		//foreach (var key in source.Keys)
-		//	backingDictionary[Sanitize(key)] = source[key];
-
 		_backingDictionary = source;
 	}
 
-	private string Sanitize(IndexName key) => _settings is not null ? key?.GetString(_settings) : string.Empty;
+	private string Sanitize(IndexName key) => _settings is not null ? (key as IUrlParameter)?.GetString(_settings) : string.Empty;
 
 	public TValue this[IndexName key] => _backingDictionary.TryGetValue(Sanitize(key), out var v) ? v : default;
 	public TValue this[string key] => _backingDictionary.TryGetValue(key, out var v) ? v : default;

src/Elastic.Clients.Elasticsearch/Core/UrlParameters/IndexUuid/IndexUuid.cs

@@ -26,7 +26,9 @@ public bool Equals(IndexUuid other)
 		return false;
 	}
 
-	public string GetString(ITransportConfiguration settings) => Value;
+	string IUrlParameter.GetString(ITransportConfiguration settings) => Value;
+
+	public override string ToString() => Value;
 
 	public override bool Equals(object obj)
 	{

src/Elastic.Clients.Elasticsearch/Core/UrlParameters/TaskId/TaskId.cs

@@ -5,7 +5,6 @@
 using System;
 using System.Diagnostics;
 using System.Globalization;
-using System.Runtime;
 using System.Text.Json;
 using System.Text.Json.Serialization;
 using Elastic.Clients.Elasticsearch.Serialization;
@@ -47,7 +46,7 @@ public TaskId(string taskId)
 
 	public bool Equals(TaskId other) => EqualsString(other?.FullyQualifiedId);
 
-	public string GetString(ITransportConfiguration settings) => FullyQualifiedId;
+	string IUrlParameter.GetString(ITransportConfiguration settings) => FullyQualifiedId;
 
 	public override string ToString() => FullyQualifiedId;
 
@@ -77,7 +76,7 @@ public override void WriteAsPropertyName(Utf8JsonWriter writer, TaskId value, Js
 	{
 		if (options.TryGetClientSettings(out var settings))
 		{
-			writer.WritePropertyName(value.GetString(settings));
+			writer.WritePropertyName(((IUrlParameter)value).GetString(settings));
 			return;
 		}
 

src/Elastic.Clients.Elasticsearch/Serialization/DefaultRequestResponseSerializer.cs

@@ -29,7 +29,6 @@ public DefaultRequestResponseSerializer(IElasticsearchClientSettings settings)
 				{
 					new KeyValuePairConverterFactory(settings),
 					new SourceConverterFactory(settings),
-					new ReadOnlyIndexNameDictionaryConverterFactory(settings),
 					new CalendarIntervalConverter(),
 					new IndexNameConverter(settings),
 					new ObjectToInferredTypesConverter(),
@@ -47,6 +46,7 @@ public DefaultRequestResponseSerializer(IElasticsearchClientSettings settings)
 					new IndicesJsonConverter(settings),
 					new IdsConverter(settings),
 					new IsADictionaryConverterFactory(),
+					//new ResolvableReadonlyDictionaryConverterFactory(settings),
 					new ResponseItemConverterFactory(),
 					new UnionConverter(),
 					new ExtraSerializationData(settings),