Skip to content

Commit 949c594

Browse files
niemyjskiniemyjski
committed
Fix provider review regressions in JSON and input handling
Root cause: the review-driven cleanup exposed missing validation and compatibility gaps in provider payload handling, and one CodeQL cleanup accidentally removed null-tolerant filtering for malformed MapQuest responses.
1 parent 945481d commit 949c594

12 files changed

Lines changed: 749 additions & 100 deletions

File tree

‎samples/Example.Web/Program.cs‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ static string[] GetConfiguredProviders(ProviderOptions options)
117117

118118
configuredProviders.Add("google");
119119

120-
if (!String.IsNullOrWhiteSpace(GetHereApiKey(options)))
120+
if (!String.IsNullOrWhiteSpace(options.Here.ApiKey))
121121
configuredProviders.Add("here");
122122

123123
if (!String.IsNullOrWhiteSpace(options.MapQuest.ApiKey))
@@ -169,14 +169,14 @@ static bool TryCreateGeocoder(string provider, ProviderOptions options, out IGeo
169169
return false;
170170
}
171171

172-
if (String.IsNullOrWhiteSpace(GetHereApiKey(options)))
172+
if (String.IsNullOrWhiteSpace(options.Here.ApiKey))
173173
{
174174
geocoder = default!;
175175
error = "Configure Providers:Here:ApiKey before using the HERE provider.";
176176
return false;
177177
}
178178

179-
geocoder = new HereGeocoder(GetHereApiKey(options));
179+
geocoder = new HereGeocoder(options.Here.ApiKey);
180180
error = null;
181181
return true;
182182

@@ -209,11 +209,6 @@ static bool TryCreateGeocoder(string provider, ProviderOptions options, out IGeo
209209
}
210210
}
211211

212-
static string GetHereApiKey(ProviderOptions options)
213-
{
214-
return options.Here.ApiKey;
215-
}
216-
217212
static AddressResponse MapAddress(Address address) =>
218213
new(address.FormattedAddress, address.Provider, address.Coordinates.Latitude, address.Coordinates.Longitude);
219214

‎src/Geocoding.Core/Serialization/TolerantStringEnumConverter.cs‎

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ namespace Geocoding.Serialization;
55

66
/// <summary>
77
/// A <see cref="JsonConverterFactory"/> that deserializes enum values tolerantly,
8-
/// returning the default value (0) when an unrecognized string is encountered.
8+
/// returning an <c>Unknown</c> enum member when one exists, or the default value otherwise.
99
/// This prevents deserialization failures when a geocoding API returns new enum values
10-
/// that the library doesn't yet know about.
10+
/// that the library doesn't yet know about while still preserving nullable-enum behavior.
1111
/// </summary>
1212
internal sealed class TolerantStringEnumConverterFactory : JsonConverterFactory
1313
{
@@ -18,38 +18,77 @@ public override bool CanConvert(Type typeToConvert)
1818

1919
public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
2020
{
21-
var enumType = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert;
22-
var converterType = typeof(TolerantStringEnumConverter<>).MakeGenericType(enumType);
21+
var nullableEnumType = Nullable.GetUnderlyingType(typeToConvert);
22+
var converterType = nullableEnumType is null
23+
? typeof(TolerantStringEnumConverter<>).MakeGenericType(typeToConvert)
24+
: typeof(NullableTolerantStringEnumConverter<>).MakeGenericType(nullableEnumType);
2325
return (JsonConverter)Activator.CreateInstance(converterType);
2426
}
2527
}
2628

2729
internal sealed class TolerantStringEnumConverter<TEnum> : JsonConverter<TEnum> where TEnum : struct, Enum
2830
{
31+
private static readonly TEnum FallbackValue = GetFallbackValue();
32+
2933
public override TEnum Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
3034
{
3135
if (reader.TokenType == JsonTokenType.Number)
3236
{
3337
if (reader.TryGetInt32(out int intValue))
34-
return Enum.IsDefined(typeof(TEnum), intValue) ? (TEnum)(object)intValue : default;
38+
return Enum.IsDefined(typeof(TEnum), intValue) ? (TEnum)(object)intValue : FallbackValue;
3539

36-
return default;
40+
return FallbackValue;
3741
}
3842

3943
if (reader.TokenType == JsonTokenType.String)
4044
{
4145
var value = reader.GetString();
42-
if (Enum.TryParse<TEnum>(value, true, out var result))
46+
if (Enum.TryParse<TEnum>(value, true, out var result) && Enum.IsDefined(typeof(TEnum), result))
4347
return result;
4448

45-
return default;
49+
return FallbackValue;
4650
}
4751

48-
return default;
52+
return FallbackValue;
4953
}
5054

5155
public override void Write(Utf8JsonWriter writer, TEnum value, JsonSerializerOptions options)
5256
{
5357
writer.WriteStringValue(value.ToString());
5458
}
59+
60+
private static TEnum GetFallbackValue()
61+
{
62+
foreach (string name in Enum.GetNames(typeof(TEnum)))
63+
{
64+
if (String.Equals(name, "Unknown", StringComparison.OrdinalIgnoreCase))
65+
return (TEnum)Enum.Parse(typeof(TEnum), name);
66+
}
67+
68+
return default;
69+
}
70+
}
71+
72+
internal sealed class NullableTolerantStringEnumConverter<TEnum> : JsonConverter<TEnum?> where TEnum : struct, Enum
73+
{
74+
private static readonly TolerantStringEnumConverter<TEnum> InnerConverter = new();
75+
76+
public override TEnum? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
77+
{
78+
if (reader.TokenType == JsonTokenType.Null)
79+
return null;
80+
81+
return InnerConverter.Read(ref reader, typeof(TEnum), options);
82+
}
83+
84+
public override void Write(Utf8JsonWriter writer, TEnum? value, JsonSerializerOptions options)
85+
{
86+
if (!value.HasValue)
87+
{
88+
writer.WriteNullValue();
89+
return;
90+
}
91+
92+
InnerConverter.Write(writer, value.Value, options);
93+
}
5594
}

‎src/Geocoding.Here/HereGeocoder.cs‎

Lines changed: 9 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,9 @@ private string BuildQueryString(IEnumerable<KeyValuePair<string, string>> parame
148148
/// <inheritdoc />
149149
public async Task<IEnumerable<HereAddress>> GeocodeAsync(string address, CancellationToken cancellationToken = default(CancellationToken))
150150
{
151+
if (String.IsNullOrWhiteSpace(address))
152+
throw new ArgumentException("address can not be null or empty.", nameof(address));
153+
151154
try
152155
{
153156
var url = GetQueryUrl(address);
@@ -219,20 +222,6 @@ async Task<IEnumerable<Address>> IGeocoder.ReverseGeocodeAsync(double latitude,
219222
return await ReverseGeocodeAsync(latitude, longitude, cancellationToken).ConfigureAwait(false);
220223
}
221224

222-
private bool AppendParameter(StringBuilder sb, string parameter, string format, bool first)
223-
{
224-
if (!String.IsNullOrEmpty(parameter))
225-
{
226-
if (!first)
227-
{
228-
sb.Append('&');
229-
}
230-
sb.Append(String.Format(format, UrlEncode(parameter)));
231-
return false;
232-
}
233-
return first;
234-
}
235-
236225
private IEnumerable<HereAddress> ParseResponse(HereResponse response)
237226
{
238227
if (response.Items is null)
@@ -274,16 +263,14 @@ private HttpClient BuildClient()
274263

275264
private async Task<HereResponse> GetResponse(Uri queryUrl, CancellationToken cancellationToken)
276265
{
277-
using (var client = BuildClient())
278-
using (var response = await client.SendAsync(CreateRequest(queryUrl), cancellationToken).ConfigureAwait(false))
279-
{
280-
var json = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
266+
using var client = BuildClient();
267+
using var response = await client.SendAsync(CreateRequest(queryUrl), cancellationToken).ConfigureAwait(false);
268+
var json = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
281269

282-
if (!response.IsSuccessStatusCode)
283-
throw new HereGeocodingException($"HERE request failed ({(int)response.StatusCode} {response.ReasonPhrase}): {json}", response.ReasonPhrase, ((int)response.StatusCode).ToString(CultureInfo.InvariantCulture));
270+
if (!response.IsSuccessStatusCode)
271+
throw new HereGeocodingException($"HERE request failed ({(int)response.StatusCode} {response.ReasonPhrase}): {json}", response.ReasonPhrase, ((int)response.StatusCode).ToString(CultureInfo.InvariantCulture));
284272

285-
return JsonSerializer.Deserialize<HereResponse>(json, Extensions.JsonOptions) ?? new HereResponse();
286-
}
273+
return JsonSerializer.Deserialize<HereResponse>(json, Extensions.JsonOptions) ?? new HereResponse();
287274
}
288275

289276
private static HereLocationType MapLocationType(string? resultType)

‎src/Geocoding.MapQuest/MapQuestGeocoder.cs‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,8 @@ private IEnumerable<Address> HandleSingleResponse(MapQuestResponse res)
5050
{
5151
if (res is not null && !res.Results.IsNullOrEmpty())
5252
{
53-
return HandleSingleResponse(from r in res.Results
54-
where r is not null && !r.Locations.IsNullOrEmpty()
55-
from l in r.Locations
53+
return HandleSingleResponse(from r in res.Results.OfType<MapQuestResult>()
54+
from l in r.Locations?.OfType<MapQuestLocation>() ?? Enumerable.Empty<MapQuestLocation>()
5655
select l);
5756
}
5857
else
@@ -65,8 +64,8 @@ private IEnumerable<Address> HandleSingleResponse(IEnumerable<MapQuestLocation>
6564
return new Address[0];
6665
else
6766
{
68-
return from l in locs
69-
where l is not null && l.Quality < Quality.COUNTRY
67+
return from l in locs.OfType<MapQuestLocation>()
68+
where l.Quality < Quality.COUNTRY
7069
let q = (int)l.Quality
7170
let c = String.IsNullOrWhiteSpace(l.Confidence) ? "ZZZZZZ" : l.Confidence
7271
orderby q ascending, c ascending
@@ -275,10 +274,10 @@ private ICollection<ResultItem> HandleBatchResponse(MapQuestResponse res)
275274
{
276275
if (res is not null && !res.Results.IsNullOrEmpty())
277276
{
278-
return (from r in res.Results
279-
where r is not null && !r.Locations.IsNullOrEmpty()
280-
let resp = HandleSingleResponse(r.Locations!)
281-
where resp is not null
277+
return (from r in res.Results.OfType<MapQuestResult>()
278+
let locations = r.Locations?.OfType<MapQuestLocation>().ToArray() ?? Array.Empty<MapQuestLocation>()
279+
where locations.Length > 0
280+
let resp = HandleSingleResponse(locations)
282281
select new ResultItem(r.ProvidedLocation!, resp)).ToArray();
283282
}
284283
else

‎src/Geocoding.Microsoft/BingMapsGeocoder.cs‎

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -231,31 +231,45 @@ private bool AppendParameter(StringBuilder sb, string parameter, string format,
231231
return first;
232232
}
233233

234-
private IEnumerable<BingAddress> ParseResponse(Json.Response response)
234+
/// <summary>
235+
/// Parses a Bing Maps response into address results.
236+
/// </summary>
237+
/// <param name="response">The Bing Maps response payload.</param>
238+
/// <returns>The parsed address results.</returns>
239+
protected virtual IEnumerable<BingAddress> ParseResponse(Json.Response response)
235240
{
236241
var list = new List<BingAddress>();
237242

238-
foreach (Json.Location location in response.ResourceSets[0].Resources)
243+
if (response.ResourceSets.IsNullOrEmpty())
244+
return list;
245+
246+
foreach (var resourceSet in response.ResourceSets)
239247
{
240-
if (location.Point is null || location.Address is null)
248+
if (resourceSet is null || resourceSet.Locations.IsNullOrEmpty())
241249
continue;
242250

243-
if (!Enum.TryParse(location.EntityType, out EntityType entityType))
244-
entityType = EntityType.Unknown;
245-
246-
list.Add(new BingAddress(
247-
location.Address.FormattedAddress!,
248-
new Location(location.Point.Coordinates[0], location.Point.Coordinates[1]),
249-
location.Address.AddressLine,
250-
location.Address.AdminDistrict,
251-
location.Address.AdminDistrict2,
252-
location.Address.CountryRegion,
253-
location.Address.Locality,
254-
location.Address.Neighborhood,
255-
location.Address.PostalCode,
256-
entityType,
257-
EvaluateConfidence(location.Confidence)
258-
));
251+
foreach (var location in resourceSet.Locations)
252+
{
253+
if (location.Point is null || location.Address is null || location.Point.Coordinates.Length < 2)
254+
continue;
255+
256+
if (!Enum.TryParse(location.EntityType, out EntityType entityType))
257+
entityType = EntityType.Unknown;
258+
259+
list.Add(new BingAddress(
260+
location.Address.FormattedAddress!,
261+
new Location(location.Point.Coordinates[0], location.Point.Coordinates[1]),
262+
location.Address.AddressLine,
263+
location.Address.AdminDistrict,
264+
location.Address.AdminDistrict2,
265+
location.Address.CountryRegion,
266+
location.Address.Locality,
267+
location.Address.Neighborhood,
268+
location.Address.PostalCode,
269+
entityType,
270+
EvaluateConfidence(location.Confidence)
271+
));
272+
}
259273
}
260274

261275
return list;
@@ -280,7 +294,14 @@ private HttpClient BuildClient()
280294
{
281295
using (var client = BuildClient())
282296
{
283-
var response = await client.SendAsync(CreateRequest(queryUrl), cancellationToken).ConfigureAwait(false);
297+
using var response = await client.SendAsync(CreateRequest(queryUrl), cancellationToken).ConfigureAwait(false);
298+
299+
if (!response.IsSuccessStatusCode)
300+
{
301+
var body = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
302+
throw new Exception($"Bing Maps request failed ({(int)response.StatusCode} {response.ReasonPhrase}): {body}");
303+
}
304+
284305
using (var stream = await response.Content.ReadAsStreamAsync().ConfigureAwait(false))
285306
{
286307
return await JsonSerializer.DeserializeAsync<Json.Response>(stream, Extensions.JsonOptions, cancellationToken).ConfigureAwait(false)

0 commit comments

Comments
 (0)