fix: Local evaluation silently drops identity overrides on large environments by marc-merino · Pull Request #236 · Flagsmith/flagsmith-java-client · GitHub
Skip to content

fix: Local evaluation silently drops identity overrides on large environments #236

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

Open
marc-merino wants to merge 1 commit into
base: main
Choose a base branch
from
+277 −9
Open

fix: Local evaluation silently drops identity overrides on large environments #236

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
86 changes: 80 additions & 6 deletions src/main/java/com/flagsmith/FlagsmithApiWrapper.java
View file
Open in desktop
12 changes: 12 additions & 0 deletions src/main/java/com/flagsmith/FlagsmithLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ public void error(String var1, Object... var2) {
}
}

/**
* Prints warning into the log.
*
* @param var1 a param to print
* @param var2 a param to print
*/
public void warn(String var1, Object... var2) {
if (isLoggingEnabled(FlagsmithLoggerLevel.ERROR)) {
logger.warn("Flagsmith: " + var1, var2);
}
}

/**
* Prints info into the log.
*
Expand Down
46 changes: 43 additions & 3 deletions src/main/java/com/flagsmith/threads/RequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.flagsmith.FlagsmithLogger;
import com.flagsmith.MapperFactory;
import com.flagsmith.config.Retry;
Expand Down Expand Up @@ -96,6 +95,37 @@ public <T> Future<T> executeAsync(
*/
public <T> CompletableFuture<T> submit(
Request request, TypeReference<T> clazz, Boolean doThrow, Retry retries) {
return submit(request,
response -> MapperFactory.getMapper().readValue(response.body().string(), clazz),
doThrow, retries);
}

/**
* Execute the request in async mode, building the result from the successful response.
*
* @param request request to invoke
* @param reader reads a successful response into the result
* @param doThrow should throw Exception (boolean)
* @param <T> Type inference for the response
*/
public <T> CompletableFuture<T> submit(
Request request, ResponseReader<T> reader, Boolean doThrow) {
return submit(request, reader, doThrow, retries);
}

/**
* Execute the request in async mode, building the result from the successful response.
*
* @param request Request object
* @param reader reads a successful response into the result
* @param doThrow should throw Exception
* @param retries no of retries before failing
* @param <T> Type inference for the response
* @return a future completed with the reader's result, or null when the call failed and
* doThrow is false
*/
public <T> CompletableFuture<T> submit(
Request request, ResponseReader<T> reader, Boolean doThrow, Retry retries) {
CompletableFuture<T> completableFuture = new CompletableFuture<>();
Retry localRetry = retries.toBuilder().build();
// run the execute method in a fixed thread with retries.
Expand All @@ -111,8 +141,7 @@ public <T> CompletableFuture<T> submit(
try (Response response = call.execute()) {
statusCode = response.code();
if (response.isSuccessful()) {
ObjectMapper mapper = MapperFactory.getMapper();
completableFuture.complete(mapper.readValue(response.body().string(), clazz));
completableFuture.complete(reader.read(response));
// break the while
break;

Expand Down Expand Up @@ -141,6 +170,17 @@ public <T> CompletableFuture<T> submit(
return completableFuture;
}

/**
* Reads a successful response into a value, with access to the response headers.
*
* @param <T> the result type
*/
@FunctionalInterface
public interface ResponseReader<T> {

T read(Response response) throws IOException;
}

public void close() {
this.executor.shutdown();
}
Expand Down
56 changes: 56 additions & 0 deletions src/test/java/com/flagsmith/FlagsmithApiWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,17 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.flagsmith.config.FlagsmithConfig;
import com.flagsmith.config.Retry;
import com.flagsmith.flagengine.EvaluationContext;
import com.flagsmith.models.BaseFlag;
import com.flagsmith.models.features.FeatureStateModel;
import com.flagsmith.models.features.FeatureModel;
Expand All @@ -40,6 +43,7 @@
import okhttp3.mock.MockInterceptor;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;


public class FlagsmithApiWrapperTest {
Expand Down Expand Up @@ -168,6 +172,58 @@ public void identifyUserWithTraits_fail() {
verify(flagsmithLogger, times(0)).httpError(any(), any(IOException.class), anyBoolean());
}

@Test
public void getEvaluationContext_singlePage_sendsOneRequest() throws JsonProcessingException {
// Arrange
interceptor.addRule()
.get(BASE_URL + "/environment-document/")
.respond(FlagsmithTestHelper.environmentString(), MEDIATYPE_JSON);

// Act
EvaluationContext context = sut.getEvaluationContext();

// Assert
assertNotNull(context);
verify(flagsmithLogger, times(0)).httpError(any(), any(Response.class), anyBoolean());
}

@Test
public void getEvaluationContext_warnsWhenFetchTakesLongerThanRefreshInterval()
throws JsonProcessingException {
// Arrange
defaultConfig = FlagsmithConfig.newBuilder()
.addHttpInterceptor(interceptor)
.retries(new Retry(1))
.baseUri(BASE_URL)
.withEnvironmentRefreshIntervalSeconds(0)
.build();
sut = new FlagsmithApiWrapper(defaultConfig, null, flagsmithLogger, API_KEY);

interceptor.addRule()
.get(BASE_URL + "/environment-document/")
.respond(FlagsmithTestHelper.environmentString(), MEDIATYPE_JSON);

// Act
sut.getEvaluationContext();

// Assert
ArgumentCaptor<String> message = ArgumentCaptor.forClass(String.class);
verify(flagsmithLogger).warn(message.capture(), anyString(), eq(0));
assertTrue(message.getValue().contains("Fetching the environment document took"));
assertTrue(message.getValue().contains("environment refresh interval of"));
}

@Test
public void extractNextPageId_decodesPageIdFromNextLink() {
assertNull(sut.extractNextPageId(null));
assertNull(sut.extractNextPageId("</environment-document/>; rel=\"next\""));
assertNull(sut.extractNextPageId("</environment-document/?page_id=x>; rel=\"prev\""));
assertEquals(
"identity_override:1:00000000-0000-0000-0000-000000000001",
sut.extractNextPageId("</environment-document/?page_id=identity_override%3A1%3A"
+ "00000000-0000-0000-0000-000000000001>; rel=\"next\""));
}

@Test
public void testClose_ClosesRequestProcessor() {
// Given
Expand Down
86 changes: 86 additions & 0 deletions src/test/java/com/flagsmith/FlagsmithClientTest.java
View file
Open in desktop