Skip to content

feat(opensearch): configureSettings escape hatch on both client factories - #123

Merged
bfarmer67 merged 1 commit into
mainfrom
devs/bfarmer/opensearch-connection-settings-hook
Aug 26, 2026
Merged

bfarmer67 merged 1 commit into
mainfrom
devs/bfarmer/opensearch-connection-settings-hook

Conversation

@bfarmer67

@bfarmer67 bfarmer67 commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #122 (which is stacked on #121). GitHub retargets automatically as each merges. Review the third commit only.

Closes a real consumer gap: there was no way to reach ConnectionSettings, so the only escape was to stop calling AddOpenSearchAwsClient and fork it.

API

services.AddOpenSearchClient(
    endpoint,
    auth     => auth.Mode = OpenSearchAuthenticationMode.Basic,
    settings => settings.RequestTimeout( TimeSpan.FromMinutes( 2 ) )
                        .MaximumRetries( 5 )
                        .EnableHttpCompression() );

services.AddOpenSearchAwsClient(
    endpoint,
    aws      => aws.Region = "us-east-1",
    settings => settings.RequestTimeout( TimeSpan.FromMinutes( 2 ) ) );

Both IConfiguration overloads get the same treatment.

Purely additive: source- AND binary-compatible with 3.1.x. This ships as four new overloads, not as an appended optional parameter. Appending one is source-compatible but not binary-compatible - it changes the existing method's signature, so the 3.1.x entry point stops existing and anything compiled against it throws MissingMethodException until recompiled. This library claims SemVer; a minor release must not do that.

The four 3.1.x signatures are preserved exactly and forward to the new ones. Their = null defaults move to the new overloads, which is not a signature change (a default is parameter metadata, and a 3.1.x caller that omitted the argument already baked the null into its own call site) and is what keeps the pair unambiguous:

AddOpenSearchClient( services, uri )                         -> new overload (both default)
AddOpenSearchClient( services, uri, auth )                   -> 3.1.x overload (fewer params wins)
AddOpenSearchClient( services, uri, auth, cfg )              -> new overload
AddOpenSearchClient( services, uri, configureSettings: cfg ) -> new overload

Two reflection tests pin all eight signatures so the guarantee cannot regress silently. Reflection rather than call sites on purpose - a source-level call binds happily to a widened signature and proves nothing.

Applied last, after the endpoint and auth wiring. An escape hatch the library can silently overwrite is not an escape hatch; there's a test asserting a consumer BasicAuthentication beats the library's.

Action<ConnectionSettings> rather than Func<...>: ConnectionSettings mutates in place and returns itself, so a required return adds only a way to get it wrong — and Action<TOptions> is the configure-callback idiom already used throughout these extensions.

Both packages route through one internal BuildClient, so the hook and its validation behave identically on both paths. That needs InternalsVisibleTo from core to .Aws.

Why this is not the _mget fix

Worth being explicit since this PR exists because of that bug. Fixing #121 by having consumers declare DefaultMappingFor<OpenSearchMigrationRecord> would make correct operation opt-in and push a library-internal type into consumer wiring. #121 fixes it properly; this is a separate capability for consumer-owned concerns. Recorded in ADR-0030 with ADR-0029 as the counterweight.

The guardrail

The hook introduces one real foot-gun, so it ships with the safety.

The ledger index is created with a strict mapping using camelCase fields, matching the client's default field-name inference. A hook that replaces the serializer or sets a client-wide non-camelCase DefaultFieldNameInferrer breaks every ledger write. That failure is loud on its own — strict_dynamic_mapping_exception — but it arrives at first write, names fields rather than the cause, and reads like a schema problem.

Registration probes one known ledger property through the configured inferrer and fails with the cause and the remediation:

The configureSettings callback changed field-name inference: the ledger property
`AppliedBy` now serializes as `AppliedBy` instead of `appliedBy`. The migration ledger
index is created with a strict mapping using camelCase field names, so every ledger
write would be rejected with strict_dynamic_mapping_exception.

This is usually caused by replacing the serializer or by calling
DefaultFieldNameInferrer(...) with a non-camelCase convention. Both are supported for
your own document types -- scope them with DefaultMappingFor<TDocument>() instead of
changing the client-wide default, or register a separate IOpenSearchClient for
application use and leave the migration client stock.

One probe is enough — field-name inference is a single client-wide setting. Validation runs only when a hook is supplied, so the default path costs nothing.

