Skip to content

Add typed feature flag getters #1713

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

Merged
cathleeny merged 6 commits into from
Oct 6, 2026
Merged

Add typed feature flag getters #1713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7dc740a
Add typed feature flag getters
cathleeny Oct 3, 2026
6fb4af1
Replace feature flag boolean wrapper with typed getter
cathleeny Oct 5, 2026
7cd97a1
Document typed SAFE feature flag getters
cathleeny Oct 5, 2026
7f4a6e5
Preserve boolean feature flag defaults in getBoolean
cathleeny Oct 5, 2026
ff7cd70
Merge branch 'main' into feature/native-feature-flags
cathleeny Oct 5, 2026
a642294
Avoid redundant feature flag string-list copy
cathleeny Oct 5, 2026
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
2 changes: 2 additions & 0 deletions NEXT_CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

### Added

- Added typed getters for all six SAFE feature flag types using the existing cache.

### Updated

- Bumped Apache HttpClient 5 (`httpclient5`) from 5.6.3 to 5.6.4.
Expand Down
5 changes: 2 additions & 3 deletions src/main/java/com/databricks/jdbc/api/impl/DatabricksConnectionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ public DatabricksClientType getClientTypeFromContext() {
}
// Check feature flag to determine if SEA client should be enabled
if (DatabricksDriverFeatureFlagsContextFactory.getInstance(this)
.isFeatureEnabled(SQL_EXEC_FLAG_NAME)) {
.getBoolean(SQL_EXEC_FLAG_NAME)) {
return DatabricksClientType.SEA;
}
// Default to THRIFT if feature flag is not enabled or cannot be determined
Expand Down Expand Up @@ -1393,8 +1393,7 @@ private boolean resolveFeatureFlag(DatabricksJdbcUrlParams clientParam, String s
boolean serverEnabled = false;
try {
serverEnabled =
DatabricksDriverFeatureFlagsContextFactory.getInstance(this)
.isFeatureEnabled(serverFlagName);
DatabricksDriverFeatureFlagsContextFactory.getInstance(this).getBoolean(serverFlagName);
} catch (Exception e) {
LOGGER.debug("Failed to check server-side flag {}: {}", serverFlagName, e.getMessage());
}
Expand Down
70 changes: 69 additions & 1 deletion src/main/java/com/databricks/jdbc/common/safe/DatabricksDriverFeatureFlagsContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,19 @@
import com.databricks.jdbc.exception.DatabricksHttpException;
import com.databricks.jdbc.log.JdbcLogger;
import com.databricks.jdbc.log.JdbcLoggerFactory;
import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalDouble;
import java.util.OptionalInt;
import java.util.OptionalLong;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
Expand Down Expand Up @@ -152,11 +160,71 @@ void updateConnectionContext(IDatabricksConnectionContext newContext) {
this.connectionContext = newContext;
}

public boolean isFeatureEnabled(String name) {
/** Returns true only for a case-insensitive "true" value; missing values default to false. */
public boolean getBoolean(String name) {

@peco-review-bot peco-review-bot Bot Oct 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Low — Null-handling is inconsistent across the getter family. getBoolean(null) throws NullPointerException (Guava Cache.getIfPresent(null) rejects null keys — and the test asserts this), while every typed getter (getInt32/getInt64/getDouble/getString/getStringList) routes through parse(), which guards name == null and returns an empty result instead. A caller treating these six methods as a uniform API will get a thrown exception from one and a graceful empty from the others for the same null input. This matches the pre-existing isFeatureEnabled behavior so it's not a regression, but since the typed getters are new and establish the "missing/invalid → empty" contract, consider making getBoolean consistent (e.g. guard null the same way) or documenting the divergence in the Javadoc, which currently only mentions "missing values default to false" and says nothing about null throwing.

(Anchored to the nearest changed line — see the description for the exact location.)

String value = featureFlags.getIfPresent(name);
return Boolean.parseBoolean(value);
}

public OptionalInt getInt32(String name) {
JsonNode value = parse(name);
return value != null && value.isIntegralNumber() && value.canConvertToInt()
? OptionalInt.of(value.intValue())
: OptionalInt.empty();
}

public OptionalLong getInt64(String name) {
JsonNode value = parse(name);
return value != null && value.isIntegralNumber() && value.canConvertToLong()
? OptionalLong.of(value.longValue())
: OptionalLong.empty();
}

public OptionalDouble getDouble(String name) {
JsonNode value = parse(name);
if (value == null || !value.isNumber()) {
return OptionalDouble.empty();
}
double number = value.doubleValue();
return Double.isFinite(number) ? OptionalDouble.of(number) : OptionalDouble.empty();
}

public Optional<String> getString(String name) {
JsonNode value = parse(name);
return value != null && value.isTextual() ? Optional.of(value.textValue()) : Optional.empty();
}

public Optional<List<String>> getStringList(String name) {
JsonNode value = parse(name);
if (value == null || !value.isArray()) {
return Optional.empty();
}
List<String> result = new ArrayList<>(value.size());
for (JsonNode item : value) {
if (!item.isTextual()) {
return Optional.empty();
}
result.add(item.textValue());
}
return Optional.of(Collections.unmodifiableList(result));
}

private JsonNode parse(String name) {
if (name == null || name.isEmpty()) {
return null;
}
String value = featureFlags.getIfPresent(name);
if (value == null) {
return null;
}
try {
return JsonUtil.getMapper().readTree(value);
} catch (IOException | RuntimeException e) {
LOGGER.debug("Feature flag {} has malformed JSON; using the consumer default", name);
return null;
}
}

public void shutdown() {
ScheduledFuture<?> task = scheduledRefreshTask;
if (task != null) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/databricks/jdbc/telemetry/TelemetryHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public static boolean isTelemetryAllowedForConnection(IDatabricksConnectionConte
}
return context.isTelemetryEnabled()
&& DatabricksDriverFeatureFlagsContextFactory.getInstance(context)
.isFeatureEnabled(TELEMETRY_FEATURE_FLAG_NAME);
.getBoolean(TELEMETRY_FEATURE_FLAG_NAME);
}

public static void exportTelemetryLog(
Expand Down
18 changes: 9 additions & 9 deletions .../java/com/databricks/jdbc/common/safe/DatabricksDriverFeatureFlagsContextFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,14 @@ void testContextPersistsUntilLastRemoval() {

DatabricksDriverFeatureFlagsContext context =
DatabricksDriverFeatureFlagsContextFactory.getInstance(connX);
assertTrue(context.isFeatureEnabled("test.flag"));
assertTrue(context.getBoolean("test.flag"));

// Remove connX — context should still exist because connY is still open
DatabricksDriverFeatureFlagsContextFactory.removeInstance(connX);

DatabricksDriverFeatureFlagsContext contextAfterX =
DatabricksDriverFeatureFlagsContextFactory.getInstance(connY);
assertTrue(contextAfterX.isFeatureEnabled("test.flag"));
assertTrue(contextAfterX.getBoolean("test.flag"));

// Clean up
DatabricksDriverFeatureFlagsContextFactory.removeInstance(connY);
Expand All @@ -217,8 +217,8 @@ void testSetFeatureFlagsContextWorks() {
DatabricksDriverFeatureFlagsContext context =
DatabricksDriverFeatureFlagsContextFactory.getInstance(connectionContext1);

assertTrue(context.isFeatureEnabled("feature1"));
assertFalse(context.isFeatureEnabled("feature2"));
assertTrue(context.getBoolean("feature1"));
assertFalse(context.getBoolean("feature2"));
}

@Test
Expand Down Expand Up @@ -251,7 +251,7 @@ void testMultipleConnectionsToSameWorkspaceShareFlags() {
DatabricksDriverFeatureFlagsContextFactory.getInstance(conn2);

assertSame(context1, context2);
assertTrue(context2.isFeatureEnabled("shared.flag"));
assertTrue(context2.getBoolean("shared.flag"));

// Clean up
DatabricksDriverFeatureFlagsContextFactory.removeInstance(conn1);
Expand All @@ -276,10 +276,10 @@ void testDifferentWorkspacesHaveIsolatedFlags() {
DatabricksDriverFeatureFlagsContextFactory.getInstance(connectionContext2);

// Verify flags are isolated
assertTrue(context1.isFeatureEnabled("workspace1.flag"));
assertFalse(context1.isFeatureEnabled("workspace2.flag"));
assertTrue(context1.getBoolean("workspace1.flag"));
assertFalse(context1.getBoolean("workspace2.flag"));

assertFalse(context2.isFeatureEnabled("workspace1.flag"));
assertTrue(context2.isFeatureEnabled("workspace2.flag"));
assertFalse(context2.getBoolean("workspace1.flag"));
assertTrue(context2.getBoolean("workspace2.flag"));
}
}
79 changes: 55 additions & 24 deletions src/test/java/com/databricks/jdbc/common/safe/DatabricksDriverFeatureFlagsContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ void testFetchAndSetFlagsFromServer_Success() throws Exception {
.thenReturn(response);
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);
assertTrue(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertTrue(context.getBoolean(FEATURE_FLAG_NAME));
verify(httpClientMock).execute(request);
}
}
Expand All @@ -126,7 +126,7 @@ void testFetchAndSetFlagsFromServer_WithCustomTTL() throws Exception {
.thenReturn(response);
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);
assertTrue(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertTrue(context.getBoolean(FEATURE_FLAG_NAME));
verify(httpClientMock).execute(request);
}
}
Expand All @@ -138,7 +138,7 @@ void testFetchAndSetFlagsFromServer_HttpError() throws IOException, DatabricksHt
when(httpClientMock.execute(any(HttpGet.class))).thenReturn(httpResponseMock);
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);
assertFalse(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertFalse(context.getBoolean(FEATURE_FLAG_NAME));
verify(httpClientMock).execute(request);
}

Expand All @@ -158,7 +158,7 @@ void testFetchAndSetFlagsFromServer_EmptyFlags() throws Exception {
.thenReturn(response);
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);
assertFalse(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertFalse(context.getBoolean(FEATURE_FLAG_NAME));
verify(httpClientMock).execute(request);
}
}
Expand All @@ -179,42 +179,73 @@ void testFetchAndSetFlagsFromServer_NullFlags() throws Exception {
.thenReturn(response);
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);
assertFalse(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertFalse(context.getBoolean(FEATURE_FLAG_NAME));
verify(httpClientMock).execute(request);
}
}

