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

Pass ExtensionsRunner instance to createComponents #477

Merged
merged 2 commits into from
Feb 21, 2023

Conversation

dbwiddis
Copy link
Member

Description

Guice has not finished initializing the injector when calling createComponents as the return objects need to be injected. Components which require objects from the ExtensionsRunner for instantiation need a copy of them available.

While the constructor could be filled with a long list of these parameters (as is done in plugins) this creates a hard API dependency on the parameters, and changing the number of parameters would break compatibility.

By passing the ExtensionsRunner, additional objects can be made available via getters without changing the API.

Issues Resolved

Improves on #466

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.

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Merging #477 (afefeae) into main (8f3bcde) will increase coverage by 0.20%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #477      +/-   ##
============================================
+ Coverage     65.96%   66.17%   +0.20%     
- Complexity      183      186       +3     
============================================
  Files            35       35              
  Lines           808      813       +5     
  Branches         24       24              
============================================
+ Hits            533      538       +5     
  Misses          264      264              
  Partials         11       11              
Impacted Files Coverage Δ
src/main/java/org/opensearch/sdk/Extension.java 100.00% <ø> (ø)
...main/java/org/opensearch/sdk/ExtensionsRunner.java 70.76% <100.00%> (+0.88%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dbwiddis dbwiddis force-pushed the createcomponentsfix branch from 9cd14cd to afefeae Compare February 21, 2023 07:49
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

We might need to revisit this logic while working on #467. Approved for now.

@owaiskazi19 owaiskazi19 merged commit b68bf20 into opensearch-project:main Feb 21, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 21, 2023
* Pass ExtensionsRunner instance to createComponents

Signed-off-by: Daniel Widdis <[email protected]>

* Add more getters

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Daniel Widdis <[email protected]>
(cherry picked from commit b68bf20)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@dbwiddis
Copy link
Member Author

We might need to revisit this logic while working on #467.

Agreed. Here's the overall logical constraints (which probably should find its way into code comments):

Order matters:

  1. Initialize concrete classes needed by create components. If these are things that may change during operation (e.g. NamedXContents) use a concrete wrapper class with the ability to update its internals.
  2. Inject classes (any order, Guice figures out the dependency map later)
    • Inject the concrete classes. Since they aren't yet available to create components, make sure they have getters and then just pass the runner to create components. Injecting them using the getters is a subtle reminder of this. :)
    • Inject the objects returned from create components.
    • Inject any other extension points that require injection (in this case getActions).
  3. After injection, call other extension points (e.g., rest handlers). This permits these other extension points to have access to the injected classes.

@dbwiddis dbwiddis deleted the createcomponentsfix branch February 21, 2023 19:09
dbwiddis pushed a commit that referenced this pull request Feb 21, 2023
* Pass ExtensionsRunner instance to createComponents



* Add more getters



---------


(cherry picked from commit b68bf20)

Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
…t#477)

* Pass ExtensionsRunner instance to createComponents

Signed-off-by: Daniel Widdis <[email protected]>

* Add more getters

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Daniel Widdis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants