From 10bb7244d46e7c569525abafb1a82250254b6f72 Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Tue, 11 Aug 2026 19:07:50 -0700 Subject: [PATCH 1/7] Move read-bridge column-default ramp into OpenHouse Keep the deployment-specific ColumnDefaultsSource optional and data-only; OpenHouse owns the read-bridge.column-default feature id, self-serve enabled property, and fail-open toggle lookup before asking for defaults. --- .../openhouse/tables/api/ApiConfig.java | 36 ++-- .../impl/OpenHouseTablesApiHandler.java | 22 +-- .../readbridge/ColumnDefaultsSource.java | 39 +++- .../readbridge/ReadBridgeConfigResolver.java | 171 ++++++++++++++++-- .../ReadBridgeConfigResolverTest.java | 163 ++++++++++++++++- 5 files changed, 364 insertions(+), 67 deletions(-) diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java index 00408f757..510c5e44f 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java @@ -4,8 +4,8 @@ import com.linkedin.openhouse.tables.api.handler.impl.OpenHouseTablesApiHandler; import com.linkedin.openhouse.tables.readbridge.ColumnDefaultsSource; import com.linkedin.openhouse.tables.readbridge.ReadBridgeConfigResolver; -import java.util.Collections; -import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import com.linkedin.openhouse.tables.toggle.TableFeatureToggle; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -18,22 +18,26 @@ public TablesApiHandler tablesApiHandler() { } /** - * Open-source default {@link ColumnDefaultsSource}: supplies none, so read-bridge stays inert. + * Server-side encoder that stamps the read-bridge {@code config}. + * + *

{@link ColumnDefaultsSource} is the column-default capability's single extension point, and + * it is resolved here rather than declared as an overridable default bean. A deployment supplies + * one; with none present that capability is inert and never consults the feature toggle. Each + * capability is wired, and rolled out, on its own. + * + *

Deliberately not a {@code @ConditionalOnMissingBean} default bean. Spring Boot documents + * that condition as safe only inside auto-configuration, and this is an ordinary + * {@code @Configuration}: a component-scanned override happens to work, because {@code + * ConfigurationClassPostProcessor} finishes scanning before it evaluates {@code @Bean} + * conditions, but a deployment declaring its source with {@code @Bean} in a configuration class + * parsed after this one would get a competing no-op bean and need {@code @Primary} to avoid a + * {@code NoUniqueBeanDefinitionException}. With {@link ObjectProvider} no default bean is ever + * registered, so exactly one bean of the type exists however it was declared. */ @Bean - @ConditionalOnMissingBean(ColumnDefaultsSource.class) - public ColumnDefaultsSource columnDefaultsSource() { - return tableDto -> Collections.emptyMap(); - } - - /** - * Server-side encoder that stamps the read-bridge {@code config} from {@link - * ColumnDefaultsSource}. - */ - @Bean - @ConditionalOnMissingBean(ReadBridgeConfigResolver.class) public ReadBridgeConfigResolver readBridgeConfigResolver( - ColumnDefaultsSource columnDefaultsSource) { - return new ReadBridgeConfigResolver(columnDefaultsSource); + ObjectProvider columnDefaultsSource, TableFeatureToggle featureToggle) { + return new ReadBridgeConfigResolver( + columnDefaultsSource.getIfAvailable(() -> ColumnDefaultsSource.NONE), featureToggle); } } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java index 8de0d0a49..5abf91557 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java @@ -43,11 +43,8 @@ public class OpenHouseTablesApiHandler implements TablesApiHandler { * LoadTableResponse.config} convention) onto a freshly mapped response body. The mapper leaves * {@code config} null; it is a request-time decision resolved here. */ - private GetTableResponseBody withConfig( - GetTableResponseBody body, String databaseId, String tableId, TableDto tableDto) { - return body.toBuilder() - .config(readBridgeConfigResolver.resolve(databaseId, tableId, tableDto)) - .build(); + private GetTableResponseBody withConfig(GetTableResponseBody body, TableDto tableDto) { + return body.toBuilder().config(readBridgeConfigResolver.resolve(tableDto)).build(); } @Override @@ -57,9 +54,7 @@ public ApiResponse getTable( TableDto tableDto = tableService.getTable(databaseId, tableId, actingPrincipal); return ApiResponse.builder() .httpStatus(HttpStatus.OK) - .responseBody( - withConfig( - tablesMapper.toGetTableResponseBody(tableDto), databaseId, tableId, tableDto)) + .responseBody(withConfig(tablesMapper.toGetTableResponseBody(tableDto), tableDto)) .build(); } @@ -111,12 +106,7 @@ public ApiResponse createTable( TableDto tableDto = putResult.getFirst(); return ApiResponse.builder() .httpStatus(HttpStatus.CREATED) - .responseBody( - withConfig( - tablesMapper.toGetTableResponseBody(tableDto), - databaseId, - tableDto.getTableId(), - tableDto)) + .responseBody(withConfig(tablesMapper.toGetTableResponseBody(tableDto), tableDto)) .build(); } @@ -134,9 +124,7 @@ public ApiResponse updateTable( TableDto tableDto = putResult.getFirst(); return ApiResponse.builder() .httpStatus(status) - .responseBody( - withConfig( - tablesMapper.toGetTableResponseBody(tableDto), databaseId, tableId, tableDto)) + .responseBody(withConfig(tablesMapper.toGetTableResponseBody(tableDto), tableDto)) .build(); } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java index 74444119a..d5452e153 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java @@ -2,25 +2,44 @@ import com.fasterxml.jackson.databind.JsonNode; import com.linkedin.openhouse.tables.model.TableDto; +import java.util.Collections; import java.util.Map; /** * Pluggable input to the open-source {@code read-bridge} feature: the per-column initial-defaults * to overlay at read time, keyed by Iceberg field-id and valued as Iceberg single-value JSON. * - *

This is the only part of read-bridge a deployment supplies. The open-source default (see - * {@code ApiConfig}) returns nothing, so the feature is wired but inert until a deployment - * overrides this bean (e.g. li-openhouse derives the defaults from the {@code avro.schema.literal} - * table property). + *

This is the only part of read-bridge a deployment supplies, and it supplies data only + * — not policy. Whether a table is bridged at all is decided by {@link ReadBridgeConfigResolver} + * from the open-source feature toggle, so an implementation neither consults nor knows about the + * ramp. Deriving the defaults — from whatever a deployment treats as the authority on a column's + * declared default — is the one deployment-specific step, and the only reason this interface + * exists. * - *

Called on every table-load/commit response, so implementations must be cheap. An empty map - * means "nothing to bridge for this table" — no default is declared, or a declared default is of a - * kind this source does not support; either way the column keeps reading {@code NULL} as it does - * today. Throw instead when a default is declared but cannot be honored (e.g. it does not - * bind to its column's type): degrading there would leave the column reading {@code NULL} while the - * table claims to be bridged, hiding a real defect. + *

No open-source default bean exists: {@code ApiConfig} resolves the type through an {@code + * ObjectProvider} and falls back to {@link #NONE}, so the feature is wired but completely inert out + * of the box — including skipping the toggle lookup entirely. + * + *

{@code JsonNode} rather than {@code String} is deliberate. It makes a stamped value + * well-formed by construction at the only place that produces one, which is what entitles + * the client decoder to treat a malformed entry as a bug and fail loud instead of degrading. + * + *

Called only for tables the ramp has activated, so an implementation may do real work (parsing + * a schema, say) without paying it on every table load fleet-wide. An empty map means "nothing to + * bridge for this table" — no default is declared, or a declared default is of a kind this source + * does not support; either way the column keeps reading {@code NULL} as it does today. Throw + * instead when a default is declared but cannot be honored (e.g. it does not bind to its + * column's type): degrading there would leave the column reading {@code NULL} while the table + * claims to be bridged, hiding a real defect. */ public interface ColumnDefaultsSource { + + /** + * Supplies nothing. The value {@code ApiConfig} falls back to when a deployment supplies no + * source; {@link ReadBridgeConfigResolver} recognises it and short-circuits before the toggle. + */ + ColumnDefaultsSource NONE = tableDto -> Collections.emptyMap(); + /** * @param tableDto the already-loaded table state (no extra fetch needed) * @return field-id -> initial-default as Iceberg single-value JSON; empty/{@code null} = none diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java index 405bb2d3b..627ce8032 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java @@ -2,38 +2,162 @@ import com.fasterxml.jackson.databind.JsonNode; import com.linkedin.openhouse.tables.model.TableDto; +import com.linkedin.openhouse.tables.toggle.TableFeatureToggle; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import lombok.extern.slf4j.Slf4j; /** - * Open-source encoder for the {@code read-bridge} feature: it asks the pluggable {@link - * ColumnDefaultsSource} for a table's column initial-defaults and stamps each as a namespaced entry - * in the per-table {@code config} — {@code openhouse.read-bridge.column-default. = - * }. The client decoder ({@code ReadBridge} in {@code openhouse-java-runtime}) - * reads these entries and overlays the defaults at metadata-load time. + * Open-source encoder for the {@code read-bridge} feature: for a table the ramp has activated, it + * asks the pluggable {@link ColumnDefaultsSource} for that table's column initial-defaults and + * stamps each as a namespaced entry in the per-table {@code config} — {@code + * openhouse.read-bridge.column-default. = }. The client decoder ({@code + * ReadBridge} in {@code openhouse-java-runtime}) reads these entries and overlays the defaults at + * metadata-load time. * *

No envelope/POJO: the flat config map (Iceberg REST {@code LoadTableResponse.config} - * convention) carries the structure directly. Behaviorless by default — the open-source {@link - * ColumnDefaultsSource} bean supplies nothing (see {@code ApiConfig}), so no entries are stamped. A - * deployment delivers the bridge by overriding only {@link ColumnDefaultsSource}. + * convention) carries the structure directly. + * + *

Who decides what

+ * + * This class owns the policy — the feature id, the ramp, and the wire keys — and a + * deployment supplies only the data, via {@link ColumnDefaultsSource}. Keeping the ramp + * here means every deployment inherits it, the self-service property {@code read-bridge.enabled} is + * documented alongside the {@code openhouse.read-bridge.*} keys it controls, and a deployment's + * source is never asked to derive defaults for a table that is not bridged. + * + *

What capabilities share, and what they don't

+ * + * Capabilities bridged through this class share the infrastructure and nothing else: the + * per-table {@code config} channel, the {@code openhouse.read-bridge.*} namespace, and the client's + * decode/apply path. Rollout is never shared. Each capability has its own source, feature id, + * self-service table property and cluster kill switch, so it can be ramped, paused or killed + * without touching any other. There is deliberately no single switch, toggle id or property meaning + * "all of read-bridge". + * + *

{@link #resolve(TableDto)} therefore contains no cross-capability gate at all — it only merges + * what each capability decided for itself. A shared short-circuit there would couple rollouts that + * are meant to be independent: a deployment supplying a deletion-vector source but no + * column-default source must still get deletion vectors. + * + *

Combining rollouts later

+ * + * Independence is the default, not the ceiling. A superset ramp — say {@code v3-read-bridge}, + * activating every capability at once for tables that want the whole V3 read surface — is a natural + * later addition, and nothing here blocks it: it would be one more feature id consulted alongside + * the capability's own. The mechanism-wide ids ({@code read-bridge}, {@code v3-read-bridge}) are + * left unused today so one of them can take that role without colliding with a capability. It will + * need an explicit precedence rule; the sane one is that the more specific wins, so a table setting + * {@code read-bridge.column-default.enabled=false} stays opted out of that capability even while + * opted into the superset. + * + *

Gating, cheapest check first (per capability)

+ * + *
    + *
  1. No source supplied for the capability ({@link ColumnDefaultsSource#NONE}) — structurally + * inert, and notably makes no toggle lookup, so open-source and dev deployments add nothing + * to the table-load path. + *
  2. {@link TableFeatureToggle#isFeatureActivatedWithOverride} — the per-table ramp: an explicit + * {@code read-bridge.column-default.enabled} table property opts a table in or out, otherwise + * the server-managed toggle decides. Rules match database and table as globs, so a {@code *} + * / {@code *} rule ramps or un-ramps the fleet as data, taking effect immediately. That is + * the kill switch; there is deliberately no cluster property duplicating it, which would only + * add a second place to look and a slower one, since it would need a redeploy to change. + *
+ * + *

The toggle is on the read path, so it fails open

+ * + * Consulting the ramp here puts a blocking HouseTables call on every table load, which is a path it + * is not otherwise on — elsewhere toggles gate writes and table-property changes. A HouseTables + * blip must therefore not fail table reads, so a lookup failure is logged and treated as "not + * bridged". + * + *

That is safe for exactly the same reason old clients may ignore unknown keys: not bridging + * leaves the reader at today's behavior. The two are the same property of a capability, used twice. + * A capability where ignoring is unsafe — deletion vectors, where skipping means returning deleted + * rows — must NOT reuse this fail-open block; for those, a lookup failure has to fail the read, + * because serving data that is silently wrong is worse than serving an error. + * + *

The override-honoring form is the correct one here and should stay that way: read-bridge is a + * rollout, not an authorization gate. Features that decide whether a user may write a preserved + * property must keep using the server-only {@code isFeatureActivated}, because the table property + * this form honors is writable by the very user being gated. + * + *

Adding a capability

+ * + * Add a source interface, a {@code Config} method that owns its own source check, kill + * switch, ramp and keys, and one merge line in {@link #resolve(TableDto)}. Nothing in the existing + * capability changes. Deliberately not generalised into a capability registry yet: with a single + * implementation that interface would be a guess. + * + *

Ignoring is not always safe. The client ignores config keys it does not recognise, so a + * capability may only be bridged this way if ignoring it leaves the client at today's behavior. + * That holds for column defaults — an old client reads {@code NULL}, exactly as it does now. It + * would NOT hold for something like deletion vectors, where ignoring the key means returning + * deleted rows: a silent correctness violation rather than a missed improvement. A capability of + * that kind cannot rely on the ignore rule and must not be stamped for a client too old to honor + * it, which means gating on the client version advertised in the {@code User-Agent} header. * *

Mirror: {@link #COLUMN_DEFAULT_PREFIX} is the shared contract with the client decoder; - * keep it in sync. Further V3 features ride the same {@code openhouse.read-bridge.*} namespace as - * additional keys. + * keep it in sync. */ +@Slf4j public class ReadBridgeConfigResolver { - /** Config key prefix for a per-column read-time default; suffixed with the Iceberg field-id. */ - public static final String COLUMN_DEFAULT_PREFIX = "openhouse.read-bridge.column-default."; + /** + * Feature id for the column-default capability's ramp. Also names its self-service table property + * ({@code read-bridge.column-default.enabled}) and its config keys, below. + * + *

Per capability, NOT one id for all of read-bridge. Capabilities bridged through this + * namespace differ wildly in risk and readiness — deletion vectors must be rampable separately + * from column defaults, not dragged along by them. The id is also baked into a user-facing table + * property, so splitting it later means migrating properties customers have already set; it costs + * nothing to get right while nothing is ramped. The bare {@code read-bridge} id is left free for + * a future superset ramp. + */ + public static final String COLUMN_DEFAULT_FEATURE_ID = "read-bridge.column-default"; + + /** + * Config key prefix for a per-column read-time default; suffixed with the Iceberg field-id. + * Derived from the feature id so the ramp, the property and the wire keys cannot drift apart. + */ + public static final String COLUMN_DEFAULT_PREFIX = "openhouse." + COLUMN_DEFAULT_FEATURE_ID + "."; private final ColumnDefaultsSource columnDefaultsSource; - public ReadBridgeConfigResolver(ColumnDefaultsSource columnDefaultsSource) { + private final TableFeatureToggle featureToggle; + + public ReadBridgeConfigResolver( + ColumnDefaultsSource columnDefaultsSource, TableFeatureToggle featureToggle) { this.columnDefaultsSource = columnDefaultsSource; + this.featureToggle = featureToggle; } - public Map resolve(String databaseId, String tableId, TableDto tableDto) { + /** + * Resolves the per-table client {@code config} for {@code tableDto}, empty when nothing is + * bridged. Purely a merge of independently-gated capabilities; see the class javadoc for why + * there is no shared gate here. + * + *

Takes the DTO alone: it already carries the database and table ids, and passing them + * separately invites the call sites to disagree about where they came from. + */ + public Map resolve(TableDto tableDto) { + Map config = new HashMap<>(); + config.putAll(columnDefaultConfig(tableDto)); + return config; + } + + /** The column-default capability: its own source, ramp and keys. */ + private Map columnDefaultConfig(TableDto tableDto) { + if (columnDefaultsSource == ColumnDefaultsSource.NONE) { + // Nothing can be bridged for this capability: skip the toggle lookup, which is a remote + // HouseTables call on the table-load path. + return Collections.emptyMap(); + } + if (!isColumnDefaultRamped(tableDto)) { + return Collections.emptyMap(); // not ramped for this table -> stamp nothing + } Map columnDefaults = columnDefaultsSource.defaults(tableDto); if (columnDefaults == null || columnDefaults.isEmpty()) { return Collections.emptyMap(); // nothing to bridge -> stamp nothing @@ -44,4 +168,23 @@ public Map resolve(String databaseId, String tableId, TableDto t (fieldId, value) -> config.put(COLUMN_DEFAULT_PREFIX + fieldId, value.toString())); return config; } + + /** + * Whether the column-default capability is ramped for this table, failing open on a toggle-lookup + * failure so a HouseTables blip degrades bridging rather than failing the read. Safe only because + * not bridging is today's behavior; see the class javadoc. + */ + private boolean isColumnDefaultRamped(TableDto tableDto) { + try { + return featureToggle.isFeatureActivatedWithOverride(tableDto, COLUMN_DEFAULT_FEATURE_ID); + } catch (RuntimeException e) { + log.warn( + "read-bridge: toggle lookup failed for {}.{}; treating {} as not ramped", + tableDto.getDatabaseId(), + tableDto.getTableId(), + COLUMN_DEFAULT_FEATURE_ID, + e); + return false; + } + } } diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java index a5e06a02f..110a74759 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java @@ -4,6 +4,7 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import com.fasterxml.jackson.databind.JsonNode; @@ -16,6 +17,7 @@ import com.linkedin.openhouse.tables.dto.mapper.TablesMapper; import com.linkedin.openhouse.tables.model.TableDto; import com.linkedin.openhouse.tables.services.TablesService; +import com.linkedin.openhouse.tables.toggle.TableFeatureToggle; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; @@ -26,21 +28,164 @@ public class ReadBridgeConfigResolverTest { /** Open-source default source: supplies nothing, so the feature is inert. */ - private static final ColumnDefaultsSource NONE = tableDto -> Collections.emptyMap(); + private static final ColumnDefaultsSource NONE = ColumnDefaultsSource.NONE; private static final String PREFIX = ReadBridgeConfigResolver.COLUMN_DEFAULT_PREFIX; + /** A toggle that ramps everything, so a test isolates the encoder rather than the ramp. */ + private static final TableFeatureToggle ALL_ON = + new TableFeatureToggle() { + @Override + public boolean isFeatureActivated(String databaseId, String tableId, String featureId) { + return true; + } + }; + + private static ReadBridgeConfigResolver resolverFor(ColumnDefaultsSource source) { + return new ReadBridgeConfigResolver(source, ALL_ON); + } + + private static ColumnDefaultsSource oneDefault() { + return tableDto -> Collections.singletonMap(5, TextNode.valueOf("US")); + } + + /** A table carrying an explicit self-service opt-in/opt-out property. */ + private static TableDto tableWithOverride(String value) { + return TableDto.builder() + .databaseId("db") + .tableId("tbl") + .tableProperties( + Collections.singletonMap( + ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID + + TableFeatureToggle.ENABLED_PROPERTY_SUFFIX, + value)) + .build(); + } + + /** Gate 1: no deployment-supplied source => inert, and crucially no toggle lookup at all. */ @Test - public void testEmptyWhenNoColumnDefaults() { + public void testInertAndSkipsToggleWhenNoSourceSupplied() { + TableFeatureToggle toggle = mock(TableFeatureToggle.class); + ReadBridgeConfigResolver resolver = + new ReadBridgeConfigResolver(ColumnDefaultsSource.NONE, toggle); + + Assertions.assertTrue(resolver.resolve(mock(TableDto.class)).isEmpty()); + // The toggle is a remote HouseTables call on the table-load path; it must not be made. + verifyNoInteractions(toggle); + } + + /** + * The ramp lookup is a blocking HouseTables call, and this is the table-load path — a path + * toggles are not otherwise on. A HouseTables outage must degrade bridging, not fail reads. Sound + * only because not bridging is exactly today's behavior; a capability where ignoring is unsafe + * (deletion vectors) would have to fail the read instead. + */ + @Test + public void testToggleLookupFailureDegradesInsteadOfFailingTheRead() { + TableFeatureToggle exploding = + new TableFeatureToggle() { + @Override + public boolean isFeatureActivated(String databaseId, String tableId, String featureId) { + throw new IllegalStateException("housetables is down"); + } + }; + + Map config = + new ReadBridgeConfigResolver(oneDefault(), exploding) + .resolve(TableDto.builder().databaseId("db").tableId("tbl").build()); + + Assertions.assertTrue(config.isEmpty()); + } + + /** Gate 3: a table the ramp has not activated is not bridged, and its source is never asked. */ + @Test + public void testUnrampedTableIsNotBridgedAndSourceNotConsulted() { + ColumnDefaultsSource source = mock(ColumnDefaultsSource.class); + TableFeatureToggle allOff = + new TableFeatureToggle() { + @Override + public boolean isFeatureActivated(String databaseId, String tableId, String featureId) { + return false; + } + }; + + Assertions.assertTrue( + new ReadBridgeConfigResolver(source, allOff) + .resolve(TableDto.builder().databaseId("db").tableId("tbl").build()) + .isEmpty()); + // Deriving defaults can be expensive (a deployment may parse a schema); gate first. + verifyNoInteractions(source); + } + + /** The self-service property opts a table in even when the server-managed ramp says no. */ + @Test + public void testTablePropertyOptsInOverServerToggle() { + TableFeatureToggle allOff = + new TableFeatureToggle() { + @Override + public boolean isFeatureActivated(String databaseId, String tableId, String featureId) { + return false; + } + }; + Map config = + new ReadBridgeConfigResolver(oneDefault(), allOff).resolve(tableWithOverride("true")); + + Assertions.assertEquals("\"US\"", config.get(PREFIX + "5")); + } + + /** ...and opts it out even when the server-managed ramp says yes. */ + @Test + public void testTablePropertyOptsOutOverServerToggle() { + Assertions.assertTrue(resolverFor(oneDefault()).resolve(tableWithOverride("false")).isEmpty()); + } + + /** + * The capability's feature id, its self-service property and its wire keys are one token. Pinned + * as literals because all three are external contracts: the id is stored in HouseTables toggle + * rules, the property is set on customer tables, and the prefix is mirrored by the client + * decoder. Deriving them from each other keeps them consistent; asserting the literals keeps a + * refactor from silently renaming all three at once. + */ + @Test + public void testFeatureIdPropertyAndKeysAreOneToken() { + Assertions.assertEquals( + "read-bridge.column-default", ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID); + Assertions.assertEquals( + "read-bridge.column-default.enabled", + ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID + + TableFeatureToggle.ENABLED_PROPERTY_SUFFIX); + Assertions.assertEquals( + "openhouse.read-bridge.column-default.", ReadBridgeConfigResolver.COLUMN_DEFAULT_PREFIX); + } + + /** + * Rollout is per capability, never for read-bridge as a whole: capabilities share only the + * transport. A table opted out of column defaults must not thereby be opted out of a capability + * added later, and vice versa. Pinned because the id is baked into a customer-set property, so + * splitting it after the fact means a migration. + * + *

The bare "read-bridge" id staying unclaimed is also the room a future superset ramp (e.g. + * "v3-read-bridge", activating every capability at once) needs in order to exist without + * colliding with a capability's own id. + */ + @Test + public void testRolloutIdIsScopedToTheCapabilityNotTheMechanism() { + Assertions.assertNotEquals("read-bridge", ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID); + Assertions.assertNotEquals( + "v3-read-bridge", ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID); Assertions.assertTrue( - new ReadBridgeConfigResolver(NONE).resolve("db", "tbl", mock(TableDto.class)).isEmpty()); + ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID.startsWith("read-bridge.")); + } + + @Test + public void testEmptyWhenNoColumnDefaults() { + Assertions.assertTrue(resolverFor(NONE).resolve(mock(TableDto.class)).isEmpty()); } @Test public void testStampsColumnDefaultEntry() { ColumnDefaultsSource source = tableDto -> Collections.singletonMap(5, TextNode.valueOf("US")); - Map config = - new ReadBridgeConfigResolver(source).resolve("db", "tbl", mock(TableDto.class)); + Map config = resolverFor(source).resolve(mock(TableDto.class)); // value is the single-value JSON for the default ("US" -> "\"US\""). Assertions.assertEquals("\"US\"", config.get(PREFIX + "5")); } @@ -54,8 +199,7 @@ public void testStampsAllColumnDefaultsAsSeparateEntries() { defaults.put(7, IntNode.valueOf(0)); return defaults; }; - Map config = - new ReadBridgeConfigResolver(source).resolve("db", "tbl", mock(TableDto.class)); + Map config = resolverFor(source).resolve(mock(TableDto.class)); Assertions.assertEquals(2, config.size()); Assertions.assertEquals("\"US\"", config.get(PREFIX + "5")); Assertions.assertEquals("0", config.get(PREFIX + "7")); @@ -74,7 +218,7 @@ public void testGetTableStampsResolvedConfig() { .thenReturn(GetTableResponseBody.builder().tableId("tbl").databaseId("db").build()); Map resolved = Collections.singletonMap(PREFIX + "5", "\"US\""); - when(resolver.resolve(eq("db"), eq("tbl"), eq(tableDto))).thenReturn(resolved); + when(resolver.resolve(eq(tableDto))).thenReturn(resolved); OpenHouseTablesApiHandler handler = handlerWith(tableService, tablesMapper, resolver); @@ -94,8 +238,7 @@ public void testGetTableLeavesConfigEmptyWithNoColumnDefaults() { when(tablesMapper.toGetTableResponseBody(any())) .thenReturn(GetTableResponseBody.builder().tableId("tbl").databaseId("db").build()); - OpenHouseTablesApiHandler handler = - handlerWith(tableService, tablesMapper, new ReadBridgeConfigResolver(NONE)); + OpenHouseTablesApiHandler handler = handlerWith(tableService, tablesMapper, resolverFor(NONE)); ApiResponse response = handler.getTable("db", "tbl", "principal"); From d8c50b9d991ab5a3b1403c41f0d02d6016d06e04 Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Tue, 11 Aug 2026 22:13:05 -0700 Subject: [PATCH 2/7] Fix read-bridge policy docs and pin empty-source / opt-in-skip-HTS Correct the self-service property name and drop the contradictory "cluster kill switch" claim. Add coverage for a real source that returns no defaults, and assert table-property opt-in never calls the server toggle. Testing Done: - :services:tables:test --tests '*ReadBridgeConfigResolverTest' --- .../readbridge/ReadBridgeConfigResolver.java | 14 ++++---- .../ReadBridgeConfigResolverTest.java | 35 ++++++++++++++----- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java index 627ce8032..e0c5563e5 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java @@ -23,18 +23,18 @@ * * This class owns the policy — the feature id, the ramp, and the wire keys — and a * deployment supplies only the data, via {@link ColumnDefaultsSource}. Keeping the ramp - * here means every deployment inherits it, the self-service property {@code read-bridge.enabled} is - * documented alongside the {@code openhouse.read-bridge.*} keys it controls, and a deployment's - * source is never asked to derive defaults for a table that is not bridged. + * here means every deployment inherits it, the self-service property {@code + * read-bridge.column-default.enabled} is documented alongside the {@code openhouse.read-bridge.*} + * keys it controls, and a deployment's source is never asked to derive defaults for a table that is + * not bridged. * *

What capabilities share, and what they don't

* * Capabilities bridged through this class share the infrastructure and nothing else: the * per-table {@code config} channel, the {@code openhouse.read-bridge.*} namespace, and the client's - * decode/apply path. Rollout is never shared. Each capability has its own source, feature id, - * self-service table property and cluster kill switch, so it can be ramped, paused or killed - * without touching any other. There is deliberately no single switch, toggle id or property meaning - * "all of read-bridge". + * decode/apply path. Rollout is never shared. Each capability has its own source, feature id, and + * self-service table property, so it can be ramped, paused or killed without touching any other. + * There is deliberately no single switch, toggle id or property meaning "all of read-bridge". * *

{@link #resolve(TableDto)} therefore contains no cross-capability gate at all — it only merges * what each capability decided for itself. A shared short-circuit there would couple rollouts that diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java index 110a74759..9e2140924 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java @@ -3,7 +3,10 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.CALLS_REAL_METHODS; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -120,17 +123,17 @@ public boolean isFeatureActivated(String databaseId, String tableId, String feat /** The self-service property opts a table in even when the server-managed ramp says no. */ @Test public void testTablePropertyOptsInOverServerToggle() { - TableFeatureToggle allOff = - new TableFeatureToggle() { - @Override - public boolean isFeatureActivated(String databaseId, String tableId, String featureId) { - return false; - } - }; + // CALLS_REAL_METHODS so the override-honoring default reads the table property; stub the + // server-side form so an accidental HTS call would return false. + TableFeatureToggle toggle = mock(TableFeatureToggle.class, CALLS_REAL_METHODS); + when(toggle.isFeatureActivated(anyString(), anyString(), anyString())).thenReturn(false); + Map config = - new ReadBridgeConfigResolver(oneDefault(), allOff).resolve(tableWithOverride("true")); + new ReadBridgeConfigResolver(oneDefault(), toggle).resolve(tableWithOverride("true")); Assertions.assertEquals("\"US\"", config.get(PREFIX + "5")); + // Explicit opt-in is decided from the table property alone; no HouseTables round-trip. + verify(toggle, never()).isFeatureActivated(anyString(), anyString(), anyString()); } /** ...and opts it out even when the server-managed ramp says yes. */ @@ -139,6 +142,22 @@ public void testTablePropertyOptsOutOverServerToggle() { Assertions.assertTrue(resolverFor(oneDefault()).resolve(tableWithOverride("false")).isEmpty()); } + /** + * Source present and table ramped, but the source has nothing to stamp — still empty config. Not + * the same as {@link ColumnDefaultsSource#NONE}: the toggle ran and the source was asked. + */ + @Test + public void testEmptyWhenSourceReturnsNoDefaults() { + ColumnDefaultsSource emptySource = mock(ColumnDefaultsSource.class); + when(emptySource.defaults(any())).thenReturn(Collections.emptyMap()); + + Assertions.assertTrue( + resolverFor(emptySource) + .resolve(TableDto.builder().databaseId("db").tableId("tbl").build()) + .isEmpty()); + verify(emptySource).defaults(any()); + } + /** * The capability's feature id, its self-service property and its wire keys are one token. Pinned * as literals because all three are external contracts: the id is stored in HouseTables toggle From 05a56e09607a569cadec6915920a7552ae280c43 Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Tue, 11 Aug 2026 22:20:14 -0700 Subject: [PATCH 3/7] Docs: describe exact-match HTS ramp, not glob */* BaseTableFeatureToggle looks up (databaseId, tableId, featureId) exactly; claiming a * / * fleet kill switch was inaccurate. Also note HTS is only hit when the self-service property is absent. --- .../readbridge/ReadBridgeConfigResolver.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java index e0c5563e5..34729cb2d 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java @@ -59,19 +59,22 @@ * inert, and notably makes no toggle lookup, so open-source and dev deployments add nothing * to the table-load path. *

  • {@link TableFeatureToggle#isFeatureActivatedWithOverride} — the per-table ramp: an explicit - * {@code read-bridge.column-default.enabled} table property opts a table in or out, otherwise - * the server-managed toggle decides. Rules match database and table as globs, so a {@code *} - * / {@code *} rule ramps or un-ramps the fleet as data, taking effect immediately. That is - * the kill switch; there is deliberately no cluster property duplicating it, which would only - * add a second place to look and a slower one, since it would need a redeploy to change. + * {@code read-bridge.column-default.enabled} table property opts a table in or out without a + * HouseTables call; when absent, the server-managed toggle decides via an exact {@code + * (databaseId, tableId, featureId)} lookup ({@link + * com.linkedin.openhouse.tables.toggle.BaseTableFeatureToggle}). There is no glob / {@code *} + * matcher today — fleet ramp means writing {@code ACTIVE} rows (or setting the table + * property) per table. That HTS row (or the property) is the kill switch; there is + * deliberately no cluster property duplicating it, which would only add a second place to + * look and a slower one, since it would need a redeploy to change. * * *

    The toggle is on the read path, so it fails open

    * - * Consulting the ramp here puts a blocking HouseTables call on every table load, which is a path it - * is not otherwise on — elsewhere toggles gate writes and table-property changes. A HouseTables - * blip must therefore not fail table reads, so a lookup failure is logged and treated as "not - * bridged". + * Consulting the ramp here can put a blocking HouseTables call on table load when the self-service + * property is absent — a path toggles are not otherwise on (elsewhere they gate writes and + * table-property changes). A HouseTables blip must therefore not fail table reads, so a lookup + * failure is logged and treated as "not bridged". * *

    That is safe for exactly the same reason old clients may ignore unknown keys: not bridging * leaves the reader at today's behavior. The two are the same property of a capability, used twice. From 4bcad86362044d0e65e1b06f6feb31d507f46a4d Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Tue, 11 Aug 2026 22:32:27 -0700 Subject: [PATCH 4/7] Trim read-bridge policy comments to short why-notes Replace essay javadoc with brief ownership, contract, ObjectProvider, and fail-open notes so the PR description carries the design narrative. --- .../openhouse/tables/api/ApiConfig.java | 19 +-- .../readbridge/ColumnDefaultsSource.java | 36 +---- .../readbridge/ReadBridgeConfigResolver.java | 138 ++---------------- 3 files changed, 19 insertions(+), 174 deletions(-) diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java index 510c5e44f..580686da0 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/ApiConfig.java @@ -9,7 +9,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -/** Class that holds all the Beans related to a controller. */ +/** Beans related to tables API controllers. */ @Configuration public class ApiConfig { @Bean @@ -18,21 +18,8 @@ public TablesApiHandler tablesApiHandler() { } /** - * Server-side encoder that stamps the read-bridge {@code config}. - * - *

    {@link ColumnDefaultsSource} is the column-default capability's single extension point, and - * it is resolved here rather than declared as an overridable default bean. A deployment supplies - * one; with none present that capability is inert and never consults the feature toggle. Each - * capability is wired, and rolled out, on its own. - * - *

    Deliberately not a {@code @ConditionalOnMissingBean} default bean. Spring Boot documents - * that condition as safe only inside auto-configuration, and this is an ordinary - * {@code @Configuration}: a component-scanned override happens to work, because {@code - * ConfigurationClassPostProcessor} finishes scanning before it evaluates {@code @Bean} - * conditions, but a deployment declaring its source with {@code @Bean} in a configuration class - * parsed after this one would get a competing no-op bean and need {@code @Primary} to avoid a - * {@code NoUniqueBeanDefinitionException}. With {@link ObjectProvider} no default bean is ever - * registered, so exactly one bean of the type exists however it was declared. + * Prefer {@link ObjectProvider} over a {@code @ConditionalOnMissingBean} noop so a deployment + * {@code @Bean} source cannot collide with an OSS default. */ @Bean public ReadBridgeConfigResolver readBridgeConfigResolver( diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java index d5452e153..5793d1a78 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java @@ -6,43 +6,17 @@ import java.util.Map; /** - * Pluggable input to the open-source {@code read-bridge} feature: the per-column initial-defaults - * to overlay at read time, keyed by Iceberg field-id and valued as Iceberg single-value JSON. - * - *

    This is the only part of read-bridge a deployment supplies, and it supplies data only - * — not policy. Whether a table is bridged at all is decided by {@link ReadBridgeConfigResolver} - * from the open-source feature toggle, so an implementation neither consults nor knows about the - * ramp. Deriving the defaults — from whatever a deployment treats as the authority on a column's - * declared default — is the one deployment-specific step, and the only reason this interface - * exists. - * - *

    No open-source default bean exists: {@code ApiConfig} resolves the type through an {@code - * ObjectProvider} and falls back to {@link #NONE}, so the feature is wired but completely inert out - * of the box — including skipping the toggle lookup entirely. - * - *

    {@code JsonNode} rather than {@code String} is deliberate. It makes a stamped value - * well-formed by construction at the only place that produces one, which is what entitles - * the client decoder to treat a malformed entry as a bug and fail loud instead of degrading. - * - *

    Called only for tables the ramp has activated, so an implementation may do real work (parsing - * a schema, say) without paying it on every table load fleet-wide. An empty map means "nothing to - * bridge for this table" — no default is declared, or a declared default is of a kind this source - * does not support; either way the column keeps reading {@code NULL} as it does today. Throw - * instead when a default is declared but cannot be honored (e.g. it does not bind to its - * column's type): degrading there would leave the column reading {@code NULL} while the table - * claims to be bridged, hiding a real defect. + * Deployment-supplied column defaults (data only). Keyed by Iceberg field-id; values are Iceberg + * single-value JSON. Policy/ramp lives in {@link ReadBridgeConfigResolver}. */ public interface ColumnDefaultsSource { - /** - * Supplies nothing. The value {@code ApiConfig} falls back to when a deployment supplies no - * source; {@link ReadBridgeConfigResolver} recognises it and short-circuits before the toggle. - */ + /** Sentinel when no deployment bean is registered; resolver short-circuits before HTS. */ ColumnDefaultsSource NONE = tableDto -> Collections.emptyMap(); /** - * @param tableDto the already-loaded table state (no extra fetch needed) - * @return field-id -> initial-default as Iceberg single-value JSON; empty/{@code null} = none + * @return field-id → default JSON; empty/null means nothing to stamp. Throw if a declared default + * cannot bind (do not silently omit). */ Map defaults(TableDto tableDto); } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java index 34729cb2d..e0042ac6a 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java @@ -9,122 +9,16 @@ import lombok.extern.slf4j.Slf4j; /** - * Open-source encoder for the {@code read-bridge} feature: for a table the ramp has activated, it - * asks the pluggable {@link ColumnDefaultsSource} for that table's column initial-defaults and - * stamps each as a namespaced entry in the per-table {@code config} — {@code - * openhouse.read-bridge.column-default. = }. The client decoder ({@code - * ReadBridge} in {@code openhouse-java-runtime}) reads these entries and overlays the defaults at - * metadata-load time. - * - *

    No envelope/POJO: the flat config map (Iceberg REST {@code LoadTableResponse.config} - * convention) carries the structure directly. - * - *

    Who decides what

    - * - * This class owns the policy — the feature id, the ramp, and the wire keys — and a - * deployment supplies only the data, via {@link ColumnDefaultsSource}. Keeping the ramp - * here means every deployment inherits it, the self-service property {@code - * read-bridge.column-default.enabled} is documented alongside the {@code openhouse.read-bridge.*} - * keys it controls, and a deployment's source is never asked to derive defaults for a table that is - * not bridged. - * - *

    What capabilities share, and what they don't

    - * - * Capabilities bridged through this class share the infrastructure and nothing else: the - * per-table {@code config} channel, the {@code openhouse.read-bridge.*} namespace, and the client's - * decode/apply path. Rollout is never shared. Each capability has its own source, feature id, and - * self-service table property, so it can be ramped, paused or killed without touching any other. - * There is deliberately no single switch, toggle id or property meaning "all of read-bridge". - * - *

    {@link #resolve(TableDto)} therefore contains no cross-capability gate at all — it only merges - * what each capability decided for itself. A shared short-circuit there would couple rollouts that - * are meant to be independent: a deployment supplying a deletion-vector source but no - * column-default source must still get deletion vectors. - * - *

    Combining rollouts later

    - * - * Independence is the default, not the ceiling. A superset ramp — say {@code v3-read-bridge}, - * activating every capability at once for tables that want the whole V3 read surface — is a natural - * later addition, and nothing here blocks it: it would be one more feature id consulted alongside - * the capability's own. The mechanism-wide ids ({@code read-bridge}, {@code v3-read-bridge}) are - * left unused today so one of them can take that role without colliding with a capability. It will - * need an explicit precedence rule; the sane one is that the more specific wins, so a table setting - * {@code read-bridge.column-default.enabled=false} stays opted out of that capability even while - * opted into the superset. - * - *

    Gating, cheapest check first (per capability)

    - * - *
      - *
    1. No source supplied for the capability ({@link ColumnDefaultsSource#NONE}) — structurally - * inert, and notably makes no toggle lookup, so open-source and dev deployments add nothing - * to the table-load path. - *
    2. {@link TableFeatureToggle#isFeatureActivatedWithOverride} — the per-table ramp: an explicit - * {@code read-bridge.column-default.enabled} table property opts a table in or out without a - * HouseTables call; when absent, the server-managed toggle decides via an exact {@code - * (databaseId, tableId, featureId)} lookup ({@link - * com.linkedin.openhouse.tables.toggle.BaseTableFeatureToggle}). There is no glob / {@code *} - * matcher today — fleet ramp means writing {@code ACTIVE} rows (or setting the table - * property) per table. That HTS row (or the property) is the kill switch; there is - * deliberately no cluster property duplicating it, which would only add a second place to - * look and a slower one, since it would need a redeploy to change. - *
    - * - *

    The toggle is on the read path, so it fails open

    - * - * Consulting the ramp here can put a blocking HouseTables call on table load when the self-service - * property is absent — a path toggles are not otherwise on (elsewhere they gate writes and - * table-property changes). A HouseTables blip must therefore not fail table reads, so a lookup - * failure is logged and treated as "not bridged". - * - *

    That is safe for exactly the same reason old clients may ignore unknown keys: not bridging - * leaves the reader at today's behavior. The two are the same property of a capability, used twice. - * A capability where ignoring is unsafe — deletion vectors, where skipping means returning deleted - * rows — must NOT reuse this fail-open block; for those, a lookup failure has to fail the read, - * because serving data that is silently wrong is worse than serving an error. - * - *

    The override-honoring form is the correct one here and should stay that way: read-bridge is a - * rollout, not an authorization gate. Features that decide whether a user may write a preserved - * property must keep using the server-only {@code isFeatureActivated}, because the table property - * this form honors is writable by the very user being gated. - * - *

    Adding a capability

    - * - * Add a source interface, a {@code Config} method that owns its own source check, kill - * switch, ramp and keys, and one merge line in {@link #resolve(TableDto)}. Nothing in the existing - * capability changes. Deliberately not generalised into a capability registry yet: with a single - * implementation that interface would be a guess. - * - *

    Ignoring is not always safe. The client ignores config keys it does not recognise, so a - * capability may only be bridged this way if ignoring it leaves the client at today's behavior. - * That holds for column defaults — an old client reads {@code NULL}, exactly as it does now. It - * would NOT hold for something like deletion vectors, where ignoring the key means returning - * deleted rows: a silent correctness violation rather than a missed improvement. A capability of - * that kind cannot rely on the ignore rule and must not be stamped for a client too old to honor - * it, which means gating on the client version advertised in the {@code User-Agent} header. - * - *

    Mirror: {@link #COLUMN_DEFAULT_PREFIX} is the shared contract with the client decoder; - * keep it in sync. + * Stamps per-table {@code config} for read-bridge capabilities. Owns policy (feature id, ramp, + * keys); deployments supply data via {@link ColumnDefaultsSource}. */ @Slf4j public class ReadBridgeConfigResolver { - /** - * Feature id for the column-default capability's ramp. Also names its self-service table property - * ({@code read-bridge.column-default.enabled}) and its config keys, below. - * - *

    Per capability, NOT one id for all of read-bridge. Capabilities bridged through this - * namespace differ wildly in risk and readiness — deletion vectors must be rampable separately - * from column defaults, not dragged along by them. The id is also baked into a user-facing table - * property, so splitting it later means migrating properties customers have already set; it costs - * nothing to get right while nothing is ramped. The bare {@code read-bridge} id is left free for - * a future superset ramp. - */ + /** Capability id; also names {@code .enabled} and the config key prefix below. */ public static final String COLUMN_DEFAULT_FEATURE_ID = "read-bridge.column-default"; - /** - * Config key prefix for a per-column read-time default; suffixed with the Iceberg field-id. - * Derived from the feature id so the ramp, the property and the wire keys cannot drift apart. - */ + /** Client contract: {@code openhouse.read-bridge.column-default.}. */ public static final String COLUMN_DEFAULT_PREFIX = "openhouse." + COLUMN_DEFAULT_FEATURE_ID + "."; private final ColumnDefaultsSource columnDefaultsSource; @@ -137,45 +31,35 @@ public ReadBridgeConfigResolver( this.featureToggle = featureToggle; } - /** - * Resolves the per-table client {@code config} for {@code tableDto}, empty when nothing is - * bridged. Purely a merge of independently-gated capabilities; see the class javadoc for why - * there is no shared gate here. - * - *

    Takes the DTO alone: it already carries the database and table ids, and passing them - * separately invites the call sites to disagree about where they came from. - */ + /** Merges independently gated capabilities; empty when nothing is bridged. */ public Map resolve(TableDto tableDto) { Map config = new HashMap<>(); config.putAll(columnDefaultConfig(tableDto)); return config; } - /** The column-default capability: its own source, ramp and keys. */ private Map columnDefaultConfig(TableDto tableDto) { + // No deployment source → skip HTS entirely. if (columnDefaultsSource == ColumnDefaultsSource.NONE) { - // Nothing can be bridged for this capability: skip the toggle lookup, which is a remote - // HouseTables call on the table-load path. return Collections.emptyMap(); } if (!isColumnDefaultRamped(tableDto)) { - return Collections.emptyMap(); // not ramped for this table -> stamp nothing + return Collections.emptyMap(); } Map columnDefaults = columnDefaultsSource.defaults(tableDto); if (columnDefaults == null || columnDefaults.isEmpty()) { - return Collections.emptyMap(); // nothing to bridge -> stamp nothing + return Collections.emptyMap(); } Map config = new HashMap<>(); - // JsonNode.toString() is the single-value JSON (e.g. "US" -> "\"US\"", 0 -> "0"). columnDefaults.forEach( (fieldId, value) -> config.put(COLUMN_DEFAULT_PREFIX + fieldId, value.toString())); return config; } /** - * Whether the column-default capability is ramped for this table, failing open on a toggle-lookup - * failure so a HouseTables blip degrades bridging rather than failing the read. Safe only because - * not bridging is today's behavior; see the class javadoc. + * Uses {@link TableFeatureToggle#isFeatureActivatedWithOverride} so {@code + * read-bridge.column-default.enabled} can opt in/out without HTS. Fail-open on lookup errors: not + * bridging equals today's NULL reads. */ private boolean isColumnDefaultRamped(TableDto tableDto) { try { From 6903c7f3851aaa75a27dd45eb4643bc9ae53fb12 Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Tue, 11 Aug 2026 22:33:04 -0700 Subject: [PATCH 5/7] Shorten withConfig javadoc to a one-liner --- .../tables/api/handler/impl/OpenHouseTablesApiHandler.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java index 5abf91557..12091bae1 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/api/handler/impl/OpenHouseTablesApiHandler.java @@ -38,11 +38,7 @@ public class OpenHouseTablesApiHandler implements TablesApiHandler { @Autowired private ReadBridgeConfigResolver readBridgeConfigResolver; - /** - * Stamp the server-resolved, per-table client {@code config} (Iceberg REST {@code - * LoadTableResponse.config} convention) onto a freshly mapped response body. The mapper leaves - * {@code config} null; it is a request-time decision resolved here. - */ + /** Request-time {@code config} stamp; mapper leaves it null. */ private GetTableResponseBody withConfig(GetTableResponseBody body, TableDto tableDto) { return body.toBuilder().config(readBridgeConfigResolver.resolve(tableDto)).build(); } From b8110826b63df5bbffca2f382bcdc8d73b832ce0 Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Wed, 12 Aug 2026 15:56:31 -0700 Subject: [PATCH 6/7] Add HTTP e2e for column-default config ramp. --- .../h2/ReadBridgeColumnDefaultE2ETest.java | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/ReadBridgeColumnDefaultE2ETest.java diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/ReadBridgeColumnDefaultE2ETest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/ReadBridgeColumnDefaultE2ETest.java new file mode 100644 index 000000000..cc551d3d4 --- /dev/null +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/e2e/h2/ReadBridgeColumnDefaultE2ETest.java @@ -0,0 +1,191 @@ +package com.linkedin.openhouse.tables.e2e.h2; + +import static com.linkedin.openhouse.tables.model.TableModelConstants.CLUSTER_NAME; +import static com.linkedin.openhouse.tables.model.TableModelConstants.GET_TABLE_RESPONSE_BODY; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.fasterxml.jackson.databind.node.TextNode; +import com.jayway.jsonpath.JsonPath; +import com.linkedin.openhouse.cluster.storage.StorageManager; +import com.linkedin.openhouse.common.test.cluster.PropertyOverrideContextInitializer; +import com.linkedin.openhouse.housetables.client.model.ToggleStatus; +import com.linkedin.openhouse.tables.api.spec.v0.response.GetTableResponseBody; +import com.linkedin.openhouse.tables.mock.properties.AuthorizationPropertiesInitializer; +import com.linkedin.openhouse.tables.readbridge.ColumnDefaultsSource; +import com.linkedin.openhouse.tables.readbridge.ReadBridgeConfigResolver; +import com.linkedin.openhouse.tables.toggle.TableFeatureToggle; +import com.linkedin.openhouse.tables.toggle.model.TableToggleStatus; +import com.linkedin.openhouse.tables.toggle.repository.ToggleStatusesRepository; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Import; +import org.springframework.http.MediaType; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.MvcResult; +import org.springframework.test.web.servlet.ResultActions; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; + +/** + * HTTP create/get stamps {@code config} from a stub {@link ColumnDefaultsSource} according to the + * OpenHouse ramp. Deployment encoders are out of scope; resolver unit tests cover the same matrix. + */ +@SpringBootTest +@AutoConfigureMockMvc +@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_CLASS) +@Import(ReadBridgeColumnDefaultE2ETest.StubDefaults.class) +@ContextConfiguration( + initializers = { + PropertyOverrideContextInitializer.class, + AuthorizationPropertiesInitializer.class + }) +public class ReadBridgeColumnDefaultE2ETest { + + private static final String CONFIG_KEY = ReadBridgeConfigResolver.COLUMN_DEFAULT_PREFIX + "5"; + private static final String ENABLED_PROP = + ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID + + TableFeatureToggle.ENABLED_PROPERTY_SUFFIX; + + @TestConfiguration + static class StubDefaults { + @Bean + ColumnDefaultsSource stubColumnDefaults() { + return tableDto -> Collections.singletonMap(5, TextNode.valueOf("US")); + } + } + + @Autowired private MockMvc mvc; + @Autowired private StorageManager storageManager; + @Autowired private ToggleStatusesRepository toggleStatusesRepository; + + private GetTableResponseBody created; + private TableToggleStatus toggleStatus; + + @AfterEach + public void tearDown() throws Exception { + if (created != null) { + RequestAndValidateHelper.deleteTableAndValidateResponse(mvc, created); + created = null; + } + if (toggleStatus != null) { + toggleStatusesRepository.delete(toggleStatus); + toggleStatus = null; + } + } + + @Test + public void createAndGet_stampsColumnDefaultConfigWhenEnabled() throws Exception { + created = create(uniqueTable("prop_on"), Collections.singletonMap(ENABLED_PROP, "true")); + + MvcResult createdResult = + RequestAndValidateHelper.createTableAndValidateResponse(created, mvc, storageManager); + assertEquals( + "\"US\"", + JsonPath.read( + createdResult.getResponse().getContentAsString(), "$.config['" + CONFIG_KEY + "']")); + + getTable() + .andExpect(status().isOk()) + .andExpect(jsonPath("$.config['" + CONFIG_KEY + "']", is("\"US\""))) + .andExpect(jsonPath("$.tableProperties['" + ENABLED_PROP + "']", is("true"))); + } + + @Test + public void get_omitsColumnDefaultConfigWhenFeatureDisabled() throws Exception { + created = create(uniqueTable("prop_off"), Collections.singletonMap(ENABLED_PROP, "false")); + RequestAndValidateHelper.createTableAndValidateResponse(created, mvc, storageManager); + getTable() + .andExpect(status().isOk()) + .andExpect(jsonPath("$.config['" + CONFIG_KEY + "']").doesNotExist()); + } + + @Test + public void get_omitsConfigWhenNoPropertyAndNoHtsToggle() throws Exception { + created = create(uniqueTable("no_ramp"), Collections.emptyMap()); + RequestAndValidateHelper.createTableAndValidateResponse(created, mvc, storageManager); + getTable() + .andExpect(status().isOk()) + .andExpect(jsonPath("$.config['" + CONFIG_KEY + "']").doesNotExist()); + } + + @Test + public void get_stampsWhenHtsToggleActiveAndNoProperty() throws Exception { + String tableId = uniqueTable("hts_on"); + created = create(tableId, Collections.emptyMap()); + activateHtsToggle(created); + RequestAndValidateHelper.createTableAndValidateResponse(created, mvc, storageManager); + getTable() + .andExpect(status().isOk()) + .andExpect(jsonPath("$.config['" + CONFIG_KEY + "']", is("\"US\""))); + } + + @Test + public void get_propertyFalseOptsOutEvenWhenHtsToggleActive() throws Exception { + created = + create(uniqueTable("hts_on_prop_off"), Collections.singletonMap(ENABLED_PROP, "false")); + activateHtsToggle(created); + RequestAndValidateHelper.createTableAndValidateResponse(created, mvc, storageManager); + getTable() + .andExpect(status().isOk()) + .andExpect(jsonPath("$.config['" + CONFIG_KEY + "']").doesNotExist()); + } + + @Test + public void get_unparseablePropertyFailsClosedEvenIfHtsActive() throws Exception { + created = create(uniqueTable("bad_prop"), Collections.singletonMap(ENABLED_PROP, "sometimes")); + activateHtsToggle(created); + RequestAndValidateHelper.createTableAndValidateResponse(created, mvc, storageManager); + getTable() + .andExpect(status().isOk()) + .andExpect(jsonPath("$.config['" + CONFIG_KEY + "']").doesNotExist()); + } + + private void activateHtsToggle(GetTableResponseBody table) { + toggleStatus = + TableToggleStatus.builder() + .featureId(ReadBridgeConfigResolver.COLUMN_DEFAULT_FEATURE_ID) + .databaseId(table.getDatabaseId()) + .tableId(table.getTableId()) + .toggleStatusEnum(ToggleStatus.StatusEnum.ACTIVE) + .build(); + toggleStatusesRepository.save(toggleStatus); + } + + private static GetTableResponseBody create(String tableId, Map extraProps) { + Map props = new HashMap<>(GET_TABLE_RESPONSE_BODY.getTableProperties()); + props.putAll(extraProps); + return GET_TABLE_RESPONSE_BODY + .toBuilder() + .tableId(tableId) + .tableUri(CLUSTER_NAME + ".d1." + tableId) + .tableProperties(props) + .build(); + } + + private static String uniqueTable(String suffix) { + return "rbcd_" + suffix + "_" + UUID.randomUUID().toString().substring(0, 8); + } + + private ResultActions getTable() throws Exception { + return mvc.perform( + MockMvcRequestBuilders.get( + String.format( + ValidationUtilities.CURRENT_MAJOR_VERSION_PREFIX + "/databases/%s/tables/%s", + created.getDatabaseId(), + created.getTableId())) + .accept(MediaType.APPLICATION_JSON)); + } +} From ea956d973a6043691e1f4a47a5cf5d789d38574b Mon Sep 17 00:00:00 2001 From: Christian Bush Date: Wed, 12 Aug 2026 15:56:31 -0700 Subject: [PATCH 7/7] Fail closed when the column-defaults source cannot bind. --- .../tables/readbridge/ColumnDefaultsSource.java | 4 ++-- .../readbridge/ReadBridgeConfigResolver.java | 12 +++++++++++- .../ReadBridgeConfigResolverTest.java | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java index 5793d1a78..55f468d70 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ColumnDefaultsSource.java @@ -15,8 +15,8 @@ public interface ColumnDefaultsSource { ColumnDefaultsSource NONE = tableDto -> Collections.emptyMap(); /** - * @return field-id → default JSON; empty/null means nothing to stamp. Throw if a declared default - * cannot bind (do not silently omit). + * Field-id → Iceberg single-value JSON. Empty/null stamps nothing. Omit a field that cannot bind + * (today's NULL); do not throw — this is the table-load path. */ Map defaults(TableDto tableDto); } diff --git a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java index e0042ac6a..6de94c6e4 100644 --- a/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java +++ b/services/tables/src/main/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolver.java @@ -46,7 +46,17 @@ private Map columnDefaultConfig(TableDto tableDto) { if (!isColumnDefaultRamped(tableDto)) { return Collections.emptyMap(); } - Map columnDefaults = columnDefaultsSource.defaults(tableDto); + Map columnDefaults; + try { + columnDefaults = columnDefaultsSource.defaults(tableDto); + } catch (RuntimeException e) { + log.warn( + "read-bridge: column-defaults source failed for {}.{}; treating as not bridged", + tableDto.getDatabaseId(), + tableDto.getTableId(), + e); + return Collections.emptyMap(); + } if (columnDefaults == null || columnDefaults.isEmpty()) { return Collections.emptyMap(); } diff --git a/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java b/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java index 9e2140924..f7a891544 100644 --- a/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java +++ b/services/tables/src/test/java/com/linkedin/openhouse/tables/readbridge/ReadBridgeConfigResolverTest.java @@ -100,6 +100,23 @@ public boolean isFeatureActivated(String databaseId, String tableId, String feat Assertions.assertTrue(config.isEmpty()); } + /** + * A buggy deployment source must not 500 GET. Not bridging is today's NULL, same as a toggle + * outage. + */ + @Test + public void testSourceFailureDegradesInsteadOfFailingTheRead() { + ColumnDefaultsSource exploding = + tableDto -> { + throw new IllegalStateException("encoder exploded"); + }; + + Map config = + resolverFor(exploding).resolve(TableDto.builder().databaseId("db").tableId("tbl").build()); + + Assertions.assertTrue(config.isEmpty()); + } + /** Gate 3: a table the ramp has not activated is not bridged, and its source is never asked. */ @Test public void testUnrampedTableIsNotBridgedAndSourceNotConsulted() {