From ed6e3a3ccc2e41579084a9d4580fbf6597729569 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Tue, 29 Sep 2020 12:19:39 -0700 Subject: [PATCH] Resolve PR feedback --- .../src/Indexes/FieldBuilder.cs | 4 +- .../src/Spatial/SpatialProxyFactory.cs | 48 ++++++++----------- .../tests/FieldBuilderTests.cs | 42 ++++++++++++++++ .../tests/Spatial/SpatialProxyFactoryTests.cs | 32 ++++++++++--- 4 files changed, 90 insertions(+), 36 deletions(-) diff --git a/sdk/search/Azure.Search.Documents/src/Indexes/FieldBuilder.cs b/sdk/search/Azure.Search.Documents/src/Indexes/FieldBuilder.cs index 332d05e8e875f..970831ad59635 100644 --- a/sdk/search/Azure.Search.Documents/src/Indexes/FieldBuilder.cs +++ b/sdk/search/Azure.Search.Documents/src/Indexes/FieldBuilder.cs @@ -226,7 +226,7 @@ static bool IsNullableType(Type type) => { return DataTypeInfo.Simple(searchFieldDataType); } - else if (SpatialProxyFactory.CanCreate(propertyType)) + else if (SpatialProxyFactory.IsSupportedPoint(propertyType)) { return DataTypeInfo.Simple(SearchFieldDataType.GeographyPoint); } @@ -370,7 +370,7 @@ public static bool TryGet(Type type, IMemberNameConverter nameProvider, out Obje !type.IsEnum && !s_unsupportedTypes.Contains(type) && !s_primitiveTypeMap.ContainsKey(type) && - !SpatialProxyFactory.CanCreate(type) && + !SpatialProxyFactory.IsSupportedPoint(type) && !typeof(IEnumerable).IsAssignableFrom(type)) { const BindingFlags bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic; diff --git a/sdk/search/Azure.Search.Documents/src/Spatial/SpatialProxyFactory.cs b/sdk/search/Azure.Search.Documents/src/Spatial/SpatialProxyFactory.cs index 8cab6a9ad641e..d76ebe08311c8 100644 --- a/sdk/search/Azure.Search.Documents/src/Spatial/SpatialProxyFactory.cs +++ b/sdk/search/Azure.Search.Documents/src/Spatial/SpatialProxyFactory.cs @@ -13,15 +13,16 @@ namespace Azure.Search.Documents internal static class SpatialProxyFactory { private const string Name = "Microsoft.Spatial"; + private const string GeographyPointTypeName = "Microsoft.Spatial.GeographyPoint"; // TODO: When GeoJson is built into Azure.Core, change these proxies to adapter for GeoJson types. // https://github.com/Azure/azure-sdk-for-net/issues/13319 private static readonly IReadOnlyDictionary> s_types = - new Dictionary>(StringComparer.OrdinalIgnoreCase) + new Dictionary>(StringComparer.Ordinal) { + [GeographyPointTypeName] = value => new GeographyPointProxy(value), ["Microsoft.Spatial.GeographyLineString"] = value => new GeographyLineStringProxy(value), - ["Microsoft.Spatial.GeographyPoint"] = value => new GeographyPointProxy(value), ["Microsoft.Spatial.GeographyPolygon"] = value => new GeographyPolygonProxy(value), }; @@ -32,7 +33,7 @@ internal static class SpatialProxyFactory /// A value indicating whether can be created by this factory. public static bool CanCreate(Type type) { - if (type is null) + if (type == null) { return false; } @@ -40,28 +41,6 @@ public static bool CanCreate(Type type) return TryGetFactory(type, out _); } - /// - /// Creates a from the given if supported. - /// - /// - /// The value to proxy. - /// A from the given if supported, or null if is null. - /// The type is not supported. - public static GeographyProxy Create(object value) - { - if (value is null) - { - return null; - } - - if (TryCreate(value, out GeographyProxy proxy)) - { - return proxy; - } - - throw new NotSupportedException($"Type {value.GetType()} is not supported"); - } - /// /// Attempts to creates a from the given if supported. /// @@ -69,7 +48,7 @@ public static GeographyProxy Create(object value) /// The value to proxy. /// The proxied value if supported. /// True if the could be proxied; otherwise, false. - internal static bool TryCreate(object value, out GeographyProxy proxy) + public static bool TryCreate(object value, out GeographyProxy proxy) { if (value is { }) { @@ -85,10 +64,23 @@ internal static bool TryCreate(object value, out GeographyProxy proxy) return false; } - private static bool TryGetFactory(Type type, out Func factory) + /// + /// Gets a value indicating whether the given represents a supported spatial point. + /// + /// The type to check. + /// A value indicating whether the given represents a supported spatial point. + public static bool IsSupportedPoint(Type type) => + type != null && IsSupportedAssembly(type) && GeographyPointTypeName.Equals(type.FullName, StringComparison.Ordinal); + + private static bool IsSupportedAssembly(Type type) { AssemblyName assemblyName = type.Assembly.GetName(); - if (string.Equals(assemblyName.Name, Name, StringComparison.OrdinalIgnoreCase)) + return string.Equals(assemblyName.Name, Name, StringComparison.OrdinalIgnoreCase); + } + + private static bool TryGetFactory(Type type, out Func factory) + { + if (IsSupportedAssembly(type)) { while (type != typeof(object)) { diff --git a/sdk/search/Azure.Search.Documents/tests/FieldBuilderTests.cs b/sdk/search/Azure.Search.Documents/tests/FieldBuilderTests.cs index 495b0d3b5c16b..2b0b1ea942b4b 100644 --- a/sdk/search/Azure.Search.Documents/tests/FieldBuilderTests.cs +++ b/sdk/search/Azure.Search.Documents/tests/FieldBuilderTests.cs @@ -7,6 +7,7 @@ using System.Text.Json.Serialization; using Azure.Search.Documents.Indexes; using Azure.Search.Documents.Indexes.Models; +using Microsoft.Spatial; using NUnit.Framework; using KeyFieldAttribute = System.ComponentModel.DataAnnotations.KeyAttribute; @@ -447,6 +448,29 @@ public void FieldBuilderFailsWithHelpfulErrorMessageOnUnsupportedTypes(Type mode Assert.AreEqual(expectedErrorMessage, e.Message); } + [Test] + public void SupportsSpecificSpatialTypes() + { + IList fields = new FieldBuilder().Build(typeof(ModelWithSpatialProperties)); + foreach (SearchField field in fields) + { + switch (field.Name) + { + case nameof(ModelWithSpatialProperties.ID): + Assert.AreEqual(SearchFieldDataType.String, field.Type); + break; + + case nameof(ModelWithSpatialProperties.GeographyPoint): + Assert.AreEqual(SearchFieldDataType.GeographyPoint, field.Type); + break; + + default: + Assert.AreEqual(SearchFieldDataType.Complex, field.Type, $"Unexpected type for field '{field.Name}'"); + break; + } + } + } + private static IEnumerable<(Type, SearchFieldDataType, string)> CombineTestData( IEnumerable modelTypes, IEnumerable<(SearchFieldDataType dataType, string fieldName)> testData) => @@ -597,5 +621,23 @@ private class ModelWithIgnoredProperties public InnerModelWithIgnoredProperties[] Inner { get; set; } } + + private class ModelWithSpatialProperties + { + [SimpleField(IsKey = true)] + public string ID { get; set; } + + public GeographyPoint GeographyPoint { get; set; } + + public GeographyLineString GeographyLineString { get; set; } + + public GeographyPolygon GeographyPolygon { get; set; } + + public GeometryPoint GeometryPoint { get; set; } + + public GeometryLineString GeometryLineString { get; set; } + + public GeometryPolygon GeometryPolygon { get; set; } + } } } diff --git a/sdk/search/Azure.Search.Documents/tests/Spatial/SpatialProxyFactoryTests.cs b/sdk/search/Azure.Search.Documents/tests/Spatial/SpatialProxyFactoryTests.cs index 798c4b423a54a..935c06f52bdc0 100644 --- a/sdk/search/Azure.Search.Documents/tests/Spatial/SpatialProxyFactoryTests.cs +++ b/sdk/search/Azure.Search.Documents/tests/Spatial/SpatialProxyFactoryTests.cs @@ -25,8 +25,11 @@ public void CanCreate(Type type, bool expected) => Assert.AreEqual(expected, SpatialProxyFactory.CanCreate(type)); [Test] - public void CreateNull() => - Assert.IsNull(SpatialProxyFactory.Create(null)); + public void CreateNull() + { + Assert.IsFalse(SpatialProxyFactory.TryCreate(null, out GeographyProxy proxy)); + Assert.IsNull(proxy); + } [Test] public void CreateGeographyPoint() @@ -89,11 +92,28 @@ public void CreateGeographyLineString() } } - [TestCaseSource(nameof(CreateThrowsData))] - public void CreateThrows(object value) => - Assert.Throws(() => SpatialProxyFactory.Create(value)); + [TestCaseSource(nameof(CreateFailsData))] + public void CreateFails(object value) + { + Assert.IsFalse(SpatialProxyFactory.TryCreate(value, out GeographyProxy proxy)); + Assert.IsNull(proxy); + } + + [TestCase(null, false)] + [TestCase(typeof(object), false)] + [TestCase(typeof(int), false)] + [TestCase(typeof(GeographyPoint), true)] + [TestCase(typeof(GeometryPoint), false)] + [TestCase(typeof(GeographyPosition), false)] + [TestCase(typeof(GeometryPosition), false)] + [TestCase(typeof(GeographyPolygon), false)] + [TestCase(typeof(GeometryPolygon), false)] + [TestCase(typeof(GeographyLineString), false)] + [TestCase(typeof(GeometryLineString), false)] + public void IsSupportedPoint(Type type, bool expected) => + Assert.AreEqual(expected, SpatialProxyFactory.IsSupportedPoint(type)); - private static IEnumerable CreateThrowsData => new[] + private static IEnumerable CreateFailsData => new[] { new TestCaseData(new object()), new TestCaseData(1),