@Test
void testIsFeatureEnabled() {
void testGetBoolean() {
// Test with valid boolean values
Map<String, String> flags = new HashMap<>();
flags.put("flag1", "true");
flags.put("flag2", "false");
flags.put("mixedCase", "TrUe");
context = new DatabricksDriverFeatureFlagsContext(connectionContextMock, flags);
assertTrue(context.isFeatureEnabled("flag1"));
assertFalse(context.isFeatureEnabled("flag2"));
assertTrue(context.getBoolean("flag1"));
assertFalse(context.getBoolean("flag2"));
assertTrue(context.getBoolean("mixedCase"));

// Test with invalid values
flags.put("flag3", "invalid");
flags.put("flag4", "yes");
flags.put("flag5", "null");
flags.put("flag6", "\"true\"");
flags.put("flag7", "");
flags.put("flag8", " true ");
context = new DatabricksDriverFeatureFlagsContext(connectionContextMock, flags);
assertFalse(context.isFeatureEnabled("flag3"));
assertFalse(context.isFeatureEnabled("flag4"));
for (String name :
new String[] {"flag3", "flag4", "flag5", "flag6", "flag7", "flag8", "nonexistent", ""}) {
assertFalse(context.getBoolean(name));
}
assertThrows(NullPointerException.class, () -> context.getBoolean(null));
}

// Test with non-existent flag
assertFalse(context.isFeatureEnabled("nonexistent"));
@Test
void testTypedGetters() {
context =
new DatabricksDriverFeatureFlagsContext(
connectionContextMock,
Map.of(
"boolean", "true",
"int32", Integer.toString(Integer.MIN_VALUE),
"int64", Long.toString(Long.MAX_VALUE),
"double", "3.5",
"string", "\"hello\"",
"string-list", "[\"a\",\"b\"]",
"wrong-type", "\"true\"",
"malformed", "not-json"));

assertTrue(context.getBoolean("boolean"));
assertEquals(Integer.MIN_VALUE, context.getInt32("int32").orElseThrow());
assertEquals(Long.MAX_VALUE, context.getInt64("int64").orElseThrow());
assertEquals(3.5, context.getDouble("double").orElseThrow());
assertEquals("hello", context.getString("string").orElseThrow());
assertEquals(List.of("a", "b"), context.getStringList("string-list").orElseThrow());
assertFalse(context.getBoolean("wrong-type"));
assertTrue(context.getString("malformed").isEmpty());
}

// ===== Additional Integration Tests =====

@Test
void testIsFeatureEnabledForSqlExecFlag() {
void testGetBooleanForSqlExecFlag() {
Map<String, String> flags = new HashMap<>();
flags.put("databricks.partnerplatform.clientConfigsFeatureFlags.enableSqlExecForJdbc", "true");
context = new DatabricksDriverFeatureFlagsContext(connectionContextMock, flags);

assertTrue(
context.isFeatureEnabled(
context.getBoolean(
"databricks.partnerplatform.clientConfigsFeatureFlags.enableSqlExecForJdbc"));
}

Expand Down Expand Up @@ -244,11 +275,11 @@ void testMultipleFeatureFlagsInResponse() throws Exception {
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);

assertTrue(context.isFeatureEnabled("flag1"));
assertTrue(context.getBoolean("flag1"));
assertTrue(
context.isFeatureEnabled(
context.getBoolean(
"databricks.partnerplatform.clientConfigsFeatureFlags.enableSqlExecForJdbc"));
assertFalse(context.isFeatureEnabled("flag3"));
assertFalse(context.getBoolean("flag3"));
}
}

Expand All @@ -262,7 +293,7 @@ void testFetchAndSetFlagsFromServer_404Error() throws IOException, DatabricksHtt
context.fetchAndSetFlagsFromServer(httpClientMock, request);

// Should not throw, and feature should be disabled by default
assertFalse(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertFalse(context.getBoolean(FEATURE_FLAG_NAME));
}

@Test
Expand All @@ -274,7 +305,7 @@ void testFetchAndSetFlagsFromServer_403Error() throws IOException, DatabricksHtt
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);

assertFalse(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertFalse(context.getBoolean(FEATURE_FLAG_NAME));
}

@Test
Expand All @@ -286,17 +317,17 @@ void testFetchAndSetFlagsFromServer_503Error() throws IOException, DatabricksHtt
HttpGet request = new HttpGet(FEATURE_FLAGS_ENDPOINT);
context.fetchAndSetFlagsFromServer(httpClientMock, request);

assertFalse(context.isFeatureEnabled(FEATURE_FLAG_NAME));
assertFalse(context.getBoolean(FEATURE_FLAG_NAME));
}

@Test
void testIsFeatureEnabledCaseSensitive() {
void testGetBooleanCaseSensitive() {
Map<String, String> flags = new HashMap<>();
flags.put("TestFlag", "true");
context = new DatabricksDriverFeatureFlagsContext(connectionContextMock, flags);

assertTrue(context.isFeatureEnabled("TestFlag"));
assertFalse(context.isFeatureEnabled("testflag"));
assertFalse(context.isFeatureEnabled("TESTFLAG"));
assertTrue(context.getBoolean("TestFlag"));
assertFalse(context.getBoolean("testflag"));
assertFalse(context.getBoolean("TESTFLAG"));
}
}
Loading