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

SWC-7044 - Handle error when fetching feature flag configuration #5587

Merged
merged 1 commit into from
Nov 22, 2024
Merged

SWC-7044 - Handle error when fetching feature flag configuration #5587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 39 additions & 16 deletions src/main/java/org/sagebionetworks/web/server/servlet/AppConfigServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package org.sagebionetworks.web.server.servlet;

import com.amazonaws.services.appconfigdata.AWSAppConfigData;
import com.amazonaws.services.appconfigdata.model.BadRequestException;
import com.amazonaws.services.appconfigdata.model.GetLatestConfigurationRequest;
import com.amazonaws.services.appconfigdata.model.GetLatestConfigurationResult;
import com.amazonaws.services.appconfigdata.model.InternalServerException;
import com.amazonaws.services.appconfigdata.model.StartConfigurationSessionRequest;
import com.amazonaws.services.appconfigdata.model.StartConfigurationSessionResult;
import com.google.gwt.thirdparty.guava.common.base.Supplier;
Expand Down Expand Up @@ -95,11 +97,31 @@ public void initializeConfigSupplier() {
);
}

public JSONObjectAdapter getLastConfigValueOrDefault() {
if (lastConfigValue != null) {
return lastConfigValue;
}
try {
return new JSONObjectAdapterImpl(DEFAULT_CONFIG_VALUE);
} catch (JSONObjectAdapterException e) {
logger.log(
Level.SEVERE,
"JSONObjectAdapterException occurred in default configuration",
e
);
throw new RuntimeException(e);
Copy link
Contributor Author

@nickgros nickgros Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we can't serialize a constant string "{}", we have bigger problems

}
}

