Skip to content

Commit 12c0ad3

Browse files
jaeoptjaeopt
authored
[FSSDK-12111] update cmab error handling (#522)
1 parent 2360466 commit 12c0ad3

File tree

6 files changed

+212
-24
lines changed

6 files changed

+212
-24
lines changed

‎.github/workflows/android.yml‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,18 @@ jobs:
8181
strategy:
8282
fail-fast: false
8383
matrix:
84-
api-level: [21, 25, 26, 29]
84+
# 21 is too old (not properly supported by github)
85+
# mockito version is not working in 30+
86+
include:
87+
- api-level: 25
88+
arch: x86
89+
target: google_apis
90+
- api-level: 27
91+
arch: x86
92+
target: google_apis
93+
- api-level: 29
94+
arch: x86_64
95+
target: default
8596
steps:
8697
- name: checkout
8798
uses: actions/checkout@v4
@@ -113,14 +124,15 @@ jobs:
113124
~/.android/avd/*
114125
~/.android/adb*
115126
~/.android/debug.keystore
116-
key: avd-${{ matrix.api-level }}
127+
key: avd-${{ matrix.api-level }}-${{ matrix.arch }}-${{ matrix.target }}
117128

118129
- name: create AVD and generate snapshot for caching
119130
if: steps.avd-cache.outputs.cache-hit != 'true'
120131
uses: reactivecircus/android-emulator-runner@v2
121132
with:
122133
api-level: ${{ matrix.api-level }}
123-
# arch: arm64-v8a # Specify ARM architecture
134+
arch: ${{ matrix.arch }}
135+
target: ${{ matrix.target }}
124136
force-avd-creation: false
125137
emulator-options: -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
126138
disable-animations: false
@@ -130,6 +142,8 @@ jobs:
130142
uses: reactivecircus/android-emulator-runner@v2
131143
with:
132144
api-level: ${{ matrix.api-level }}
145+
arch: ${{ matrix.arch }}
146+
target: ${{ matrix.target }}
133147
force-avd-creation: false
134148
emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none
135149
disable-animations: true

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
/build
1111
/captures
1212
libs/
13+
fsc-test-libs/
1314
values.gradle
1415
jacoco.exec
1516
.vscode/
17+
.settings/
1618

‎android-sdk/build.gradle‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ dependencies {
7373
api project(':user-profile')
7474
api project(':odp')
7575
// Enable using a local core-api jar for testing when the 'useLocalJars' property is specified
76-
if (project.hasProperty('useLocalJars') && file('../libs/core-api.jar').exists()) {
77-
api files('../libs/core-api.jar')
76+
if (project.hasProperty('useLocalJars') && file('../fsc-test-libs/core-api.jar').exists()) {
77+
api files('../fsc-test-libs/core-api.jar')
7878
} else {
7979
api ("com.optimizely.ab:core-api:$java_core_ver") {
8080
exclude group: 'com.google.code.findbugs'

‎android-sdk/src/main/java/com/optimizely/ab/android/sdk/cmab/DefaultCmabClient.kt‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,12 @@ open class DefaultCmabClient : CmabClient {
107107
logger.error(errorMessage)
108108
throw CmabFetchException(errorMessage)
109109
}
110+
} catch (e: CmabInvalidResponseException) {
111+
// Propagate validation exceptions as-is
112+
throw e
113+
} catch (e: CmabFetchException) {
114+
// Propagate fetch exceptions as-is
115+
throw e
110116
} catch (e: Exception) {
111117
logger.debug("Failed to fetch CMAB decision for ruleId={} and userId={}", ruleId, userId);
112118
val errorMessage: String =
@@ -123,7 +129,17 @@ open class DefaultCmabClient : CmabClient {
123129
}
124130
}
125131
}
126-
return client.execute(request, REQUEST_BACKOFF_TIMEOUT, REQUEST_RETRIES_POWER)
132+
val result = client.execute(request, REQUEST_BACKOFF_TIMEOUT, REQUEST_RETRIES_POWER)
133+
134+
// CmabService (in java-sdk core) expects exceptions on cmab errors (so it'll convert to Error Decision)
135+
// Null result means CMAB fetch failed, so convert to exceptions here.
136+
if (result == null) {
137+
val errorMessage = String.format(cmabClientHelper.cmabFetchFailed, "Client returned null - request failed")
138+
logger.error(errorMessage)
139+
throw CmabFetchException(errorMessage)
140+
}
141+
142+
return result
127143
}
128144

129145
companion object {

‎android-sdk/src/test/java/com/optimizely/ab/android/sdk/cmab/DefaultCmabClientTest.java‎

Lines changed: 172 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package com.optimizely.ab.android.sdk.cmab;
1616

1717
import com.optimizely.ab.android.shared.Client;
18+
import com.optimizely.ab.cmab.client.CmabFetchException;
19+
import com.optimizely.ab.cmab.client.CmabInvalidResponseException;
1820
import org.junit.Before;
1921
import org.junit.Test;
2022
import org.junit.runner.RunWith;
@@ -88,15 +90,20 @@ public void testConstructorWithContextAndHelper() {
8890
}
8991

9092
@Test
91-
public void testFetchDecisionSuccess() throws Exception {
93+
public void testFetchDecisionSuccess() {
9294
HttpURLConnection mockUrlConnection = mock(HttpURLConnection.class);
9395
ByteArrayOutputStream mockOutputStream = mock(ByteArrayOutputStream.class);
9496

95-
String mockResponseJson = "{\"variation_id\":\"variation_1\",\"status\":\"success\"}";
96-
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
97-
when(mockUrlConnection.getResponseCode()).thenReturn(200);
98-
when(mockClient.readStream(mockUrlConnection)).thenReturn(mockResponseJson);
99-
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
97+
try {
98+
String mockResponseJson = "{\"variation_id\":\"variation_1\",\"status\":\"success\"}";
99+
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
100+
when(mockUrlConnection.getResponseCode()).thenReturn(200);
101+
when(mockClient.readStream(mockUrlConnection)).thenReturn(mockResponseJson);
102+
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
103+
} catch (IOException e) {
104+
// Never happens with mocked connection
105+
}
106+
100107
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
101108
Client.Request<String> request = invocation.getArgument(0);
102109
return request.execute();
@@ -120,13 +127,18 @@ public void testFetchDecisionSuccess() throws Exception {
120127

121128
verify(mockUrlConnection).setConnectTimeout(10*1000);
122129
verify(mockUrlConnection).setReadTimeout(60*1000);
123-
verify(mockUrlConnection).setRequestMethod("POST");
130+
try {
131+
verify(mockUrlConnection).setRequestMethod("POST");
132+
} catch (Exception e) {
133+
// Never happens - verify() doesn't actually invoke the method
134+
}
124135
verify(mockUrlConnection).setRequestProperty("content-type", "application/json");
125136
verify(mockUrlConnection).setDoOutput(true);
126137
}
127138

128-
@Test
129-
public void testFetchDecisionConnectionFailure() throws Exception {
139+
@Test(expected = CmabFetchException.class)
140+
public void testFetchDecisionConnectionFailure() {
141+
// When openConnection returns null, should throw CmabFetchException
130142
when(mockClient.openConnection(any(URL.class))).thenReturn(null);
131143
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
132144
Client.Request<String> request = invocation.getArgument(0);
@@ -135,20 +147,164 @@ public void testFetchDecisionConnectionFailure() throws Exception {
135147

136148
mockCmabClient = new DefaultCmabClient(mockClient, mockCmabClientHelper);
137149

138-
String result = mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
139-
assertNull(result);
150+
// Should throw CmabFetchException when connection fails to open
151+
mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
152+
}
153+
154+
@Test(expected = CmabFetchException.class)
155+
public void testFetchDecisionThrowsExceptionOn500Error() {
156+
HttpURLConnection mockUrlConnection = mock(HttpURLConnection.class);
157+
ByteArrayOutputStream mockOutputStream = mock(ByteArrayOutputStream.class);
158+
159+
try {
160+
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
161+
when(mockUrlConnection.getResponseCode()).thenReturn(500);
162+
when(mockUrlConnection.getResponseMessage()).thenReturn("Internal Server Error");
163+
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
164+
} catch (IOException e) {
165+
// Never happens with mocked connection
166+
}
167+
168+
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
169+
Client.Request<String> request = invocation.getArgument(0);
170+
return request.execute();
171+
});
172+
173+
doReturn("{\"user_id\":\"test-user-456\"}")
174+
.when(mockCmabClientHelper)
175+
.buildRequestJson(any(), any(), any(), any());
176+
177+
mockCmabClient = new DefaultCmabClient(mockClient, mockCmabClientHelper);
178+
179+
// Should throw CmabFetchException for 500 error
180+
mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
181+
}
182+
183+
@Test(expected = CmabFetchException.class)
184+
public void testFetchDecisionThrowsExceptionOn400Error() {
185+
HttpURLConnection mockUrlConnection = mock(HttpURLConnection.class);
186+
ByteArrayOutputStream mockOutputStream = mock(ByteArrayOutputStream.class);
187+
188+
try {
189+
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
190+
when(mockUrlConnection.getResponseCode()).thenReturn(400);
191+
when(mockUrlConnection.getResponseMessage()).thenReturn("Bad Request");
192+
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
193+
} catch (IOException e) {
194+
// Never happens with mocked connection
195+
}
196+
197+
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
198+
Client.Request<String> request = invocation.getArgument(0);
199+
return request.execute();
200+
});
201+
202+
doReturn("{\"user_id\":\"test-user-456\"}")
203+
.when(mockCmabClientHelper)
204+
.buildRequestJson(any(), any(), any(), any());
205+
206+
mockCmabClient = new DefaultCmabClient(mockClient, mockCmabClientHelper);
207+
208+
// Should throw CmabFetchException for 400 error
209+
mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
210+
}
211+
212+
@Test(expected = CmabFetchException.class)
213+
public void testFetchDecisionThrowsExceptionOnNetworkError() {
214+
HttpURLConnection mockUrlConnection = mock(HttpURLConnection.class);
215+
ByteArrayOutputStream mockOutputStream = mock(ByteArrayOutputStream.class);
216+
217+
try {
218+
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
219+
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
220+
doThrow(new IOException("Network error")).when(mockUrlConnection).getResponseCode();
221+
} catch (IOException e) {
222+
// Never happens with mocked connection
223+
}
224+
225+
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
226+
Client.Request<String> request = invocation.getArgument(0);
227+
return request.execute();
228+
});
229+
230+
doReturn("{\"user_id\":\"test-user-456\"}")
231+
.when(mockCmabClientHelper)
232+
.buildRequestJson(any(), any(), any(), any());
233+
234+
mockCmabClient = new DefaultCmabClient(mockClient, mockCmabClientHelper);
235+
236+
// Should throw CmabFetchException when network IOException occurs
237+
mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
238+
}
239+
240+
@Test(expected = CmabInvalidResponseException.class)
241+
public void testFetchDecisionThrowsExceptionOnInvalidJson() {
242+
HttpURLConnection mockUrlConnection = mock(HttpURLConnection.class);
243+
ByteArrayOutputStream mockOutputStream = mock(ByteArrayOutputStream.class);
244+
245+
try {
246+
String invalidResponseJson = "{\"invalid\":\"response\"}";
247+
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
248+
when(mockUrlConnection.getResponseCode()).thenReturn(200);
249+
when(mockClient.readStream(mockUrlConnection)).thenReturn(invalidResponseJson);
250+
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
251+
} catch (IOException e) {
252+
// Never happens with mocked connection
253+
}
254+
255+
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
256+
Client.Request<String> request = invocation.getArgument(0);
257+
return request.execute();
258+
});
259+
260+
doReturn("{\"user_id\":\"test-user-456\"}")
261+
.when(mockCmabClientHelper)
262+
.buildRequestJson(any(), any(), any(), any());
263+
doReturn(false) // Invalid response
264+
.when(mockCmabClientHelper)
265+
.validateResponse(any());
266+
267+
mockCmabClient = new DefaultCmabClient(mockClient, mockCmabClientHelper);
268+
269+
// Should throw CmabInvalidResponseException when response validation fails
270+
mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
140271
}
141272

142273
@Test
143-
public void testRetryOnFailureWithRetryBackoff() throws Exception {
144-
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenReturn(null);
274+
public void testRetryConfigurationPassedToClient() {
275+
HttpURLConnection mockUrlConnection = mock(HttpURLConnection.class);
276+
ByteArrayOutputStream mockOutputStream = mock(ByteArrayOutputStream.class);
277+
278+
try {
279+
String mockResponseJson = "{\"variation_id\":\"variation_1\",\"status\":\"success\"}";
280+
when(mockClient.openConnection(any(URL.class))).thenReturn(mockUrlConnection);
281+
when(mockUrlConnection.getResponseCode()).thenReturn(200);
282+
when(mockClient.readStream(mockUrlConnection)).thenReturn(mockResponseJson);
283+
when(mockUrlConnection.getOutputStream()).thenReturn(mockOutputStream);
284+
} catch (IOException e) {
285+
// Never happens with mocked connection
286+
}
287+
288+
when(mockClient.execute(any(Client.Request.class), anyInt(), anyInt())).thenAnswer(invocation -> {
289+
Client.Request<String> request = invocation.getArgument(0);
290+
return request.execute();
291+
});
292+
293+
doReturn("{\"user_id\":\"test-user-456\"}")
294+
.when(mockCmabClientHelper)
295+
.buildRequestJson(any(), any(), any(), any());
296+
doReturn(true)
297+
.when(mockCmabClientHelper)
298+
.validateResponse(any());
299+
doReturn("variation_1")
300+
.when(mockCmabClientHelper)
301+
.parseVariationId(any());
145302

146303
mockCmabClient = new DefaultCmabClient(mockClient, mockCmabClientHelper);
147304

148-
String result = mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
149-
assertNull(result);
305+
mockCmabClient.fetchDecision(testRuleId, testUserId, testAttributes, testCmabUuid);
150306

151-
// Verify the retry configuration matches our constants
307+
// Verify the retry configuration is passed to client.execute()
152308
verify(mockClient).execute(any(Client.Request.class), eq(DefaultCmabClient.REQUEST_BACKOFF_TIMEOUT), eq(DefaultCmabClient.REQUEST_RETRIES_POWER));
153309
assertEquals("REQUEST_BACKOFF_TIMEOUT should be 1", 1, DefaultCmabClient.REQUEST_BACKOFF_TIMEOUT);
154310
assertEquals("REQUEST_RETRIES_POWER should be 1", 1, DefaultCmabClient.REQUEST_RETRIES_POWER);

‎shared/build.gradle‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ android {
4646

4747
dependencies {
4848
// Enable using a local core-api jar for testing when the 'useLocalJars' property is specified
49-
if (project.hasProperty('useLocalJars') && file('../libs/core-api.jar').exists()) {
50-
api files('../libs/core-api.jar')
49+
if (project.hasProperty('useLocalJars') && file('../fsc-test-libs/core-api.jar').exists()) {
50+
api files('../fsc-test-libs/core-api.jar')
5151
} else {
5252
api ("com.optimizely.ab:core-api:$java_core_ver") {
5353
exclude group: 'com.google.code.findbugs'

0 commit comments

Comments
 (0)