Skip to content

fix(ebean-dao): do not cache empty schema loads (SharedSchemaCache poisoning, #618 regression) #625

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
NatalliaUlashchick wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(ebean-dao): do not cache empty schema loads (SharedSchemaCache poisoning, #618 regression) #625

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
49 changes: 27 additions & 22 deletions dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SchemaValidatorUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,8 @@ public boolean columnExists(@Nonnull String tableName, @Nonnull String columnNam
if (sharedCache != null) {
return sharedCache.columnExists(tableName, columnName);
}
String lowerTable = tableName.toLowerCase();
String lowerColumn = columnName.toLowerCase();

Set<String> columns = columnCache.get(lowerTable, tbl -> {
log.info("Refreshing column cache for table '{}'", tbl);
return loadColumns(tbl);
});

return columns.contains(lowerColumn);
return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "column", this::loadColumns)
.contains(columnName.toLowerCase());
}

/**
Expand All @@ -141,11 +134,7 @@ public Set<String> getColumns(@Nonnull String tableName) {
if (sharedCache != null) {
return sharedCache.getColumns(tableName);
}
String lowerTable = tableName.toLowerCase();
return columnCache.get(lowerTable, tbl -> {
log.info("Refreshing column cache for table '{}'", tbl);
return loadColumns(tbl);
});
return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "column", this::loadColumns);
}

/**
Expand All @@ -159,15 +148,31 @@ public boolean indexExists(@Nonnull String tableName, @Nonnull String indexName)
if (sharedCache != null) {
return sharedCache.indexExists(tableName, indexName);
}
String lowerTable = tableName.toLowerCase();
String lowerIndex = indexName.toLowerCase();

Set<String> indexes = indexCache.get(lowerTable, tbl -> {
log.info("Refreshing index cache for table '{}'", tbl);
return loadIndexes(tbl);
});
return getCachingNonEmpty(indexCache, tableName.toLowerCase(), "index", this::loadIndexes)
.contains(indexName.toLowerCase());
}

return indexes.contains(lowerIndex);
/**
* Returns the cached set for {@code lowerTable}, loading it on a cache miss. A non-empty result is
* cached; an empty result is NOT cached. For columns and indexes an empty result means the table
* is not present yet (e.g. schema not migrated when this ran), so pinning it for the TTL would
* make {@code columnExists()}/{@code indexExists()} report false for entries that do exist. Not
* caching it lets a later call re-resolve. Mirrors the fix in {@link SharedSchemaCache} for the
* same regression (#618).
*/
@Nonnull
private Set<String> getCachingNonEmpty(@Nonnull Cache<String, Set<String>> cache, @Nonnull String lowerTable,
@Nonnull String what, @Nonnull java.util.function.Function<String, Set<String>> loader) {
Set<String> cached = cache.getIfPresent(lowerTable);
if (cached != null) {
return cached;
}
log.info("Refreshing {} cache for table '{}'", what, lowerTable);
Set<String> loaded = loader.apply(lowerTable);
if (!loaded.isEmpty()) {
cache.put(lowerTable, loaded);
}
return loaded;
}


Expand Down
63 changes: 49 additions & 14 deletions dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SharedSchemaCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import lombok.extern.slf4j.Slf4j;
Expand Down Expand Up @@ -98,25 +99,45 @@ public void registerAndPreWarm(@Nonnull String tableName) {
// ── cache access ─────────────────────────────────────────────────────────────

public boolean columnExists(@Nonnull String tableName, @Nonnull String columnName) {
return columnCache.get(tableName.toLowerCase(), tbl -> {
log.info("Inline schema cache miss: loading columns for table '{}'", tbl);
return loadColumns(tbl);
}).contains(columnName.toLowerCase());
return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "columns", this::loadColumns)
.contains(columnName.toLowerCase());
}

@Nonnull
public Set<String> getColumns(@Nonnull String tableName) {
return columnCache.get(tableName.toLowerCase(), tbl -> {
log.info("Inline schema cache miss: loading columns for table '{}'", tbl);
return loadColumns(tbl);
});
return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "columns", this::loadColumns);
}

public boolean indexExists(@Nonnull String tableName, @Nonnull String indexName) {
return indexCache.get(tableName.toLowerCase(), tbl -> {
log.info("Inline schema cache miss: loading indexes for table '{}'", tbl);
return loadIndexes(tbl);
}).contains(indexName.toLowerCase());
return getCachingNonEmpty(indexCache, tableName.toLowerCase(), "indexes", this::loadIndexes)
.contains(indexName.toLowerCase());
}

/**
* Returns the cached metadata for {@code lowerTable}, loading it on a cache miss. A non-empty
* result is cached; an <em>empty</em> result is deliberately NOT cached. For columns and indexes
* an empty result means the table does not exist yet — e.g. the pre-warm in the
* {@link com.linkedin.metadata.dao.EbeanLocalAccess} constructor ran before schema migration
* created the table. Caching that empty set would pin a stale "table has nothing" view for the
* full {@value #CACHE_TTL_MINUTES}-minute TTL, so every later {@code columnExists()} returns false
* and {@code EbeanLocalAccess.batchGetUnion} drops the aspect from its query — surfacing as 404s
* on aspect reads, wrong autofill defaults, lost writes and wrong row counts (regression #618).
* Not caching the empty result lets a later call re-resolve once the schema is in place; for a
* genuinely absent table the extra information_schema query is cheap and self-correcting.
*/
@Nonnull
private static Set<String> getCachingNonEmpty(@Nonnull Cache<String, Set<String>> cache,
@Nonnull String lowerTable, @Nonnull String what, @Nonnull Function<String, Set<String>> loader) {
Set<String> cached = cache.getIfPresent(lowerTable);
if (cached != null) {
return cached;
}
log.info("Inline schema cache miss: loading {} for table '{}'", what, lowerTable);
Set<String> loaded = loader.apply(lowerTable);
if (!loaded.isEmpty()) {
cache.put(lowerTable, loaded);
}
return loaded;
}

@Nullable
Expand Down Expand Up @@ -159,15 +180,29 @@ private void refreshAll() {

private void refreshTable(String tableName) {
try {
columnCache.put(tableName, loadColumns(tableName));
indexCache.put(tableName, loadIndexes(tableName));
// Only cache non-empty column/index metadata: an empty result means the table does not exist
// yet (e.g. pre-warm runs before schema migration), and pinning that stale view for the TTL is
// the #618 regression. Index expressions may be legitimately empty (a table with no expression
// indexes), so they are cached as-is.
putIfNonEmpty(columnCache, tableName, loadColumns(tableName));
putIfNonEmpty(indexCache, tableName, loadIndexes(tableName));
indexExpressionCache.put(tableName, loadIndexesAndExpressions(tableName));
log.info("Schema cache refreshed for table '{}'", tableName);
} catch (Exception e) {
log.warn("Schema cache refresh failed for table '{}': {}", tableName, e.getMessage());
}
}

private static void putIfNonEmpty(@Nonnull Cache<String, Set<String>> cache, @Nonnull String tableName,
@Nonnull Set<String> value) {
if (value.isEmpty()) {
// Drop any previously-cached empty/absent entry so the next access re-resolves.
cache.invalidate(tableName);
} else {
cache.put(tableName, value);
}
}

// ── DB loaders ────────────────────────────────────────────────────────────────

private Set<String> loadColumns(String tableName) {
Expand Down
58 changes: 58 additions & 0 deletions dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/SharedSchemaCacheTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,64 @@ public void testSingletonPerUrl() {
assertNotSame(cache, different);
}

/**
* Regression test for #618 (datahub-gma 0.6.185): the SharedSchemaCache pre-warm was moved into
* the EbeanLocalAccess constructor so it always runs at startup. For consumers that run schema
* migrations as a separate job (LinkedIn's prod pattern) or in integration tests, the
* constructor — and therefore the pre-warm — can execute BEFORE the entity table is migrated
* into existence. loadColumns() then returns an empty set, and caching that empty set pinned a
* stale "table has no columns" view for the full TTL: every later columnExists() returned false,
* so EbeanLocalAccess.batchGetUnion dropped the aspect from its query and reads silently returned
* nothing (404 / wrong autofill defaults / lost writes / wrong counts).
*
* <p>An empty column load means "table not present yet" and must NOT be cached, so a later access
* re-resolves once the schema is in place.</p>
*/
@Test
public void testPreWarmBeforeTableCreatedDoesNotPoisonColumnCache() {
final String table = "metadata_entity_prewarm_regression";
server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table));

// Pre-warm while the table does NOT yet exist (mimics constructor pre-warm before migration).
cache.registerAndPreWarm(table);
assertFalse(cache.columnExists(table, "a_aspectfoo"));

// Schema migration runs AFTER construction: the table + aspect column now exist.
server.execute(Ebean.createSqlUpdate(
"CREATE TABLE " + table + " (urn VARCHAR(100) NOT NULL, a_aspectfoo JSON, "
+ "CONSTRAINT pk_" + table + " PRIMARY KEY (urn))"));

// The column added after pre-warm must now be visible — the empty pre-warm result must not be
// pinned. Before the fix this returned false because the empty set was cached for the TTL.
assertTrue("column added after pre-warm must be visible; empty pre-warm load must not be cached (regression #618)",
cache.columnExists(table, "a_aspectfoo"));

server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table));
}

/**
* Same #618 regression as {@link #testPreWarmBeforeTableCreatedDoesNotPoisonColumnCache} but via
* the lazy (cache-miss) read path rather than registerAndPreWarm: a columnExists() call issued
* before the table exists must not pin an empty result.
*/
@Test
public void testLazyReadBeforeTableCreatedDoesNotPoisonColumnCache() {
final String table = "metadata_entity_lazy_regression";
server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table));

// Lazy read while the table does NOT yet exist.
assertFalse(cache.columnExists(table, "a_aspectfoo"));

server.execute(Ebean.createSqlUpdate(
"CREATE TABLE " + table + " (urn VARCHAR(100) NOT NULL, a_aspectfoo JSON, "
+ "CONSTRAINT pk_" + table + " PRIMARY KEY (urn))"));

assertTrue("column visible after table creation; prior empty read must not be cached (regression #618)",
cache.columnExists(table, "a_aspectfoo"));

server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table));
}

/**
* MariaDB does not support the EXPRESSION column in information_schema.STATISTICS, so we mock
* the DB response to exercise the getIndexExpression happy path.
Expand Down
Loading