refactor: shift core validation rules to internal methods

[xaevik]

No description provided.

[Copilot]

Pull request overview

This PR refactors the validation pipeline so shared validation rules in the request-validator base class cannot be skipped or reordered by provider implementations, while keeping provider-specific hooks intact.

Changes:

• Added internal validation hooks (PreValidateInternalAsync / ValidateInternalAsync) to AbstractValidator<T> and updated the execution template to run internal hooks before protected hooks.
• Moved the shared country and address-field rules in AbstractAddressValidationRequestValidator<T> into sealed internal overrides so provider validators cannot bypass them.
• Updated UPS integration and documentation/architecture docs to reflect the new execution model and API surface.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/Visus.AddressValidation.Tests/ApiTests.PublicApi_HasNoBreakingChanges_Async.verified.txt	Updates the public API snapshot to reflect the refactor (removal of protected overrides from the base request validator’s API surface).
src/Visus.AddressValidation/Validation/AbstractValidator.cs	Introduces internal hook layer and updates the template method to enforce internal-before-protected execution.
src/Visus.AddressValidation/Validation/AbstractAddressValidationRequestValidator.cs	Refactors shared request validation into sealed internal hooks (country + address-field rules).
src/Visus.AddressValidation.Integration.Ups/Validation/AddressValidationRequestValidator.cs	Removes redundant base pre-validation call and simplifies provider pre-validation to non-async ValueTask returns.
docs/docs/integrations/custom/validators.md	Updates integration guidance to match the new “shared rules always enforced” behavior.
ARCHITECTURE.md	Documents the new two-layer validation hook structure and its intent.

Suppressed comments (1)

docs/docs/integrations/custom/validators.md:40

• “This check always runs” is not accurate: the shared address-field validation runs only when pre-validation succeeds (i.e., when both the internal shared country check and your PreValidateAsync return true). If PreValidateAsync returns false, validation short-circuits and the address-field checks do not run.

The base class checks the remaining address fields after the country check passes: `AddressLines` must be non-empty and contain at most 3 lines; `CityOrTown`, `StateOrProvince`, and `PostalCode` must be present (with country-specific exceptions for city-states and countries without postal codes). This check always runs, whether or not you override `ValidateAsync`.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

src/Visus.AddressValidation/Validation/AbstractAddressValidationRequestValidator.cs:27

• Because PreValidateInternalAsync now runs before any provider-specific PreValidateAsync override, providers can no longer normalize the request (for example, set a default Country when it is missing) before the shared country check runs. If any existing integrations relied on that pattern, this refactor will change behavior (invalid request instead of normalization).

    internal sealed override ValueTask<bool> PreValidateInternalAsync(T instance, ISet<ValidationState> results)
    {
        if ( instance.Country is not null )
        {
            if ( SupportedCountries.Contains(instance.Country.Value) )

src/Visus.AddressValidation/Validation/AbstractValidator.cs:52

• The internal validation hooks do not accept a CancellationToken, so cancellation passed to ExecuteAsync cannot be observed by logic placed in PreValidateInternalAsync / ValidateInternalAsync. This makes it easy to accidentally add internal async work that cannot be canceled and will still run after cancellation is requested.

    // ReSharper disable once MemberCanBeProtected.Global
    internal virtual ValueTask<bool> PreValidateInternalAsync(T instance, ISet<ValidationState> results)
    {
        return ValueTask.FromResult(true);
    }

    // ReSharper disable once MemberCanBeProtected.Global
    internal virtual ValueTask ValidateInternalAsync(T instance, ISet<ValidationState> results)
    {
        return ValueTask.CompletedTask;
    }

ARCHITECTURE.md:77

• This sentence can read as if both shared hooks run before both provider overrides. ValidateInternalAsync runs before provider ValidateAsync, but it runs after provider PreValidateAsync. Consider clarifying the order per phase to avoid confusion for maintainers.

Each phase pairs an `internal virtual PreValidateInternalAsync`/`ValidateInternalAsync` hook with the existing `protected virtual PreValidateAsync`/`ValidateAsync` hook. `ExecuteInternalAsync` calls the internal hook first, then the protected hook, for each phase. The internal hooks are not part of the public API surface. They let a base class in this hierarchy enforce validation that a subclass cannot skip or reorder.

`AbstractAddressValidationRequestValidator<T>` implements `PreValidateInternalAsync` (the shared country check) and `ValidateInternalAsync` (the shared address-field rules: lines, city, state/province, postal code) as `internal sealed override`. Both run before any provider-specific `PreValidateAsync`/`ValidateAsync` override. A provider validator never needs to call `base.PreValidateAsync`/`base.ValidateAsync` to opt into the shared rules — those inherited protected members reach only `AbstractValidator<T>`'s no-op defaults. Provider-specific validators add their own rules by overriding the protected `PreValidateAsync`/`ValidateAsync` hooks, and supply their `SupportedCountries` frozen set and `ProviderName`.

docs/docs/integrations/custom/validators.md:12

• This paragraph says the base class runs both the shared country checks and the shared address-field checks "before your own checks run", but provider-specific checks in PreValidateAsync run before the base address-field validation (ValidateInternalAsync). Consider rewording to reflect the actual order so readers do not assume the address-field checks happen first.

Extend [`AbstractAddressValidationRequestValidator<TRequest>`](xref:Visus.AddressValidation.Validation.AbstractAddressValidationRequestValidator`1) and implement the two required abstract members. The base class runs its shared country and address-field checks automatically, before your own checks run. Override `PreValidateAsync` to add provider-specific checks. You do not need to call `base.PreValidateAsync` — the base class's checks always run first, regardless of what your override does.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/Visus.AddressValidation.Integration.Ups/Validation/AddressValidationRequestValidator.cs:71

• The error message lists supported regions using string.Join over a HashSet, which can produce a non-deterministic order (and therefore non-deterministic validation messages / snapshots). Sort the regions before joining so the message is stable.

        results.Add(ValidationState.CreateError(Resources.Validation_Provider_OnlyValuesSupportedInMode,
            nameof(instance.StateOrProvince),
            string.Join(", ", _supportedDevelopmentRegions),
            "UPS",
            ClientEnvironment.DEVELOPMENT));

docs/docs/integrations/custom/validators.md:12

• This paragraph says the base checks run "regardless of what your override does", but the shared address-field checks are skipped when PreValidateAsync returns false (as noted later). Reword to avoid the contradiction and keep the guidance precise.

Extend [`AbstractAddressValidationRequestValidator<TRequest>`](xref:Visus.AddressValidation.Validation.AbstractAddressValidationRequestValidator`1) and implement the two required abstract members. The base class runs its shared country check before your `PreValidateAsync` override. It runs its shared address-field checks after your `PreValidateAsync` override returns `true`. Override `PreValidateAsync` to add provider-specific checks. You do not need to call `base.PreValidateAsync` — the base class's checks always run at their fixed point in the pipeline, regardless of what your override does.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Visus.AddressValidation/Validation/AbstractValidator.cs:63

• The validator execution order changed (internal hooks now run before/after the protected hooks). This is a significant behavior change but there are no targeted unit tests that lock in the new guarantees (e.g., shared country validation always runs even when a provider override does not call base.*, and shared address-field rules run only when both pre-validation hooks succeed). Adding tests would help prevent regressions in the hook ordering and short-circuit logic.

        if ( !await PreValidateInternalAsync(context.Instance, context.ValidationResults, cancellationToken).ConfigureAwait(false) ||
             !await PreValidateAsync(context.Instance, context.ValidationResults, cancellationToken).ConfigureAwait(false) )
        {
            return new ValidationResult(context.ValidationResults);
        }

        await ValidateInternalAsync(context.Instance, context.ValidationResults, cancellationToken).ConfigureAwait(false);
        await ValidateAsync(context.Instance, context.ValidationResults, cancellationToken).ConfigureAwait(false);