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

Use inject for non singleton class #1617

Merged
merged 34 commits into from
Dec 13, 2024
Merged

Conversation

jorg3lopez
Copy link
Contributor

@jorg3lopez jorg3lopez commented Nov 26, 2024

Description

This PR adds the logic necessary for non-singleton classes to use the @Inject annotation.

Changes

ApplicationContext.java

  • Refactoring of skipMissingImplementation logic
  • Renamed injectRegisteredImplementations -> doInjectRegisteredImplementations
  • Overload injectIntoField method
  • Added injectIntoNonSingleton() method

Reflections.java

  • Added getFieldsAnnotatedWithInstance

ApplicationContextText.groovy

  • Added test cases

TestApplicationContext.java

  • Added skipMissingImplementation logic

HapiFhirResource

  • The changes in this file were used for testing, they will get deleted once the PR is approved

HapiFhirResourceTest

  • This file was used for testing, it will get deleted once the PR is approved

Testing

  • Choose a non-singleton class to work with. Make sure the class is not dynamically instantiated
  • Use @Inject on fields that you want to inject the dependency
  • Make sure the class sends itself as a parameter toApplicationContext.injectIntoNonSingleton(), using the constructor
  • Build module and make sure all tests pass

Issue

#1361

Checklist

  • I have added tests to cover my changes

Note: You may remove items that are not applicable

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Exception Handling
The method 'injectIntoField' throws a generic IllegalArgumentException for any exception during field injection. This could mask different issues like security or logical errors which might need different handling.

Dependency Injection
The use of 'ApplicationContext.injectIntoNonSingleton(this)' within the constructor might lead to issues with incomplete construction if the injected dependencies themselves depend on the partially constructed object.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Prevent potential NullPointerException by checking if instance is null before setting a field

Ensure that injectIntoField checks for null instance before attempting to set the
field, to prevent NullPointerException.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/context/ApplicationContext.java [82]

-field.set(instance, fieldImplementation);
+if (instance != null) {
+    field.set(instance, fieldImplementation);
+}
Suggestion importance[1-10]: 8

Why: This suggestion correctly identifies a potential NullPointerException risk in the code and provides a solution to prevent it, enhancing the robustness of the method.

8
Enhance robustness by ensuring the annotation parameter is a valid subclass of Annotation

Modify getFieldsAnnotatedWithInstance to handle the case where the annotation
parameter is not a subclass of Annotation, to avoid a potential ClassCastException.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/context/Reflection.java [36]

-.filter(field -> field.isAnnotationPresent(annotation.asSubclass(Annotation.class)))
+.filter(field -> Annotation.class.isAssignableFrom(annotation) && field.isAnnotationPresent(annotation.asSubclass(Annotation.class)))
Suggestion importance[1-10]: 7

Why: The suggestion accurately addresses a potential ClassCastException by checking if the annotation parameter is a valid subclass of Annotation before casting, thus improving the method's safety.

7
General
Improve stability by handling potential injection failures gracefully

Add error handling for the injectIntoNonSingleton method in the constructor to
manage potential injection failures gracefully.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/external/hapi/HapiFhirResource.java [16]

-ApplicationContext.injectIntoNonSingleton(this);
+try {
+    ApplicationContext.injectIntoNonSingleton(this);
+} catch (Exception e) {
+    logger.error("Injection failed", e);
+}
Suggestion importance[1-10]: 6

Why: This suggestion proposes adding error handling around the injection process to manage failures more gracefully, which is a good practice for improving the reliability of the application.

6
Provide clearer diagnostics by handling null implementations more informatively

Add logging or throw a more specific exception when fieldImplementation is null, to
provide clearer diagnostics when injection fails.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/context/ApplicationContext.java [76-77]

 if (fieldImplementation == null) {
-    return;
+    throw new IllegalStateException("No implementation found for " + fieldType);
 }
Suggestion importance[1-10]: 5

Why: The suggestion to throw a more specific exception when an implementation is not found is beneficial for debugging and understanding injection failures, although it could be improved by also including logging before throwing the exception.

5

@luis-pabon-tf luis-pabon-tf marked this pull request as ready for review December 9, 2024 18:24
in HapiFhirResource
and deleted file HapifhirResourceTest.groovy
@jorg3lopez jorg3lopez merged commit e718efd into main Dec 13, 2024
17 checks passed
@jorg3lopez jorg3lopez deleted the use-inject-for-non-singleton-class branch December 13, 2024 15:26
jorg3lopez added a commit that referenced this pull request Dec 18, 2024
* comment to start branch

* getFieldsAnnotatedWithInstance helper method

* todo comment deleted

* injectIntoField overloaded method

* injectIntoNonSingleton

* access modifier protected to public

* revert access modifier change

* changes:
added logger to HapiFhirResource in order to test a non-singleton class
created HapiFhirResourceTest in order to test the changes to HapiFhirResource

* Added implementors unit test to ApplicationContextTest

* Minor refactoring on some AppContext injection methods

* Make the skip flag on TestApplicationContext switchable from the test classes

* Update ApplicationContext.java

Remove unreachable if statement

* Update TestApplicationContext.groovy

Add additional line in coverage

* Update TestApplicationContext.groovy

Remove parameter from injectRegisteredImplementations

* added unit tests to cover new code

* Update ApplicationContextTest.groovy

Added test case for unsupported injection classes

* Update ApplicationContext.java

Print an error message if the class implementation is not found

* use return instead of System.err

* refactoring - dry

* thown(NullPointerException) -> noExceptionThrown()

* added comments to indicate changes that will be deleted

* deleted commented out code

* reinstated comments for test case

* deleted test changes:
in HapiFhirResource
and deleted file HapifhirResourceTest.groovy

---------

Co-authored-by: Luis Pabon <turkeyfried@gmail.com>
Co-authored-by: Luis Pabon <luis.pabon@agile6.com>
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.

3 participants