public JSONObjectAdapter getLatestConfiguration() {
try {
if (configurationToken == null) {
logger.log(Level.SEVERE, "returning default config");
return new JSONObjectAdapterImpl(DEFAULT_CONFIG_VALUE);
logger.log(
Level.SEVERE,
"The configuration token is null, the last config value will be returned." +
" This usually means that the initial call to initialize the configuration session failed."
);
return getLastConfigValueOrDefault();
}
Comment on lines 118 to 125
Copy link
Contributor Author

@nickgros nickgros Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like this error case will only happen when initialization failed, so I updated the log message to give context.

GetLatestConfigurationRequest latestConfigRequest =
new GetLatestConfigurationRequest()
Expand All @@ -116,21 +138,22 @@ public JSONObjectAdapter getLatestConfiguration() {
if (!newConfigString.isEmpty()) {
lastConfigValue = new JSONObjectAdapterImpl(newConfigString);
}
} catch (BadRequestException | InternalServerException e) {
// Invalid token or server error, re-initialize the session to try to recover.
logger.log(
Level.SEVERE,
"Failed to get latest configuration, returning last or default configuration and attempting to re-initialize the session.",
e
);
initializeAppConfigClient();
return getLastConfigValueOrDefault();
Comment on lines +141 to +149
Copy link
Contributor Author

@nickgros nickgros Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the logs, a BadRequestException was thrown with message "Token not valid". The tokens are one-time-use and my hypothesis is that we probably used a token more than once (maybe some network failure occurred and we didn't get the response containing a new token). Attempt to recover by re-initializing the session and getting a fresh token.

} catch (Exception e) {
try {
logger.log(
Level.SEVERE,
"Failed to get or parse latest configuration, returning default configuration",
e
);
return new JSONObjectAdapterImpl(DEFAULT_CONFIG_VALUE);
} catch (JSONObjectAdapterException exception) {
logger.log(
Level.SEVERE,
"JSONObjectAdapterException occurred in default configuration",
exception
);
}
logger.log(
Level.SEVERE,
"Failed to get or parse latest configuration, returning last or default configuration.",
e
);
return getLastConfigValueOrDefault();
}
return lastConfigValue;
}
Expand Down
87 changes: 83 additions & 4 deletions src/test/java/org/sagebionetworks/web/unitserver/servlet/AppConfigServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.mockito.Mockito.*;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.amazonaws.services.appconfigdata.AWSAppConfigData;
import com.amazonaws.services.appconfigdata.model.BadRequestException;
import com.amazonaws.services.appconfigdata.model.GetLatestConfigurationRequest;
import com.amazonaws.services.appconfigdata.model.GetLatestConfigurationResult;
import com.amazonaws.services.appconfigdata.model.StartConfigurationSessionRequest;
Expand All @@ -20,7 +25,6 @@
import org.mockito.MockitoAnnotations;
import org.sagebionetworks.StackConfiguration;
import org.sagebionetworks.schema.adapter.JSONObjectAdapter;
import org.sagebionetworks.schema.adapter.JSONObjectAdapterException;
import org.sagebionetworks.schema.adapter.org.json.JSONObjectAdapterImpl;
import org.sagebionetworks.web.server.servlet.AppConfigServlet;

Expand Down Expand Up @@ -134,7 +138,7 @@ public void testGetLatestConfiguration_Success() {
}

@Test
public void testGetLatestConfiguration_Failure() {
public void testGetLatestConfiguration_Failure_ReturnDefaultValueWhenNoLastValue() {
String DEFAULT_CONFIG_VALUE = "{}";
when(
mockAppConfigDataClient.getLatestConfiguration(
Expand All @@ -143,12 +147,87 @@ public void testGetLatestConfiguration_Failure() {
)
.thenThrow(new RuntimeException("Failed to retrieve configuration"));

servlet.configurationToken = "mockToken"; // Setting the initial configuration token
JSONObjectAdapter configValue = servlet.getLatestConfiguration();

assertEquals(DEFAULT_CONFIG_VALUE, configValue.toString());
}

@Test
public void testGetLatestConfiguration_Failure_ReturnLastValue() {
// Set the initial configuration token
servlet.configurationToken = "mockToken";

// Set up a successful response
ByteBuffer mockByteBuffer = ByteBuffer
.wrap("{\"test configuration\":true}".getBytes())
.asReadOnlyBuffer();
GetLatestConfigurationResult mockConfigResult =
new GetLatestConfigurationResult()
.withConfiguration(mockByteBuffer)
.withNextPollConfigurationToken("new-mock-token");

when(
mockAppConfigDataClient.getLatestConfiguration(
any(GetLatestConfigurationRequest.class)
)
)
.thenReturn(mockConfigResult);

// Initial call succeeds
servlet.getLatestConfiguration();

// Next call will fail, but should return the old value
when(
mockAppConfigDataClient.getLatestConfiguration(
any(GetLatestConfigurationRequest.class)
)
)
.thenThrow(new RuntimeException("Failed to retrieve configuration"));

JSONObjectAdapter configValue = servlet.getLatestConfiguration();

assertEquals(mockConfiguration.toString(), configValue.toString());

// Do not attempt to re-initialize the client
verify(mockAppConfigDataClient, never()).startConfigurationSession(any());

// Try again, but with a recoverable exception
when(
mockAppConfigDataClient.getLatestConfiguration(
any(GetLatestConfigurationRequest.class)
)
)
.thenThrow(new BadRequestException("Token is invalid"));

configValue = servlet.getLatestConfiguration();

assertEquals(mockConfiguration.toString(), configValue.toString());

// We should attempt to re-initialize the client
verify(mockAppConfigDataClient).startConfigurationSession(any());
}

@Test
public void testGetLatestConfiguration_Failure_NullToken() {
String DEFAULT_CONFIG_VALUE = "{}";
servlet.configurationToken = null;

when(
mockAppConfigDataClient.getLatestConfiguration(
any(GetLatestConfigurationRequest.class)
)
)
.thenThrow(new RuntimeException("Failed to retrieve configuration"));

JSONObjectAdapter configValue = servlet.getLatestConfiguration();

assertEquals(DEFAULT_CONFIG_VALUE, configValue.toString());
// Verify we never called getLatestConfiguration
verify(mockAppConfigDataClient, never()).getLatestConfiguration(any());
// Verify we do not try to reinitialize the client
verify(mockAppConfigDataClient, never()).startConfigurationSession(any());
}

@Test
public void testInitializeAppConfigClient() {
servlet.appConfigDataClient = null; // Simulate the client not being injected
Expand Down