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

WFCORE-6347 Add ServiceDescriptor variants of OperationContext.hasOptionalCapability(...) and CapabilityServiceSupport.hasCapability(...) #5818

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

pferraro
Copy link
Contributor

@pferraro pferraro commented Jan 2, 2024

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jan 2, 2024
@pferraro
Copy link
Contributor Author

pferraro commented Jan 3, 2024

@bstansberry The full integration test failures are due to a number of instances in wildfly full where the caller invokes OperationContext.hasOptionalCapability(...) with a null "dependent" parameter resulting in method signature ambiguity. The javadoc indicates that this is forbidden, but git blame indicates that this there was an intentional effort to temporarily allow a null dependent via https://issues.redhat.com/browse/WFCORE-900.
Can you can elaborate on the current state of this? Why would a operation handler need to call this method with a null "dependent" parameter rather than use OperationContext.getCapabilityServiceSupport().hasCapability(String)?

@pferraro pferraro requested a review from bstansberry January 15, 2024 14:15
@pferraro
Copy link
Contributor Author

@bstansberry Wondering if you had a moment to look at this.

@bstansberry
Copy link
Contributor

bstansberry commented Feb 1, 2024

@bstansberry The full integration test failures are due to a number of instances in wildfly full where the caller invokes OperationContext.hasOptionalCapability(...) with a null "dependent" parameter resulting in method signature ambiguity. The javadoc indicates that this is forbidden, but git blame indicates that this there was an intentional effort to temporarily allow a null dependent via https://issues.redhat.com/browse/WFCORE-900. Can you can elaborate on the current state of this? Why would a operation handler need to call this method with a null "dependent" parameter rather than use OperationContext.getCapabilityServiceSupport().hasCapability(String)?

I don't see any need to continue to be permissive. In the WFCORE-900 description I was pretty clear that the contract (the javadoc) doesn't allow null, so that permissiveness can only be a temporary thing. That was in Aug 2015 when capabilities and requirements were really new and the bulk of the server didn't use them. So we needed flexibility to let us iterate toward doing it right. 8+ years later any code that isn't doing it right needs to be fixed and the permissiveness can go away.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

@wildfly-ci

This comment was marked as outdated.

…ionalCapability(...) and CapabilityServiceSupport.hasCapability(...) for convenience.
@wildfly-ci

This comment was marked as off-topic.

@wildfly-ci

This comment was marked as off-topic.

@pferraro
Copy link
Contributor Author

@bstansberry FYI - the downstream conflicts have been resolved and this is ready for review.

@pferraro pferraro added the 24.x label Apr 9, 2024
@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Apr 10, 2024
@yersan yersan merged commit 5d45d68 into wildfly:main Apr 10, 2024
12 checks passed
@yersan
Copy link
Collaborator

yersan commented Apr 10, 2024

Thanks @pferraro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants