Application context fix unreachable code

[luis-pabon-tf]

Description

This PR is refactoring work for issue #1361, to be merged into a different branch for the same issue.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1361 - Partially compliant Fully compliant requirements: Allow non-singleton classes to use the @Inject annotation. Not compliant requirements: Create a method in the ApplicationContext that takes in the non-singleton object with @Inject annotations. The ApplicationContext should scan this class and inject the instances for the objects using @Inject.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Removed Null Check The removal of the null check on declaringClassImplementation may introduce null pointer exceptions if the implementation is not found.

[github-actions]

Consider reintroducing the parameter for skipping missing implementations to maintain flexibility in testing scenarios. [important]

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Prevent potential NullPointerExceptions by checking if declaringClassImplementation is null Reintroduce the null check for declaringClassImplementation before proceeding with operations on it to prevent potential NullPointerExceptions. shared/src/main/java/gov/hhs/cdc/trustedintermediary/context/ApplicationContext.java [109-112] Object declaringClassImplementation = getDeclaringClassImplementation(declaringClassesToTry); +if (declaringClassImplementation == null) { + return; +} field.trySetAccessible(); Suggestion importance[1-10]: 8 Why: Reintroducing the null check for declaringClassImplementation is crucial to avoid runtime exceptions, specifically NullPointerExceptions, which could occur if the object is null when accessed later. This suggestion directly addresses a potential critical issue removed in the PR.	8
	Ensure proper scoping of the skipMissingImplementations variable to avoid side effects Ensure that the skipMissingImplementations variable is properly scoped or managed to avoid unintended side effects, as it appears to be set without any conditional or encapsulation logic. shared/src/testFixtures/groovy/gov/hhs/cdc/trustedintermediary/context/TestApplicationContext.groovy [23-24] -skipMissingImplementations = true +def skipMissingImplementations = true ApplicationContext.injectRegisteredImplementations() Suggestion importance[1-10]: 7 Why: The suggestion to scope the skipMissingImplementations variable locally is valid as it prevents potential side effects in the global scope. This change enhances the maintainability and clarity of the code.	7

[halprin]

Am I reviewing this too soon?

declaringClassImplementation can be null. Looking at the getDeclaringClassImplementation method, it can return null when declaringClassImplementation == null and skipMissingImplementations == true.

So, I would keep this if statement.

[luis-pabon-tf]

I can revert this change, though I really could not find a way it would ever hit. This is the word salad I wrote on my notes while trying to make it happen:

* getImplementation only throws an exception if the expected clazz is not in OBJECT_MAP

* getDeclaringClassImplementation only marks an implementation as null if getImplementation throws an exception

* injectIntoField only reaches getDeclaringClassImplementation if IMPLEMENTATIONS contains the declaring class for the field

* IMPLEMENTATIONS and OBJECT_MAP are both filled at the same time, when a class implementation is registered

[jorg3lopez]

Good catch. After looking into the code with @luis-pabon-tf, we agreed that this if/else statement could never be null. This is because there is another check before, that checks if the implementing class is in the IMPLEMENTATIONS map. If it is not, then it returns.

In addition, all the classes will have an interface that they are implementing, because they are singleton classes. If they don't, then they wouldn't go through this method, they would go through the non-singleton flow.

[halprin]

OK, I took another look at this. There is a very small corner case for that if statement. Without the if statement, the following field.set(declaringClassImplementation, fieldImplementation); code will throw a NullPointerException.

This corner case is if you do something like the following in a unit test...

class ApplicationContextTest extends Specification {

    interface AFieldInterface {
        Object getAField()
    }

    class InjectionDeclaringClass implements AFieldInterface {
        @Inject
        private String aField

        def getAField() {
            return aField
        }
    }

    def setup() {
        TestApplicationContext.reset()
    }

    def "test the if statement"() {
        given:

        def injectedValue = "DogCow"
        def injectionInstantiation = new InjectionDeclaringClass()

        TestApplicationContext.register(List.class, injectionInstantiation)
        // notice above that I'm registering the injectionInstantiation object as a List class.
        // injectionInstantiation is of class InjectionDeclaringClass, and InjectionDeclaringClass doesn't implement List (it only implements AFieldInterface).
        TestApplicationContext.register(String.class, injectedValue)

        TestApplicationContext.injectRegisteredImplementations()

        when:
        def aFieldValue = injectionInstantiation.getAField()

        then:
        injectedValue == aFieldValue
    }
}

Run that unit test and the test fails with a thrown NullPointerException from the TestApplicationContext.injectRegisteredImplementations() line if the if statement is removed. If the if statement was to remain, an exception wouldn't be thrown, and instead the test fails from the injectedValue == aFieldValue assertion failing.

So, basically to run into this corner case, you need to incorrectly register some implementation as some completely unrelated class. E.g. the injectionInstantiation as List instead of InjectionDeclaringClass or AFieldInterface.

I'm comfortable with allowing the removal of the if statement because any unit tests will fail regardless and you have to really mess up registering of implementations.

[luis-pabon-tf]

Thanks for hunting down how to trigger that statement. We'd been having a lot of trouble doing so. I am adding your test case example with just a few tweaks into the file.