Avoid override of routes() in BaseRestHandler to respect the default behavior defined in RestHandler

[cliu123]

Description

Remove override for routes() in BaseRestHandler to respect default behavior of routes() defined in RestHandler.
This will help developers avoid re-implementing routes() in the subclasses of BaseRestHandler
They can always override the method if they'd like.

Issues Resolved

#694

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]

✅   Gradle Wrapper Validation success f719e69883e71682fa7d1150c1f5c98c1299d11f

[opensearch-ci-bot]

❌   DCO Check Failed f719e69883e71682fa7d1150c1f5c98c1299d11f
Run ./dev-tools/signoff-check.sh remotes/origin/main f719e69883e71682fa7d1150c1f5c98c1299d11f to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

[opensearch-ci-bot]

✅   Gradle Precommit success f719e69883e71682fa7d1150c1f5c98c1299d11f

[CEHENKLE]

@cliu123 Can you add more information to this PR? Thanks...

[cliu123]

@cliu123 Can you add more information to this PR? Thanks...

Sure. This can be post-GA. I'll add more info later.

[opensearch-ci-bot]

✅   Gradle Wrapper Validation success f719e69883e71682fa7d1150c1f5c98c1299d11f

[opensearch-ci-bot]

❌   DCO Check Failed f719e69883e71682fa7d1150c1f5c98c1299d11f
Run ./dev-tools/signoff-check.sh remotes/origin/main f719e69883e71682fa7d1150c1f5c98c1299d11f to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

[opensearch-ci-bot]

✅   Gradle Precommit success f719e69883e71682fa7d1150c1f5c98c1299d11f

[opensearch-ci-bot]

✅   Gradle Wrapper Validation success 9af872758921add7c07210112a6a89d114daf623

[opensearch-ci-bot]

✅   DCO Check Passed 9af872758921add7c07210112a6a89d114daf623

[opensearch-ci-bot]

✅   Gradle Precommit success 9af872758921add7c07210112a6a89d114daf623

[dblock]

Assuming routes becomes private, how does one add a route?

How will existing implementers of BaseRestHandler need to be changed? I imagine many define routes in their own implementations?

[Bukhtawar]

Can we add more details on what this PR intends to solve

[cliu123]

Can we add more details on what this PR intends to solve

@Bukhtawar The issue #694 describes what this PR intends to solve.

[cliu123]

How will existing implementers of BaseRestHandler need to be changed?

They can remove the implementation of the abstract method routes if they do not need that method.

I imagine many define routes in their own implementations?

That is what happens, for example.
@dblock If defining routes field as protected in BaseRestHandler, from what I understand, the implementers would be allowed to add routes. Any thoughts? The intention is not to force implementers to implement the abstract method routes when they do not need the method.

[Bukhtawar]

From what I understand, we need to also ensure one of routes() or replacedRoutes() are implemented so that an implementer cannot get away without specifying any?

[cliu123]

From what I understand, we need to also ensure one of routes() or replacedRoutes() are implemented so that an implementer cannot get away without specifying any?

@Bukhtawar Agree. The intention of this PR is not to force implementers to implement the abstract method routes when they do not need the method. Perhaps routes was designed as an abstract method on purpose to ensure the implementers specify it?

[vrozov]

Why is it necessary to overwrite the default RestHandler implementation in the BaseRestHandler? Implementing it as abstract does not enforce anything. Yes, it needs to be implemented by all subclasses of BaseRestHandler, but there is no enforcement on the actual return and it can be null or an empty list. The proposed implementation is subject of NullPointerException and subclasses of RestHandler will still need to initialize routes to a list.

[cliu123]

Why is it necessary to overwrite the default RestHandler implementation in the BaseRestHandler? Implementing it as abstract does not enforce anything. Yes, it needs to be implemented by all subclasses of BaseRestHandler, but there is no enforcement on the actual return and it can be null or an empty list. The proposed implementation is subject of NullPointerException and subclasses of RestHandler will still need to initialize routes to a list.

@vrozov

Why is it necessary to overwrite the default RestHandler implementation in the BaseRestHandler?

IMO, It is actually unnecessary.

Implementing it as abstract does not enforce anything.

Is our goal that the subclasses of BaseRestHandlerdo not have to implement the method routes unless the subclasses really need to implement a custom behavior? If not, the issue #694 will need more details to clarify the goal.

there is no enforcement on the actual return and it can be null or an empty list.

For this purpose, removing the routes() method from the BaseRestHandler will make that happen?

[Bukhtawar]

Is there a harm returning an empty list and let the contract stay

[cliu123]

Is there a harm returning an empty list and let the contract stay

I do not think there is a harm returning an empty list, but looks like BaseRestHandler does not have to override the method routes(), and the abstract class routes() in BaseRestHandler makes subclasses of BaseRestHandler have to implement the method routes() even if the subclasses respects the default behavior defined in RestHandler.

[vrozov]

Is our goal that the subclasses of BaseRestHandlerdo not have to implement the method routes unless the subclasses really need the method? If not, the issue #694 will need more details to clarify the goal.

@cliu123 The reason RestHandler provides the default implementation of routes() (and other related methods) is to avoid unnecessary reimplementation of the method in the subclasses. By introducing abstract routes() method or introducing protected List<Route> routes to BaseRestHandler, the goal of the default interface implementation is defeated.

[opensearch-ci-bot]

✅   DCO Check Passed dc0b1dd

[opensearch-ci-bot]

❌   Gradle Precommit failure dc0b1dd
Log 787

[opensearch-ci-bot]

✅   DCO Check Passed 15bf7ae

[opensearch-ci-bot]

✅   Gradle Wrapper Validation success 15bf7ae

[opensearch-ci-bot]

✅   Gradle Precommit success 15bf7ae

[vrozov]

LGTM except for PR title

[cliu123]

LGTM except for PR title

@vrozov Changed the PR title

[cliu123]

@dblock @Bukhtawar How do the changes look to you?

[dblock]

@dblock @Bukhtawar How do the changes look to you?

Now that the unnecessary implementations have been removed in tests, this makes a lot more sense! Good with me.

You should still update the description in the PR for anyone reading this for the first time.

[dblock]

start gradle check

[opensearch-ci-bot]

✅   Gradle Check success 15bf7ae
Log 330

Reports 330

[cliu123]

You should still update the description in the PR for anyone reading this for the first time.

@dblock Done.

[saratvemulapalli]

Looks like I missed the whole party of conversation.
I like where we ended up!

[saratvemulapalli]

@dblock are we waiting for anything else to merge this ?

[dblock]

Merged, thanks @cliu123!

[cliu123]

@dblock @saratvemulapalli Are we going to backport this to 1.0 or 1.x(for 1.1.0) branches?

[dblock]

Since this is not a security fix it definitely doesn't go to 1.0, but I don't see why we wouldn't backport to 1.1. Feel free to make that PR @cliu123 if you need it.