Introduce RestHandler.Wrapper to help with delegate implementations

[vrozov]

Description

Wrapper class that implements delegate pattern for RestHandler

Issues Resolved

None, this is java utility class that can be utilized by OpenSearch core and OpenSearch plugins

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

✅   DCO Check Passed 2e686413840026ac523175b51c26365d6f2c93eb

[opensearch-ci-bot]

✅   Gradle Wrapper Validation success 2e686413840026ac523175b51c26365d6f2c93eb

[opensearch-ci-bot]

✅   Gradle Precommit success 2e686413840026ac523175b51c26365d6f2c93eb

[opensearch-ci-bot]

✅   DCO Check Passed 4da9029898bd32766b437e4ac0143305998288a3

[opensearch-ci-bot]

✅   Gradle Wrapper Validation success 4da9029898bd32766b437e4ac0143305998288a3

[opensearch-ci-bot]

✅   Gradle Precommit success 4da9029898bd32766b437e4ac0143305998288a3

[AmiStrn]

Is there a specific use case you would like to use this wrapper for?
Otherwise, there are many utilities one might add to any java project. But since there is no use case it is essentially adding unused code to the project. Perhaps couple it with a PR where this wrapper is required?

[vrozov]

The current use case is outside of OpenSearch repo in the security plugin that returns not null UnaryOperator<RestHandler> in ActionHandler.getRestHandlerWrapper. See https://github.com/opensearch-project/security/issues/1307#issuecomment-885072332. Implementing delegate wrapper in RestHandler will help with properly delegating all new (default) methods should one be introduced to the RestHandler in the future.

@nknize @dblock @CEHENKLE please comment on https://github.com/opensearch-project/security/issues/1307#issuecomment-886155806.

[AmiStrn]

Great:) However, if external plugins are to rely on this code then i think it should be protected by a test.

[vrozov]

Please suggest what test should be implemented? As there is no new functionality there is no test. This is plain delegate pattern. Also, there is no test for BaseRestHandler.Wrapper as well.

[AmiStrn]

there is no test for BaseRestHandler.Wrapper as well.

Good question! OpenSearchDashboardsPluginTests does in fact test the OpenSearchDashboardsWrappedRestHandler which extends BaseRestHandler.Wrapper. So the BaseRestHandler.Wrapper does actually have some test coverage.

I don't think every commit has to have a test accompanying it, however, this is code you say you require for plugins to work - without coverage how can you guarantee that this is not going to break plugins relying on it in the future?

Please suggest what test should be implemented?

This is really up to you, maybe implement an example plugin test case (if there isn't one already) that uses this code?

[vrozov]

Good question! OpenSearchDashboardsPluginTests does in fact test the OpenSearchDashboardsWrappedRestHandler which extends BaseRestHandler.Wrapper. So the BaseRestHandler.Wrapper does actually have some test coverage.

There is no (unit) test for BaseRestHandler.Wrapper in OpenSearchDashboardsPluginTests. I don't see any calls to OpenSearchDashboardsPlugin.getRestHandlers() where wrapper will be constructed. Even constructing wrapper will not test wrapper functionality correctly.

This is really up to you, maybe implement an example plugin test case (if there isn't one already) that uses this code?

This is beyond the scope of the current PR, but agree that it will be good to test ActionPlugin.getRestHandlerWrapper(). Possibly maintainers of OpenSearch can add such test.

[dblock]

start gradle check

[opensearch-ci-bot]

✅   Gradle Check success 4da9029898bd32766b437e4ac0143305998288a3
Log 357

Reports 357

[dblock]

I think this need a test that shows how this is used, at least. For starters, the wrapper constructor now raises an error when the delegate is null, could be a test. Methods that call delegate. could be invoked. Missing tests for BaseRestHandler should be added, just because they weren't added earlier doesn't mean you shouldn't try - since you're touching this code, make some effort to catch up what's missing please so we can leave things in a better state than they were.

[opensearch-ci-bot]

✅   Gradle Wrapper Validation success 0990085

[opensearch-ci-bot]

✅   DCO Check Passed 0990085

[opensearch-ci-bot]

✅   Gradle Precommit success 0990085

[vrozov]

Wrapper class is part of RestHandler interface and interfaces usually don't have unit tests as there is no implementation behind interfaces to test. Wrapper class (delegate pattern) delegates all of its functionality to delegate and I don't see any reasonable (the one that will provide value and not just will help with a unit test checkmark and add developer maintenance cost) unit test for it except one that ensures that all RestHelper methods are properly forwarded to the delegate. Adding the later unit test should be done for other wrapper classes as well and suggests introducing a common helper class (probably with some reflection utilization). I don't have time to work on introducing such helper class, but if one is introduced, will update this PR with such unit test.

A test case that shows how Wrapper class can be used should be part of ActionPlugin.getRestHandlerWrapper().

I can remove changes to BaseRestHandler.Wrapper if adding unit tests for that class is a precondition. The added value of null check is minimal (only helps with troubleshooting and does not actually impacts Wrapper class functionality as without the check NullPointerException will be delayed until the instance is used compared to when it is created).

[vrozov]

@VachaShah another instance of Link Checker failure.

[dblock]

Wrapper class is part of RestHandler interface and interfaces usually don't have unit tests as there is no implementation behind interfaces to test. Wrapper class (delegate pattern) delegates all of its functionality to delegate and I don't see any reasonable (the one that will provide value and not just will help with a unit test checkmark and add developer maintenance cost) unit test for it except one that ensures that all RestHelper methods are properly forwarded to the delegate. Adding the later unit test should be done for other wrapper classes as well and suggests introducing a common helper class (probably with some reflection utilization). I don't have time to work on introducing such helper class, but if one is introduced, will update this PR with such unit test.

A test case that shows how Wrapper class can be used should be part of ActionPlugin.getRestHandlerWrapper().

I can remove changes to BaseRestHandler.Wrapper if adding unit tests for that class is a precondition. The added value of null check is minimal (only helps with troubleshooting and does not actually impacts Wrapper class functionality as without the check NullPointerException will be delayed until the instance is used compared to when it is created).

I don't want to argue. My personal take is that when I write any code, I write tests that at least call that code so I can catch my own dumb mistakes like typo-ing delegate.callthewrongmethod from a copy-paste, or to make sure someone doesn't remove this accidentally because they feel the code is unnecessary because it's not used in this project. I'll let it be for this PR.

[dblock]

start gradle check

[opensearch-ci-bot]

✅   Gradle Check success 0990085
Log 362

Reports 362

[vrozov]

My personal take is that when I write any code, I write tests that at least call that code so I can catch my own dumb mistakes like typo-ing delegate.callthewrongmethod from a copy-paste

@dblock I mostly did not write that code. I used IDE (IntelliJ) to generate it.

to make sure someone doesn't remove this accidentally because they feel the code is unnecessary because it's not used in this project

unit test does not prevent this issue. Somebody can remove unit test as well, assuming that both code and unit test are not needed. My bigger concern is that somebody will add new method to an interface without adding it to delegate.

IMO, it will be better to revisit all wrappers and see which one don't have unit test and introduce a generic helper class that can validate proper delegation pattern.