Fix ApiClient NPE (#1129)

* Fix NPE when using the no-arg constructor of ApiClient (#1127) - Check both httpClient and cacheManager are non-null - Add unit tests - Also cleanup unused fields * fix test --------- Co-authored-by: Clément Denis <clement.denis@gmail.com>
okta · Apr 30, 2024 · a575e9a · a575e9a
1 parent 4097bdb
commit a575e9a
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 14 deletions.
diff --git a/api/src/main/resources/custom_templates/ApiClient.mustache b/api/src/main/resources/custom_templates/ApiClient.mustache
@@ -86,7 +86,7 @@ import java.nio.file.Paths;
 import java.lang.reflect.Type;
 import java.net.URI;
 
-import com.okta.commons.http.config.HttpClientConfiguration;
+import com.okta.sdk.cache.Caches;
 import com.okta.sdk.cache.Cache;
 import com.okta.sdk.cache.CacheManager;
 
@@ -156,10 +156,6 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
 
     private Cache<String, Object> cache;
 
-    private CacheManager cacheManager;
-
-    private HttpClientConfiguration httpClientConfiguration;
-
     private int statusCode;
     private Map<String, List<String>> responseHeaders;
 
@@ -168,7 +164,10 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
         // Methods that can have a request body
         private static List<String> bodyMethods = Arrays.asList("POST", "PUT", "DELETE", "PATCH");
 
-            public ApiClient(CloseableHttpClient httpClient, CacheManager cacheManager, HttpClientConfiguration httpClientConfiguration) {
+            public ApiClient(CloseableHttpClient httpClient, CacheManager cacheManager) {
+            if (httpClient == null) throw new IllegalArgumentException("httpClient cannot be null");
+            if (cacheManager == null) throw new IllegalArgumentException("cacheManager cannot be null");
+
             objectMapper = new ObjectMapper();
             objectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
             objectMapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY);
@@ -201,13 +200,11 @@ protected List<ServerConfiguration> servers = new ArrayList<ServerConfiguration>
 
             this.httpClient = httpClient;
 
-            this.cacheManager = cacheManager;
-            this.httpClientConfiguration = httpClientConfiguration;
             this.cache = cacheManager.getCache("default");
             }
 
             public ApiClient() {
-            this(HttpClients.createDefault(), null, null);
+                this(HttpClients.createDefault(), Caches.newDisabledCacheManager());
             }
 
             public static DateFormat buildDefaultDateFormat() {

diff --git a/impl/src/main/java/com/okta/sdk/impl/client/DefaultClientBuilder.java b/impl/src/main/java/com/okta/sdk/impl/client/DefaultClientBuilder.java
@@ -335,7 +335,7 @@ public ApiClient build() {
             setProxy(httpClientBuilder, clientConfig);
         }
 
-        ApiClient apiClient = new ApiClient(httpClientBuilder.build(), this.cacheManager, this.clientConfig);
+        ApiClient apiClient = new ApiClient(httpClientBuilder.build(), this.cacheManager);
         apiClient.setBasePath(this.clientConfig.getBaseUrl());
 
         String userAgentValue = ApplicationInfo.get().entrySet().stream()

diff --git a/impl/src/test/groovy/com/okta/sdk/impl/client/DefaultClientBuilderTest.groovy b/impl/src/test/groovy/com/okta/sdk/impl/client/DefaultClientBuilderTest.groovy
@@ -17,6 +17,7 @@
 package com.okta.sdk.impl.client
 
 import com.okta.sdk.authc.credentials.TokenClientCredentials
+import com.okta.sdk.cache.Caches
 import com.okta.sdk.client.AuthenticationScheme
 import com.okta.sdk.client.AuthorizationMode
 import com.okta.sdk.client.ClientBuilder
@@ -29,6 +30,9 @@ import com.okta.sdk.impl.oauth2.OAuth2HttpException
 import com.okta.sdk.impl.oauth2.OAuth2TokenRetrieverException
 import com.okta.sdk.impl.test.RestoreEnvironmentVariables
 import com.okta.sdk.impl.test.RestoreSystemProperties
+import com.okta.sdk.resource.client.ApiClient
+import com.okta.sdk.resource.client.Configuration
+import org.apache.hc.client5.http.impl.classic.HttpClients
 import org.mockito.invocation.InvocationOnMock
 import org.mockito.stubbing.Answer
 import org.testng.annotations.Listeners
@@ -82,6 +86,21 @@ class DefaultClientBuilderTest {
         assertTrue(Clients.builder() instanceof DefaultClientBuilder)
     }
 
+    @Test
+    void testDefaultApiClient() {
+        Configuration.getDefaultApiClient()
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    void testIncorrectApiClient_nullHttpClient() {
+        new ApiClient(null, Caches.newDisabledCacheManager())
+    }
+
+    @Test(expectedExceptions = IllegalArgumentException.class)
+    void testIncorrectApiClient_nullCacheManager() {
+        new ApiClient(HttpClients.createDefault(), null)
+    }
+
     @Test
     void testConfigureBaseProperties() {
         clearOktaEnvAndSysProps()

diff --git a/impl/src/test/groovy/com/okta/sdk/impl/oauth2/Oauth2ClientCredentialsTest.groovy b/impl/src/test/groovy/com/okta/sdk/impl/oauth2/Oauth2ClientCredentialsTest.groovy
@@ -15,7 +15,6 @@
  */
 package com.okta.sdk.impl.oauth2
 
-import com.okta.commons.http.config.HttpClientConfiguration
 import com.okta.sdk.cache.CacheManager
 import com.okta.sdk.impl.config.ClientConfiguration
 import org.apache.hc.client5.http.impl.classic.CloseableHttpClient
@@ -68,7 +67,7 @@ class Oauth2ClientCredentialsTest {
 
     @Test
     void testReplaceAuthentication() {
-        def apiClient = new ApiClient(mock(CloseableHttpClient), mock(CacheManager), mock(HttpClientConfiguration))
+        def apiClient = new ApiClient(mock(CloseableHttpClient), mock(CacheManager))
         def accessTokenRetrieverService = new AccessTokenRetrieverServiceImpl(mock(ClientConfiguration), apiClient)
         def credentials = new OAuth2ClientCredentials(accessTokenRetrieverService)
         apiClient.replaceAuthentication("oauth2", credentials)
@@ -78,7 +77,7 @@ class Oauth2ClientCredentialsTest {
 
     @Test
     void testReplaceAuthentication2WrongName() {
-        def apiClient = new ApiClient(mock(CloseableHttpClient), mock(CacheManager), mock(HttpClientConfiguration))
+        def apiClient = new ApiClient(mock(CloseableHttpClient), mock(CacheManager))
         def credentials = new OAuth2ClientCredentials(mock(AccessTokenRetrieverServiceImpl))
         def exception = expectThrows(RuntimeException.class) {
             apiClient.replaceAuthentication("oauth", credentials)
@@ -88,7 +87,7 @@ class Oauth2ClientCredentialsTest {
 
     @Test
     void testReplaceAuthenticationWrongType() {
-        def apiClient = new ApiClient(mock(CloseableHttpClient), mock(CacheManager), mock(HttpClientConfiguration))
+        def apiClient = new ApiClient(mock(CloseableHttpClient), mock(CacheManager))
         def exception = expectThrows(RuntimeException.class) {
             apiClient.replaceAuthentication("oauth2", new ApiKeyAuth("", ""))
         }