Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method signature mismatched with javadoc and implementation #2587

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

robobario
Copy link
Contributor

@robobario robobario commented Nov 18, 2024

The javadoc of ManagedDependentResourceContext#put describe it as returning Optional. The implementation returns an Optional but casts it to T, so it is currently a compile error to write:

Optional<String> result = new ManagedDependentResourceContext().put("key", "value");

Using the method as declared results in a ClassCastException:

DefaultManagedDependentResourceContext context = new DefaultManagedDependentResourceContext();
context.put("key", "value");
String actual = context.put("key", "valueB");

fails with:

java.lang.ClassCastException: class java.util.Optional cannot be cast to class java.lang.String (java.util.Optional and java.lang.String are in module java.base of loader 'bootstrap')

The user can cast to get at the Optional like Optional<String> actual = (Optional<String>) (Object) context.put("key", "value");

So this PR updated the javadoc and implementation to match the method declaration and return the object-or-null rather than an Optional.

@openshift-ci openshift-ci bot requested review from adam-sandor and csviri November 18, 2024 00:16
@metacosm
Copy link
Collaborator

This should target the next branch because this would be a breaking API change. Alternatively, we would need to deprecate the existing methods and provide new implementations but this cannot go as-is in main.

@robobario
Copy link
Contributor Author

pushed up creating a new method and deprecating the old, a bit clunky since put is a nicer name than the new one.

@metacosm metacosm self-requested a review November 19, 2024 08:44
@metacosm
Copy link
Collaborator

Upon further review, looking at the history of the modifications, I believe that this API got broken at some point in the past and we should probably conform to the Map-like API and change the documentation accordingly instead of returning Optional. I have made the required changes but I cannot push to your branch, @robobario.

@metacosm
Copy link
Collaborator

@robobario see #2595 for the changes that I think should be included as part of this work.

Signed-off-by: Robert Young <robeyoun@redhat.com>
metacosm and others added 2 commits November 20, 2024 09:58
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Robert Young <robeyoun@redhat.com>
@robobario
Copy link
Contributor Author

great, thanks @metacosm . Have updated and cleaned up the git history (and renamed a test method)

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM

@metacosm metacosm merged commit 1f6cf2c into operator-framework:main Nov 20, 2024
20 checks passed
@robobario
Copy link
Contributor Author

thank you!

@metacosm
Copy link
Collaborator

Thank you for your work!

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.

3 participants