feat: refactor getting tie_break_offset in SelectBestMatch (duckdb#15235

)

We have a case where we need access to the tie break offset of different
secret storages when we do `LookupSecrets` in one secret storage (the
one on the server side). This refactor adds "tie break offset" of a
secret storage to the parameter list of `SelectBestMatch` method so that
this function can be more flexibly used without needing to rely on the
class it belongs to (GetTiebreakOffset()). As a result, I'm also
changing this function to `static` (still `protected`) in SecretStorage
to make it clear that it's not tied to any class instance and adding
`tie_break_offset` to be a const parameter when instantiating a
SecretStorage class to ensure that this value doesn't get modified after
initialization.

src/include/duckdb/main/secret/secret_storage.hpp

@@ -8,8 +8,13 @@
 
 #pragma once
 
+#include "duckdb/catalog/catalog.hpp"
+#include "duckdb/catalog/catalog_set.hpp"
 #include "duckdb/common/common.hpp"
+#include "duckdb/common/enums/on_create_conflict.hpp"
 #include "duckdb/common/enums/on_entry_not_found.hpp"
+#include "duckdb/common/optional_ptr.hpp"
+#include "duckdb/common/case_insensitive_map.hpp"
 
 namespace duckdb {
 
@@ -20,15 +25,21 @@ struct CatalogTransaction;
 class DatabaseInstance;
 struct SecretMatch;
 struct SecretEntry;
+class SecretManager;
 
 //! Base class for SecretStorage API
 class SecretStorage {
 	friend class SecretManager;
 
 public:
-	explicit SecretStorage(const string &name) : storage_name(name), persistent(false) {};
+	explicit SecretStorage(const string &name, const int64_t tie_break_offset)
+	    : storage_name(name), tie_break_offset(tie_break_offset), persistent(false) {};
 	virtual ~SecretStorage() = default;
 
+	//! Default storage backend offsets
+	static const int64_t TEMPORARY_STORAGE_OFFSET = 10;
+	static const int64_t LOCAL_FILE_STORAGE_OFFSET = 20;
+
 public:
 	//! SecretStorage API
 
@@ -52,9 +63,6 @@ class SecretStorage {
 	virtual unique_ptr<SecretEntry> GetSecretByName(const string &name,
 	                                                optional_ptr<CatalogTransaction> transaction = nullptr) = 0;
 
-	//! Return the offset associated to this storage for tie-breaking secrets between storages
-	virtual int64_t GetTieBreakOffset() = 0;
-
 	//! Returns include_in_lookups, used to create secret storage
 	virtual bool IncludeInLookups() {
 		return true;
@@ -67,16 +75,13 @@ class SecretStorage {
 protected:
 	//! Helper function to select the best matching secret within a storage. Tie-breaks within a storage are broken
 	//! by secret name by default.
-	SecretMatch SelectBestMatch(SecretEntry &secret_entry, const string &path, SecretMatch &current_best);
-
-	//! Offsets the score to tie-break secrets giving preference to the storage with the lowest storage_penalty
-	//! the base implementation will be chosen last in a tie-break
-	int64_t OffsetMatchScore(int64_t score) {
-		return 100 * score - GetTieBreakOffset();
-	}
+	static SecretMatch SelectBestMatch(SecretEntry &secret_entry, const string &path, int64_t offset,
+	                                   SecretMatch &current_best);
 
 	//! Name of the storage backend (e.g. temporary, file, etc)
 	string storage_name;
+	//! Offset associated to this storage for tie-breaking secrets between storages
+	const int64_t tie_break_offset;
 	//! Whether entries in this storage will survive duckdb reboots
 	bool persistent;
 };
@@ -94,8 +99,8 @@ struct SecretCatalogEntry : public InCatalogEntry {
 //! Base Implementation for catalog set based secret storage
 class CatalogSetSecretStorage : public SecretStorage {
 public:
-	CatalogSetSecretStorage(DatabaseInstance &db_instance, const string &name_p)
-	    : SecretStorage(name_p), db(db_instance) {};
+	CatalogSetSecretStorage(DatabaseInstance &db_instance, const string &name_p, const int64_t offset)
+	    : SecretStorage(name_p, offset), db(db_instance) {};
 
 public:
 	//! SecretStorage API
@@ -128,15 +133,12 @@ class CatalogSetSecretStorage : public SecretStorage {
 
 class TemporarySecretStorage : public CatalogSetSecretStorage {
 public:
-	TemporarySecretStorage(const string &name_p, DatabaseInstance &db_p) : CatalogSetSecretStorage(db_p, name_p) {
+	TemporarySecretStorage(const string &name_p, DatabaseInstance &db_p)
+	    : CatalogSetSecretStorage(db_p, name_p, TEMPORARY_STORAGE_OFFSET) {
 		secrets = make_uniq<CatalogSet>(Catalog::GetSystemCatalog(db));
 		persistent = false;
 	}
 
-	int64_t GetTieBreakOffset() override {
-		return 10;
-	}
-
 protected:
 };
 
@@ -145,10 +147,6 @@ class LocalFileSecretStorage : public CatalogSetSecretStorage {
 	LocalFileSecretStorage(SecretManager &manager, DatabaseInstance &db, const string &name_p,
 	                       const string &secret_path);
 
-	int64_t GetTieBreakOffset() override {
-		return 20;
-	}
-
 protected:
 	//! Implements the writes to disk
 	void WriteSecret(const BaseSecret &secret, OnCreateConflict on_conflict) override;

src/main/secret/secret_manager.cpp

@@ -80,7 +80,7 @@ void SecretManager::LoadSecretStorageInternal(unique_ptr<SecretStorage> storage)
 
 	// Check for tie-break offset collisions to ensure we can always tie-break cleanly
 	for (const auto &storage_ptr : secret_storages) {
-		if (storage_ptr.second->GetTieBreakOffset() == storage->GetTieBreakOffset()) {
+		if (storage_ptr.second->tie_break_offset == storage->tie_break_offset) {
 			throw InternalException("Failed to load secret storage '%s', tie break score collides with '%s'",
 			                        storage->GetName(), storage_ptr.second->GetName());
 		}

src/main/secret/secret_storage.cpp

@@ -14,7 +14,8 @@
 
 namespace duckdb {
 
-SecretMatch SecretStorage::SelectBestMatch(SecretEntry &secret_entry, const string &path, SecretMatch &current_best) {
+SecretMatch SecretStorage::SelectBestMatch(SecretEntry &secret_entry, const string &path, int64_t offset,
+                                           SecretMatch &current_best) {
 	// Get secret match score
 	auto match_score = secret_entry.secret->MatchScore(path);
 
@@ -27,7 +28,7 @@ SecretMatch SecretStorage::SelectBestMatch(SecretEntry &secret_entry, const stri
 	D_ASSERT(match_score >= 0);
 
 	// Apply storage tie-break offset
-	match_score = OffsetMatchScore(match_score);
+	match_score = 100 * match_score - offset;
 
 	// Choose the best matching score, tie-breaking on secret name when necessary
 	if (match_score > current_best.score) {
@@ -105,7 +106,7 @@ SecretMatch CatalogSetSecretStorage::LookupSecret(const string &path, const stri
 	const std::function<void(CatalogEntry &)> callback = [&](CatalogEntry &entry) {
 		auto &cast_entry = entry.Cast<SecretCatalogEntry>();
 		if (StringUtil::CIEquals(cast_entry.secret->secret->GetType(), type)) {
-			best_match = SelectBestMatch(*cast_entry.secret, path, best_match);
+			best_match = SelectBestMatch(*cast_entry.secret, path, tie_break_offset, best_match);
 		}
 	};
 	secrets->Scan(GetTransactionOrDefault(transaction), callback);
@@ -131,7 +132,8 @@ unique_ptr<SecretEntry> CatalogSetSecretStorage::GetSecretByName(const string &n
 
 LocalFileSecretStorage::LocalFileSecretStorage(SecretManager &manager, DatabaseInstance &db_p, const string &name_p,
                                                const string &secret_path_p)
-    : CatalogSetSecretStorage(db_p, name_p), secret_path(FileSystem::ExpandPath(secret_path_p, nullptr)) {
+    : CatalogSetSecretStorage(db_p, name_p, LOCAL_FILE_STORAGE_OFFSET),
+      secret_path(FileSystem::ExpandPath(secret_path_p, nullptr)) {
 	persistent = true;
 
 	// Check existence of persistent secret dir

test/secrets/test_custom_secret_storage.cpp

@@ -43,7 +43,7 @@ struct DemoSecretType {
 class TestSecretStorage : public CatalogSetSecretStorage {
 public:
 	TestSecretStorage(const string &name_p, DatabaseInstance &db, TestSecretLog &logger_p, int64_t tie_break_offset_p)
-	    : CatalogSetSecretStorage(db, name_p), tie_break_offset(tie_break_offset_p), logger(logger_p) {
+	    : CatalogSetSecretStorage(db, name_p, tie_break_offset_p), logger(logger_p) {
 		secrets = make_uniq<CatalogSet>(Catalog::GetSystemCatalog(db));
 		persistent = true;
 		include_in_lookups = true;
@@ -52,11 +52,6 @@ class TestSecretStorage : public CatalogSetSecretStorage {
 		return include_in_lookups;
 	}
 
-	int64_t GetTieBreakOffset() override {
-		return tie_break_offset;
-	}
-
-	int64_t tie_break_offset;
 	bool include_in_lookups;
 
 protected: