Skip to content

fix: assorted robustness and clarity fixes - #3533

Draft
csviri wants to merge 1 commit into
operator-framework:mainfrom
csviri:fix/misc-robustness-nits
Draft

fix: assorted robustness and clarity fixes#3533
csviri wants to merge 1 commit into
operator-framework:mainfrom
csviri:fix/misc-robustness-nits

Conversation

@csviri

@csviri csviri commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Three unrelated small items found while auditing.

ManagedInformerEventSource.get could throw instead of falling back to the
informer cache. It compared a temporary-cache hit against
manager().lastSyncResourceVersion(namespace) without guarding either
failure mode:

  • lastSyncResourceVersion is null until an informer has completed its
    initial list, and compareResourceVersions dereferences it, so this
    threw a NullPointerException.
  • InformerManager.lastSyncResourceVersion does getSource(ns) .orElseThrow(), which throws NoSuchElementException for a namespace
    that is no longer watched after a dynamic namespace change - a window
    that only closes on the next re-list, when checkGhostResources prunes
    the entry.

TemporaryResourceCache.putResource already guards the null case; the
comparison is now extracted into a helper that guards both and falls back
to the informer cache, which is the safe answer in either case.

ExpectationResult.name() threw a bare NullPointerException when no
expectation was registered, which is a normal result state produced by
ExpectationManager.checkExpectation. It now throws
IllegalStateException pointing at isExpectationPresent(), and says so
in the javadoc.

ResourceOperations cleanups: drops the options parameter of
desiredForJsonPatch, which was never read at any of its six call sites,
and rewrites the comment on create - it claimed the operation "check if
the resource already exists", which it does not; the actual reason
filtering is safe is that the API server rejects a duplicate create.

Part of #3517

Three unrelated small items found while auditing.

ManagedInformerEventSource.get could throw instead of falling back to the
informer cache. It compared a temporary-cache hit against
`manager().lastSyncResourceVersion(namespace)` without guarding either
failure mode:

* `lastSyncResourceVersion` is null until an informer has completed its
  initial list, and `compareResourceVersions` dereferences it, so this
  threw a NullPointerException.
* `InformerManager.lastSyncResourceVersion` does `getSource(ns)
  .orElseThrow()`, which throws `NoSuchElementException` for a namespace
  that is no longer watched after a dynamic namespace change - a window
  that only closes on the next re-list, when `checkGhostResources` prunes
  the entry.

`TemporaryResourceCache.putResource` already guards the null case; the
comparison is now extracted into a helper that guards both and falls back
to the informer cache, which is the safe answer in either case.

ExpectationResult.name() threw a bare NullPointerException when no
expectation was registered, which is a normal result state produced by
`ExpectationManager.checkExpectation`. It now throws
`IllegalStateException` pointing at `isExpectationPresent()`, and says so
in the javadoc.

ResourceOperations cleanups: drops the `options` parameter of
`desiredForJsonPatch`, which was never read at any of its six call sites,
and rewrites the comment on `create` - it claimed the operation "check if
the resource already exists", which it does not; the actual reason
filtering is safe is that the API server rejects a duplicate create.
Copilot AI review requested due to automatic review settings July 30, 2026 09:05
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR delivers a small set of robustness and clarity improvements across the operator framework core, primarily addressing edge cases in informer cache reads, improving error reporting for missing expectations, and simplifying internal patch helper APIs.

Changes:

  • Prevent ManagedInformerEventSource.get from throwing when last-sync resource versions are unavailable or namespaces are no longer watched, preferring the informer cache in those cases.
  • Improve ExpectationResult.name() behavior and documentation by throwing a descriptive IllegalStateException when no expectation is present.
  • Clean up ResourceOperations by removing an unused internal parameter and correcting/improving create-event-filtering comments.

Reviewed changes

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

File Description
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/expectation/ExpectationResult.java Adds a guard + clearer exception and Javadoc for name() when no expectation is registered.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java Extracts resource-version comparison into a helper and adds guards to fall back to informer cache in problematic cases.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java Removes an unused internal parameter from JSON patch desired-state creation and clarifies create filtering comments.

Comment on lines +248 to +256
private boolean isLaterThanLastSyncResourceVersion(R resource) {
var namespace = resource.getMetadata().getNamespace();
if (!manager().isWatchingNamespace(namespace)) {
return false;
}
var lastSyncResourceVersion = manager().lastSyncResourceVersion(namespace);
if (lastSyncResourceVersion == null) {
return false;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants