Skip to content

Enforce non-null key in ValidatedKeyValue constructor#7578

Open
zbnerd wants to merge 1 commit into
micrometer-metrics:mainfrom
zbnerd:fix/validated-keyvalue-null-key
Open

Enforce non-null key in ValidatedKeyValue constructor#7578
zbnerd wants to merge 1 commit into
micrometer-metrics:mainfrom
zbnerd:fix/validated-keyvalue-null-key

Conversation

@zbnerd

@zbnerd zbnerd commented Jun 6, 2026

Copy link
Copy Markdown

Issue #7577.

Summary

ValidatedKeyValue did not enforce the package-level @NullMarked contract on its key parameter.

As a result, KeyValue.of(String, T, Predicate<? super T>) could create a KeyValue instance with getKey() == null. The object remained usable for most operations (equals, hashCode, toString, etc.) but later failed with NullPointerException in compareTo.

This change adds the missing null check and aligns ValidatedKeyValue with ImmutableKeyValue, which already enforces non-null constructor arguments.

Change

ValidatedKeyValue(String key, T value, Predicate<? super T> validator) {
    this.key = Objects.requireNonNull(key, "key must not be null");
    this.value = String.valueOf(assertValue(value, validator));
}

Tests

Added regression tests covering:

  • null key rejection in ValidatedKeyValue
  • null key rejection through KeyValue.of(String, T, Predicate)
  • null KeyName handling through the corresponding overload
  • preservation of existing validator behavior for null values
  • consistency between String and KeyName overloads

The validator overload of KeyValue.of(...) accepted a null key and
silently produced a half-broken object: getKey() returned null while
equals/hashCode/toString still worked, and the failure only surfaced
later in compareTo (via getKey().compareTo(...)).

The package is @NullMarked, so key is non-null by contract. ImmutableKeyValue
already enforces this with requireNonNull; ValidatedKeyValue did not.

Add Objects.requireNonNull(key, ...) to the constructor to fail fast at
construction time and align with the @NullMarked contract and the sibling
ImmutableKeyValue behavior. The validator's role (deciding value
nullability) is unchanged.

Closes micrometer-metricsgh-7577

Signed-off-by: zbnerd <zbnerd@users.noreply.github.com>
@zbnerd zbnerd force-pushed the fix/validated-keyvalue-null-key branch from edd4fd4 to 1b6b85c Compare June 6, 2026 18:30
@zbnerd

zbnerd commented Jun 7, 2026

Copy link
Copy Markdown
Author

One thing that makes this look unintentional is the consistency of the other overloads.

If the validator overload were intended to allow null keys whenever the predicate accepts the value, I would expect all key-accepting overloads to behave similarly.

Instead:

  • of(String, String) rejects null keys
  • of(KeyName, String) rejects null keys
  • of(KeyName, T, Predicate) rejects null keys
  • only of(String, T, Predicate) silently stores a null key

So regardless of whether one views the predicate overload as a special case for value validation, the current behavior leaves a single entry point that handles null keys differently from every other overload.

That asymmetry is what made me think this is more likely a missing null check than an intentional API distinction.

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