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.

Sign up for GitHub

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

Application context fix unreachable code #1639

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Application context fix unreachable code #1639

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff4781a
Update ApplicationContext.java
luis-pabon-tf Dec 3, 2024
e5d184d
Update TestApplicationContext.groovy
luis-pabon-tf Dec 3, 2024
083ddb4
Update TestApplicationContext.groovy
luis-pabon-tf Dec 3, 2024
0ac729e
added unit tests to cover new code
jorg3lopez Dec 4, 2024
18c2f48
Update ApplicationContextTest.groovy
luis-pabon-tf Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions shared/src/main/java/gov/hhs/cdc/trustedintermediary/context/ApplicationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,6 @@ private static void injectIntoField(Field field) {

Object declaringClassImplementation =
getDeclaringClassImplementation(declaringClassesToTry);
if (declaringClassImplementation == null) {
return;
}
Comment on lines -111 to -113
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jorg3lopez jorg3lopez Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

image

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


field.trySetAccessible();

Expand Down
6 changes: 3 additions & 3 deletions ...testFixtures/groovy/gov/hhs/cdc/trustedintermediary/context/TestApplicationContext.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ class TestApplicationContext extends ApplicationContext {
TEST_ENV_VARS.clear()
}

def static injectRegisteredImplementations(def skip = true) {
skipMissingImplementations = skip
doInjectRegisteredImplementations()
def static injectRegisteredImplementations() {
skipMissingImplementations = true
ApplicationContext.injectRegisteredImplementations()
Copy link

Choose a reason for hiding this comment

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

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

}

def static addEnvironmentVariable(String key, String value) {
Expand Down
Loading