I deliberately did not extend the validation to the whole class of ledger-adjacent settings (serializer, DefaultIndex, IConnection). Only field-name inference actually breaks the ledger; DefaultIndex is legitimate and #121 already makes the ledger immune to index inference. Replacing IConnection on the SigV4 path removes request signing — documented on the parameter rather than validated, since enumerating every way to misconfigure a transport isn't tractable.

Tests

OpenSearchConnectionSettingsHookTests (9):

  • hook reaches the resolved client — core and AWS paths
  • hook runs after auth wiring (consumer BasicAuthentication wins)
  • hook stays optional; existing call shapes unchanged
  • IConfiguration overload forwards it
  • ledger-breaking inferrer loud-fails with remediation — core and AWS paths
  • the recommended remediation actually works: DefaultMappingFor<ConsumerDocument> passes
  • ADR-0029 cross-check: a consumer DefaultIndex does not become the ledger's index

Full suite: 450 + 889 + 38 pass (unit tier was 433 + 889 + 38 on main). Whole solution builds clean on net8/9/10.

Docs

ADR-0030, docs/site/opensearch.md ("Tuning the client"), and the .Aws package README.

Not doing: symmetric hooks on the other providers

I looked, and the symmetry argument doesn't hold. Aerospike (IAsyncClient), Couchbase (IClusterProvider), MongoDB (IMongoClient), and Postgres (NpgsqlDataSource) all resolve a client the consumer registered — they already have 100% control, and there's nothing to open up. OpenSearch is the outlier because it's the only one that constructs a client. Adding factories elsewhere to make the hook symmetric would be inventing the problem to justify the solution.

The thing that did generalize was the underlying coupling, and that's #122.

🤖 Generated with Claude Code

@bfarmer67
bfarmer67 force-pushed the devs/bfarmer/ledger-field-name-contract branch from d9529ee to c0b6a9c Compare August 26, 2026 17:31
@bfarmer67
bfarmer67 force-pushed the devs/bfarmer/opensearch-connection-settings-hook branch 2 times, most recently from 2f23076 to 83bc0ba Compare August 26, 2026 19:36
@bfarmer67
bfarmer67 force-pushed the devs/bfarmer/ledger-field-name-contract branch from c0b6a9c to 5a99951 Compare August 26, 2026 21:52
@bfarmer67
bfarmer67 force-pushed the devs/bfarmer/opensearch-connection-settings-hook branch from 83bc0ba to b32e1e5 Compare August 26, 2026 21:52
…ries

AddOpenSearchClient and AddOpenSearchAwsClient (and both IConfiguration
overloads) take an optional trailing Action<ConnectionSettings>, applied
LAST -- after the endpoint and authentication wiring -- so a consumer can
override anything the library set. All parameters are optional and
trailing, so every existing call site compiles unchanged.

OpenSearch is the only provider whose client the library constructs. The
other four resolve a consumer-registered client and already have full
control. Until now the only way to reach ConnectionSettings -- for
RequestTimeout, MaximumRetries, EnableHttpCompression, a proxy,
ServerCertificateValidationCallback on a self-signed development cluster,
DisableDirectStreaming while debugging, or a DefaultMappingFor over the
consumer's OWN document types -- was to stop calling the factory and
hand-roll the registration. That forks the auth-mode switch, the
certificate loader, the AWS-endpoint loud-fail, and the mutual-exclusion
guard, and the fork then silently misses every later fix to any of them.

Both packages route through one internal BuildClient so the hook and its
validation behave identically on both registration paths.

Because the hook makes it reachable through supported API, registration
probes one ledger property through the configured inferrer and loud-fails
when field-name inference is no longer camelCase. The ledger index carries
a strict camelCase mapping, so such a change breaks every ledger write --
loud on its own, but arriving at first write naming fields rather than the
cause. Validation runs only when a hook is supplied.

This is deliberately NOT the remedy for the _mget defect fixed earlier in
this stack. Making correct operation depend on a consumer-declared mapping
for a library-internal type inverts ownership; see ADR-0029. The hook is
worth having on its own merits, for consumer-owned concerns.

No symmetric change for the other four providers: they do not construct
clients, so there is nothing to open up.

Per ADR-0030.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bfarmer67
bfarmer67 force-pushed the devs/bfarmer/opensearch-connection-settings-hook branch from b32e1e5 to 524d587 Compare August 26, 2026 21:53
@bfarmer67
bfarmer67 changed the base branch from devs/bfarmer/ledger-field-name-contract to main August 26, 2026 21:53
@bfarmer67
bfarmer67 merged commit efaf92c into main Aug 26, 2026
2 checks passed
@bfarmer67
bfarmer67 deleted the devs/bfarmer/opensearch-connection-settings-hook branch August 29, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant