Skip to content

Commit cd511b1

Browse files
niemyjskiCopilot
andauthored
Modernize providers and test infrastructure (#2)
* Modernize Geocoding.Core: pattern matching, String.* convention, nameof(), TolerantStringEnumConverter - Replace == null/!= null with is null/is not null throughout - Use uppercase String.IsNullOrWhiteSpace/String.Empty convention - Use nameof() for argument validation parameter names - Add TolerantStringEnumConverter for resilient JSON deserialization * Modernize Geocoding.Google: code style, enum completeness, sensor removal - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Add missing GoogleAddressType values (AdminLevel4-7, PlusCode, Landmark, etc.) - Add Route and Locality to GoogleComponentFilterType - Add OverDailyLimit and UnknownError to GoogleStatus - Remove obsolete sensor=false parameter from API URLs * Add Azure Maps provider and modernize Geocoding.Microsoft - Add AzureMapsGeocoder with search/reverse geocoding, culture, bounds bias - Add AzureMapsAddress with EntityType, ConfidenceLevel, neighborhood support - Fix Bing EntityType parsing: use TryParse with Unknown fallback (crash fix) - Add Unknown value to EntityType enum at position 0 - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation * Modernize Geocoding.Here: migrate to API-key auth, v7 endpoint, code style - Migrate from legacy AppId/AppCode to API key authentication - Update to HERE Geocoding v1 (v7) REST endpoints - Replace custom Json.cs with Newtonsoft.Json deserialization - Remove legacy two-parameter constructor (throws NotSupportedException) - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Document HereLocationType legacy v6.2 values in XML remarks * Modernize Geocoding.MapQuest: disable OSM, TolerantStringEnumConverter, code style - Disable OpenStreetMap mode (throws NotSupportedException) - Add TolerantStringEnumConverter for resilient Quality/SideOfStreet parsing - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Fix DataFormat.json value for correct API requests * Modernize Geocoding.Yahoo: code style, deprecation cleanup - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Mark as deprecated but keep functional for contributors with credentials * Modernize test suite: three-part naming, AAA pattern, credential gating - Rename all test methods to Method_Scenario_ExpectedResult format - Add Arrange/Act/Assert comments to multi-phase tests - Add Azure Maps test coverage (AzureMapsAsyncTest) - Add ProviderCompatibilityTest for cross-provider validation - Fix Yahoo tests: remove hardcoded Skip, use credential gating via SettingsFixture - Add yahooConsumerKey/yahooConsumerSecret to settings template - Use uppercase String.* convention in test infrastructure * Update README, samples, and config for provider modernization - Update README with Azure Maps provider, HERE API key migration notes - Mark Yahoo as deprecated in documentation - Update Example.Web sample with Azure Maps and modern patterns - Add docs/plan.md to .gitignore to prevent leaking internal planning docs - Update VS Code settings for workspace * Enable nullable reference types globally Add <Nullable>enable</Nullable> to Directory.Build.props so all projects enforce nullable analysis. Include a NullableAttributes.cs polyfill for NotNullWhenAttribute on netstandard2.0 where System.Diagnostics.CodeAnalysis is not available. * Migrate Geocoding.Core to System.Text.Json and annotate nullable types Replace Newtonsoft.Json with System.Text.Json across the Core library: - Replace JsonConvert with JsonSerializer in Extensions.cs - Extract TolerantStringEnumConverter as a JsonConverterFactory for STJ - Create frozen, shared JsonSerializerOptions with MakeReadOnly() - Add System.Text.Json NuGet dependency - Annotate all public types with nullable reference type annotations - Add [NotNullWhen(false)] to IsNullOrEmpty<T> extension - Add protected parameterless constructors to Address and ParsedAddress for STJ deserialization support * Migrate Google provider to System.Text.Json with nullable annotations Replace Newtonsoft.Json attributes with System.Text.Json equivalents and annotate all types with nullable reference type annotations. * Modernize Microsoft provider: STJ migration, decouple Azure Maps, fix exception handling - Replace DataContractJsonSerializer with System.Text.Json - Remove dead Bing Maps Routing API types (Route, RouteLeg, RoutePath, Shape, Warning, etc.) that were never used by the geocoding library - Type ResourceSet.Resources directly as Location[] instead of Resource[] - Decouple AzureMapsAddress from BingAddress with its own backing fields - Apply 'when' exception filters to prevent double-wrapping exceptions that are already typed as BingGeocodingException - Add null guards in ParseResponse for missing Point/Address data - Annotate all types with nullable reference type annotations - Remove Newtonsoft.Json dependency from csproj * Modernize HERE provider: STJ migration, exception filters, nullable annotations - Replace Newtonsoft.Json with System.Text.Json for API response parsing - Apply 'when' exception filters to prevent double-wrapping HereGeocodingException - Remove Newtonsoft.Json dependency from csproj - Annotate all types with nullable reference type annotations * Modernize MapQuest provider: STJ migration, constructor fix, nullable annotations - Replace Newtonsoft.Json with System.Text.Json across all MapQuest types - Add protected [JsonConstructor] parameterless constructor to MapQuestLocation to resolve STJ parameter binding conflict between FormattedAddress property (mapped to 'location' in JSON) and the constructor parameter - Fix FormattedAddress setter to silently ignore null/blank values during deserialization instead of throwing - Improve ToString() to handle empty base FormattedAddress - Annotate all types with nullable reference type annotations * Mark Yahoo provider as obsolete with nullable annotations Yahoo BOSS Geo Services has been discontinued. Mark all public types with [Obsolete] attributes directing users to alternative providers. Add NoWarn for CS0618 in csproj to suppress internal obsolete usage. Annotate types with nullable reference type annotations. * Modernize test project: AAA comments, nullable fixes, env var config - Add Arrange/Act/Assert section comments to all test methods - Fix nullable warnings with null! for test field initialization - Make theory parameters nullable where InlineData passes null - Add ! operator for reflection-based code in ProviderCompatibilityTest - Modernize SettingsFixture to read API keys from environment variables with GEOCODING_ prefix via AddEnvironmentVariables - Add xunit.v3.assert.source package for xUnit v3 compatibility - Restore regression test comment on MapQuest neighborhood test * Apply review feedback: consistent exception filters, defensive null handling - Align Google provider to use 'when' exception filter pattern consistent with Bing, Azure Maps, and HERE providers - Replace null-forgiving operator with defensive Array.Empty fallback in MapQuest Execute method * Handle disabled Google Geocoding API keys gracefully in tests Google integration failures were caused by the configured Google Cloud project having the Geocoding API disabled, which surfaced as REQUEST_DENIED and burned through the Google test matrix. Preserve Google's provider error message in GoogleGeocodingException, add a one-time cached Google availability guard that skips Google integration tests after the first denied request, and add non-network tests that lock down the parser-to-exception behavior. * Simplify Google exception tests Drop the reflection-based parser test and keep the direct exception coverage. The Google RCA is already proven by the live targeted integration check, so the extra private-method test added complexity without enough value. * Stabilize Google postal code filter coverage With the enabled Google key, the remaining Google failure was not a provider error but a ZERO_RESULTS response for the live Rothwell + NN14 component-filter query. Replace that brittle integration case with a deterministic ServiceUrl test that still verifies postal_code component filter construction without depending on drifting Google geocoder data. * Align test settings with sample provider configuration The test fixture was still reading legacy flat keys while the sample app binds a Providers section. Teach SettingsFixture to prefer Providers:*:ApiKey values and fall back to the legacy flat keys so local overrides keep working while test and sample configuration stay consistent. * Restore strict shared assertions and stable Google filter coverage The audit found two test-integrity regressions on this branch: shared White House assertions had been weakened, and the Google postal-code filter coverage was reduced to a URL-construction check. Restore the stricter shared assertions and reinstate live postal-code filter coverage with a stable Google case that returns OK today. * Restore explicit Yahoo test skipping for deprecated provider The branch docs and README say Yahoo remains deprecated and unverified, but the test suite had been re-enabled by deleting the old skip wrappers. Restore an explicit centralized skip in YahooGeocoderTest so the branch behavior matches the documented provider status and upstream OAuth issue history. * Tighten branch test integrity after full audit The full test audit found remaining expectation drift and a few integrity regressions in the branch. Align the async base White House query with the sync base, restore strict shared assertions, update Google live expectations to match current provider responses, keep Yahoo explicitly skipped while deprecated, and scope obsolete warning suppression to the Yahoo compatibility test only. * Require U.S. country signal in Google bounds-bias test The bounds-bias test still needed to tolerate ZIP-code drift, but it should not lose the country-level assertion. Keep the stable locality substring check and explicitly require the U.S. country signal in both the formatted address and the parsed address components. * Keep provider compatibility tests in provider suites Root cause: the shared test bases eagerly created live geocoder instances in their constructors, which pushed cheap constructor checks into separate files and made the test layout drift away from provider-specific suites. Switching the base tests to lazy generic geocoder access keeps those checks colocated without extra wrapper classes or reflection hacks. * Align Yahoo test configuration with provider settings Root cause: Yahoo credentials and behavior had drifted away from the shared provider-configuration pattern, which left tests, sample config, and README guidance out of sync. This restores Yahoo credential gating in tests under Providers:Yahoo while keeping the sample app clear that Yahoo stays out of the runnable surface because the legacy provider still uses insecure endpoints. * 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. * Align project agents with Geocoding.net workflows Root cause: the repo's custom agents and owned skills were copied from Exceptionless-specific workflows and still referenced invalid skills, paths, secret storage, and handoff semantics for this codebase. * Harden Microsoft parser edge cases Malformed provider payloads and non-int enum values were still able to slip through shared deserialization paths, which could throw or allocate more than necessary during Bing and Azure response parsing. * Restore provider compatibility surfaces The remaining review feedback traced back to public compatibility breaks and stale repo wiring around legacy provider paths. This restores HERE legacy credential support, hardens provider request construction, preserves Google enum values, and realigns docs/tests/sample config with the supported compatibility surface. * Remove HERE legacy credential flow Root cause: the branch briefly restored the retired HERE app_id/app_code path even though this major-version line is meant to standardize on the current API-key-based Geocoding and Search API. Remove the legacy HERE constructor, parsing models, sample/test config, and docs so the public surface matches the intended breaking-change direction. * Tidy remaining sample and parser review feedback Root cause: a few low-risk review comments remained open after the provider cleanup even though the behavior-preserving fixes were straightforward. Alphabetize the sample provider switch and make the Core/Bing filtering logic explicit so the outstanding review feedback matches the current implementation. * Preserve Microsoft enum compatibility Root cause: adding fallback enum members and parser hardening without locking the public numeric surface left compatibility and review gaps in the Microsoft provider package. Restore stable EntityType numeric values, make Azure result filtering explicit, and add a regression snapshot so future enum drift is caught in tests. * Polish Azure parser filtering Root cause: the last Azure Maps parser cleanup still left one code-quality complaint on the reverse-address mapping loop. Use a Select-based reverse-address pipeline so the parser stays explicit about filtering and mapping without changing behavior. * Re-enable Yahoo tests behind credential gating Root cause: the Yahoo suite had been hard-skipped even when a user supplied credentials, which prevented intentional compatibility runs. Remove the unconditional Skip attributes so the existing settings gate controls whether the Yahoo tests execute. * Harden provider transport error handling Root cause: the HttpClient transport cleanup left several provider failure paths under-tested and changed exception diagnostics/contracts in ways that made regressions easier to miss. * Align Google and Yahoo transport failures Root cause: sibling providers still used the pre-hardening HTTP failure path, so status handling and bounded diagnostics were inconsistent after the transport cleanup. * Tighten transport regression coverage Root cause: the new transport hardening tests still had analyzer noise and one locale-sensitive Google channel edge case without regression coverage. * Fix Location hash stability Root cause: Location.GetHashCode accidentally combined latitude with itself, so longitude never influenced the hash for this public value-like core type. * Clean up final analyzer findings Root cause: the latest push triggered one more static-analysis pass that flagged a disposable test-helper pattern, two Yahoo immutability nits, one unreachable Yahoo throw, and avoidable empty-array allocations in MapQuest. * Close remaining final-review gaps Root cause: the provider-modernization branch still had a small set of follow-up inconsistencies around nullable annotations, Bing response handling, and the shared HttpClient test helper after the larger transport and serializer refactors. * Split shared helpers by concern Root cause: the monolithic Extensions type mixed collection, enumerable, and JSON behavior in one file, which made the API surface harder to navigate and blocked a cleaner namespace layout for the shared helpers. * Move shared helpers into Geocoding.Extensions Root cause: the first split kept a compatibility shim and legacy JSON method casing, which preserved old entry points but did not match the requested .Extensions namespace layout or the desired .NET-style naming for JSON helpers. * Use the new helpers as real extension methods Root cause: the previous revision kept the split helper classes but still invoked them like static utilities, which did not match the requested extension-method style or the naming implied by the new Geocoding.Extensions surface. * Build a real docs site and align sample/test surfaces Root cause: the repo still exposed thin ad hoc documentation, public testing docs, and sample/test structure that had drifted from the current provider model and lacked dedicated docs validation.\n\nThis adds a VitePress docs site with provider playbooks, moves plan material out of docs, adds dedicated docs CI, aligns shared test namespaces/helpers with folder structure, and makes the sample advertise only providers that are actually usable under its configuration contract. * Stabilize provider parsing and request handling Root cause: several provider code paths still relied on culture-sensitive protocol normalization and ambiguous deserialization or disposal patterns, which left review threads open and behavior dependent on locale/runtime details. * Tighten test naming and remove request disposal ownership * Apply suggestion from @niemyjski * Address remaining PR test feedback Root cause: recent test refactors introduced assertion/name mismatches and test-guard synchronization risks that triggered new review threads. * Harden MapQuest exception handling Root cause: MapQuest transport errors still exposed key-bearing request URLs in exception context and used raw Exception instead of provider-specific exceptions. * Switch Yahoo endpoints to HTTPS Root cause: Yahoo constants and test expectations still allowed plaintext HTTP URLs, which left an insecure transport path in the compatibility provider. * Restore strict Google bias and postal assertions Root cause: test modernization simplified two Google assertions and removed important location-specific expectations used to guard regression behavior. * Update README.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update docs/guide/sample-app.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent c62e0f0 commit cd511b1

File tree

113 files changed

+8901
-1927
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

113 files changed

+8901
-1927
lines changed
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
---
2+
name: geocoding-library
3+
description: >
4+
Use this skill when implementing, reviewing, or triaging changes in Geocoding.net. Covers
5+
provider isolation, shared geocoding abstractions, provider-specific address and exception
6+
types, xUnit test strategy, API-key-backed test constraints, backward compatibility, and the
7+
sample web app's role in the repository.
8+
---
9+
10+
# Geocoding.net Library Patterns
11+
12+
## When to Use
13+
14+
- Any change under `src/`, `test/`, `samples/`, `.claude/`, or repo-owned customization files
15+
- Bug fixes that may repeat across multiple geocoding providers
16+
- Code reviews or triage work that needs repo-specific architecture context
17+
18+
## Architecture Rules
19+
20+
- Keep shared abstractions in `src/Geocoding.Core`
21+
- Keep provider-specific request/response logic inside that provider's project
22+
- Do not leak provider-specific types into `Geocoding.Core`
23+
- Prefer extending an existing provider pattern over inventing a new abstraction
24+
- Keep public async APIs suffixed with `Async`
25+
- Keep `CancellationToken` as the final public parameter and pass it through the call chain
26+
27+
## Provider Isolation
28+
29+
- Each provider owns its own address type, exceptions, DTOs, and request logic
30+
- If a bug or improvement appears in one provider, compare sibling providers for the same pattern
31+
- Shared helpers should only move into `Geocoding.Core` when they truly apply across providers
32+
33+
## Backward Compatibility
34+
35+
- Avoid breaking public interfaces, constructors, or model properties unless the task explicitly requires it
36+
- Preserve existing provider behavior unless the task is a bug fix with a documented root cause
37+
- Keep exception behavior intentional and provider-specific
38+
39+
## Testing Strategy
40+
41+
- Extend existing xUnit coverage before creating new test files when practical
42+
- Prefer targeted test runs for narrow changes
43+
- Run the full `Geocoding.Tests` project when shared abstractions, common test bases, or cross-provider behavior changes
44+
- Remember that some provider tests require local API keys in `test/Geocoding.Tests/settings-override.json` or `GEOCODING_` environment variables; keep the tracked `settings.json` placeholders empty
45+
- For bug fixes, add a regression test when the affected path is covered by automated tests
46+
47+
## Validation Commands
48+
49+
```bash
50+
dotnet build Geocoding.slnx
51+
dotnet test --project test/Geocoding.Tests/Geocoding.Tests.csproj
52+
dotnet build samples/Example.Web/Example.Web.csproj
53+
```
54+
55+
## Sample App Guidance
56+
57+
- `samples/Example.Web` demonstrates the library; it should not drive core design decisions
58+
- Only build or run the sample when the task actually touches the sample or requires manual verification there
59+
60+
## Customization Files
61+
62+
- `.claude/agents` and repo-owned skills must stay Geocoding.net-specific
63+
- Reference only skills that exist in `.agents/skills/`
64+
- Reference only commands, paths, and tools that exist in this workspace
65+
- Keep customization workflows aligned with AGENTS.md

.agents/skills/security-principles/SKILL.md

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,27 @@
11
---
22
name: security-principles
33
description: >
4-
Use this skill when handling secrets, credentials, PII, input validation, or any
5-
security-sensitive code. Covers secrets management, secure defaults, encryption, logging
6-
safety, and common vulnerability prevention. Apply when adding authentication, configuring
7-
environment variables, reviewing code for security issues, or working with sensitive data.
4+
Use this skill when handling provider API keys, external geocoding responses, request
5+
construction, logging safety, or other security-sensitive code in Geocoding.net. Apply when
6+
reviewing secrets handling, input validation, secure transport, or safety risks around
7+
external provider integrations and sample/test configuration.
88
---
99

1010
# Security Principles
1111

1212
## Secrets Management
1313

14-
Secrets are injected via Kubernetes ConfigMaps and environment variables never commit secrets to the repository.
14+
Provider credentials belong in local override files or environment variables and must never be committed to the repository.
1515

16-
- **Configuration files** — Use `appsettings.yml` for non-secret config
17-
- **Environment variables** — Secrets injected at runtime via `EX_*` prefix
18-
- **Kubernetes** — ConfigMaps mount configuration, Secrets mount credentials
16+
- **Tracked placeholders**`test/Geocoding.Tests/settings.json` is versioned and should contain placeholders only; do not put real keys there
17+
- **Test credentials** — Keep provider API keys in `test/Geocoding.Tests/settings-override.json` or via `GEOCODING_` environment variables
18+
- **Sample configuration** — Use placeholder values only in `samples/Example.Web/appsettings.json`
19+
- **Environment variables** — Use environment variables for CI or local overrides when needed
1920

2021
```csharp
21-
// AppOptions binds to configuration (including env vars)
22-
public class AppOptions
22+
public sealed class ProviderOptions
2323
{
24-
public string? StripeApiKey { get; set; }
25-
public AuthOptions Auth { get; set; } = new();
24+
public string? ApiKey { get; set; }
2625
}
2726
```
2827

@@ -31,24 +30,25 @@ public class AppOptions
3130
- Check bounds and formats before processing
3231
- Use `ArgumentNullException.ThrowIfNull()` and similar guards
3332
- Validate early, fail fast
33+
- Validate coordinates, address fragments, and batch sizes before sending requests
3434

3535
## Sanitize External Data
3636

37-
- Never trust data from queues, caches, user input, or external sources
37+
- Never trust data from geocoding providers, user input, or sample configuration
3838
- Validate against expected schema
39-
- Sanitize HTML/script content before storage or display
39+
- Handle missing or malformed response fields without assuming provider correctness
4040

4141
## No Sensitive Data in Logs
4242

43-
- Never log passwords, tokens, API keys, or PII
43+
- Never log passwords, tokens, API keys, or raw provider payloads
4444
- Log identifiers and prefixes, not full values
4545
- Use structured logging with safe placeholders
4646

4747
## Use Secure Defaults
4848

49-
- Default to encrypted connections (SSL/TLS enabled)
50-
- Default to restrictive permissions
51-
- Require explicit opt-out for security features
49+
- Default to HTTPS provider endpoints
50+
- Avoid disabling certificate or transport validation
51+
- Require explicit opt-out for any non-secure development-only behavior
5252

5353
## Avoid Deprecated Cryptographic Algorithms
5454

@@ -64,9 +64,15 @@ Use modern cryptographic algorithms:
6464

6565
## Input Bounds Checking
6666

67-
- Enforce minimum/maximum values on pagination parameters
67+
- Enforce minimum/maximum values on pagination or batch parameters
6868
- Limit batch sizes to prevent resource exhaustion
69-
- Validate string lengths before storage
69+
- Validate string lengths before request construction
70+
71+
## Safe Request Construction
72+
73+
- URL-encode user-supplied address fragments and query parameters
74+
- Do not concatenate secrets or untrusted input into URLs without escaping
75+
- Preserve provider-specific signing or authentication requirements without leaking secrets into logs
7076

7177
## OWASP Reference
7278

0 commit comments

Comments
 (0)