Skip to content

Refactor ApiRegistrar class away#1023

Merged
gerlowskija merged 2 commits intoapache:mainfrom
gerlowskija:nuke_api_registrar
Sep 19, 2022
Merged

Refactor ApiRegistrar class away#1023
gerlowskija merged 2 commits intoapache:mainfrom
gerlowskija:nuke_api_registrar

Conversation

@gerlowskija
Copy link
Contributor

@gerlowskija gerlowskija commented Sep 19, 2022

Description

Earlier on during v2 API refactoring, I had created ApiRegistrar as a way to bundle together the registration of API classes that were multiplying and cluttering up CoreContainer.load.

In hindsight, this was based on a misunderstanding that there was a reason to register APIs (directly) in CoreContainer.load in the first place. But there's not: both core and container-level APIs can be registered just fine via the 'getApis()' method present on their associated requestHandler. The only APIs that cannot use this approach, and must be registered explicitly in CoreContainer.load are those v2 APIs that don't have a v1/requestHandler equivalent.

Solution

With this in mind, I'm removing my ill-conceived attempt at an ApiRegistrar class, to simplify the overall picture for API registration.

Tests

N/A - change is a refactor only, so existing tests must continue to pass.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.

Earlier on during v2 API refactoring, I had created ApiRegistrar as a
way to bundle together the registration of API classes that were
multiplying and cluttering up CoreContainer.load.

In hindsight, this was based on a misunderstanding that there was a
reason to register APIs (directly) in CoreContainer.load in the first
place.  But there's not: both core and container-level APIs can be
registered just fine via the 'getApis()' method present on their
associated requestHandler.  The only APIs that cannot use this approach,
and must be registered explicitly in CoreContainer.load are those v2
APIs that don't have a v1/requestHandler equivalent.

With this in mind, I'm removing my ill-conceived attempt at an
ApiRegistrar class, to simplify the overall picture for API
registration.
@gerlowskija gerlowskija merged commit a414b3c into apache:main Sep 19, 2022
@gerlowskija gerlowskija deleted the nuke_api_registrar branch September 19, 2022 14:28
@gerlowskija gerlowskija changed the title Refactor way ApiRegistrar class Refactor ApiRegistrar class away Sep 19, 2022
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.

1 participant