Don't make storage.sql optional; have methods throw instead.

Having `storage.sql` be optional is potentially annoying for two reasons:
- TypeScript will force people to check if it's present, even though apps that have configured it should be able to expect it's always present.
- We'd like to provide a more detailed error message telling people how to configure SQL.

So, this commit changes things so `storage.sql` is always present, but its methods will throw exceptions if the DO isn't SQLite-backed.

src/workerd/api/actor-state.c++

@@ -16,6 +16,7 @@
 #include "sql.h"
 #include <workerd/api/web-socket.h>
 #include <workerd/io/hibernation-manager.h>
+#include <workerd/io/features.h>
 
 namespace workerd::api {
 
@@ -700,37 +701,51 @@ jsg::Promise<void> DurableObjectStorage::sync(jsg::Lock& js) {
   }
 }
 
-jsg::Optional<jsg::Ref<SqlStorage>> DurableObjectStorage::getSql(
-    jsg::Lock& js, CompatibilityFlags::Reader flags) {
+SqliteDatabase& DurableObjectStorage::getSqliteDb(jsg::Lock& js) {
   KJ_IF_SOME(db, cache->getSqliteDatabase()) {
     // Actor is SQLite-backed but let's make sure SQL is configured to be enabled.
-    if (!enableSql) {
+    if (enableSql) {
+      return db;
+    } else if (FeatureFlags::get(js).getWorkerdExperimental()) {
       // For backwards-compatibility, if the `experimental` compat flag is on, enable SQL. This is
       // deprecated, though, so warn in this case.
-      if (flags.getWorkerdExperimental()) {
-        // TODO(soon): Uncomment this warning after the D1 simulator has been updated to use
-        //   `enableSql`. Otherwise, people doing local dev against D1 may see the warning
-        //   spuriously.
-
-        // IoContext::current().logWarningOnce(
-        //     "Enabling SQL API based on the 'experimental' flag, but this will stop working soon. "
-        //     "Instead, please set `enableSql = true` in your workerd config for the DO namespace. "
-        //     "If using wrangler, under `[[migrations]]` in wrangler.toml, change `new_classes` to "
-        //     "`new_sqlite_classes`.");
-      } else {
-        // SQL is not enabled at all.
-        return kj::none;
-      }
-    }
 
-    return jsg::alloc<SqlStorage>(db, JSG_THIS);
+      // TODO(soon): Uncomment this warning after the D1 simulator has been updated to use
+      //   `enableSql`. Otherwise, people doing local dev against D1 may see the warning
+      //   spuriously.
+
+      // IoContext::current().logWarningOnce(
+      //     "Enabling SQL API based on the 'experimental' flag, but this will stop working soon. "
+      //     "Instead, please set `enableSql = true` in your workerd config for the DO namespace. "
+      //     "If using wrangler, under `[[migrations]]` in wrangler.toml, change `new_classes` to "
+      //     "`new_sqlite_classes`.");
+
+      return db;
+    } else {
+      // We're presumably running local workerd, which always uses SQLite for DO storage, but we're
+      // trying to simulate a non-SQLite DO namespace for testing purposes.
+      JSG_FAIL_REQUIRE(Error,
+          "SQL is not enabled for this Durable Object class. To enable it, set "
+          "`enableSql = true` in your workerd config for the class. If using wrangler, "
+          "under `[[migrations]]` in wrangler.toml, change `new_classes` to "
+          "`new_sqlite_classes`. Note that this change cannot be made after the class is "
+          "already deployed to production.");
+    }
   } else {
-    // Not SQLite-backed.
-    KJ_REQUIRE(!enableSql, "Durable Object is not backed by SQL but enableSql was true?");
-    return kj::none;
+    // We're in production (not local workerd) and this DO namespace is not backed by SQLite.
+    JSG_FAIL_REQUIRE(Error,
+        "This Durable Object is not backed by SQLite storage, so the SQL API is not available. "
+        "SQL can be enabled on a new Durable Object class by using the `new_sqlite_classes` "
+        "instead of `new_classes` under `[[migrations]]` in your wrangler.toml, but an "
+        "already-deployed class cannot be converted to SQLite (except by deleting the existing "
+        "data).");
   }
 }
 
+jsg::Ref<SqlStorage> DurableObjectStorage::getSql(jsg::Lock& js) {
+  return jsg::alloc<SqlStorage>(JSG_THIS);
+}
+
 kj::Promise<kj::String> DurableObjectStorage::getCurrentBookmark() {
   return cache->getCurrentBookmark();
 }

src/workerd/api/actor-state.h

@@ -180,6 +180,9 @@ class DurableObjectStorage: public jsg::Object, public DurableObjectStorageOpera
     return *cache;
   }
 
+  // Throws if not SQLite-backed.
+  SqliteDatabase& getSqliteDb(jsg::Lock& js);
+
   struct TransactionOptions {
     jsg::Optional<kj::Date> asOfTime;
     jsg::Optional<bool> lowPriority;
@@ -201,7 +204,7 @@ class DurableObjectStorage: public jsg::Object, public DurableObjectStorageOpera
 
   jsg::Promise<void> sync(jsg::Lock& js);
 
-  jsg::Optional<jsg::Ref<SqlStorage>> getSql(jsg::Lock& js, CompatibilityFlags::Reader flags);
+  jsg::Ref<SqlStorage> getSql(jsg::Lock& js);
 
   // Get a bookmark for the current state of the database. Note that since this is async, the
   // bookmark will include any writes in the current atomic batch, including writes that are

src/workerd/api/sql.c++

@@ -8,34 +8,33 @@
 
 namespace workerd::api {
 
-SqlStorage::SqlStorage(SqliteDatabase& sqlite, jsg::Ref<DurableObjectStorage> storage)
-    : sqlite(IoContext::current().addObject(sqlite)),
-      storage(kj::mv(storage)) {}
+SqlStorage::SqlStorage(jsg::Ref<DurableObjectStorage> storage): storage(kj::mv(storage)) {}
 
 SqlStorage::~SqlStorage() {}
 
 jsg::Ref<SqlStorage::Cursor> SqlStorage::exec(
     jsg::Lock& js, kj::String querySql, jsg::Arguments<BindingValue> bindings) {
   SqliteDatabase::Regulator& regulator = *this;
-  return jsg::alloc<Cursor>(*sqlite, regulator, querySql, kj::mv(bindings));
+  return jsg::alloc<Cursor>(getDb(js), regulator, querySql, kj::mv(bindings));
 }
 
 SqlStorage::IngestResult SqlStorage::ingest(jsg::Lock& js, kj::String querySql) {
   SqliteDatabase::Regulator& regulator = *this;
-  auto result = sqlite->ingestSql(regulator, querySql);
+  auto result = getDb(js).ingestSql(regulator, querySql);
   return IngestResult(
       kj::str(result.remainder), result.rowsRead, result.rowsWritten, result.statementCount);
 }
 
 jsg::Ref<SqlStorage::Statement> SqlStorage::prepare(jsg::Lock& js, kj::String query) {
-  return jsg::alloc<Statement>(sqlite->prepare(*this, query));
+  return jsg::alloc<Statement>(getDb(js).prepare(*this, query));
 }
 
-double SqlStorage::getDatabaseSize() {
-  int64_t pages = execMemoized(pragmaPageCount,
+double SqlStorage::getDatabaseSize(jsg::Lock& js) {
+  auto& db = getDb(js);
+  int64_t pages = execMemoized(db, pragmaPageCount,
       "select (select * from pragma_page_count) - (select * from pragma_freelist_count);")
                       .getInt64(0);
-  return pages * getPageSize();
+  return pages * getPageSize(db);
 }
 
 bool SqlStorage::isAllowedName(kj::StringPtr name) const {

src/workerd/api/sql.h

@@ -14,7 +14,7 @@ namespace workerd::api {
 
 class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
 public:
-  SqlStorage(SqliteDatabase& sqlite, jsg::Ref<DurableObjectStorage> storage);
+  SqlStorage(jsg::Ref<DurableObjectStorage> storage);
   ~SqlStorage();
 
   using BindingValue = kj::Maybe<kj::OneOf<kj::Array<const byte>, kj::String, double>>;
@@ -28,7 +28,7 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
 
   jsg::Ref<Statement> prepare(jsg::Lock& js, kj::String query);
 
-  double getDatabaseSize();
+  double getDatabaseSize(jsg::Lock& js);
 
   JSG_RESOURCE_TYPE(SqlStorage, CompatibilityFlags::Reader flags) {
     JSG_METHOD(exec);
@@ -58,15 +58,19 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
   void onError(kj::StringPtr message) const override;
   bool allowTransactions() const override;
 
-  IoPtr<SqliteDatabase> sqlite;
+  SqliteDatabase& getDb(jsg::Lock& js) {
+    return storage->getSqliteDb(js);
+  }
+
   jsg::Ref<DurableObjectStorage> storage;
 
   kj::Maybe<uint> pageSize;
   kj::Maybe<IoOwn<SqliteDatabase::Statement>> pragmaPageCount;
   kj::Maybe<IoOwn<SqliteDatabase::Statement>> pragmaGetMaxPageCount;
 
   template <size_t size, typename... Params>
-  SqliteDatabase::Query execMemoized(kj::Maybe<IoOwn<SqliteDatabase::Statement>>& slot,
+  SqliteDatabase::Query execMemoized(SqliteDatabase& db,
+      kj::Maybe<IoOwn<SqliteDatabase::Statement>>& slot,
       const char (&sqlCode)[size],
       Params&&... params) {
     // Run a (trusted) statement, preparing it on the first call and reusing the prepared version
@@ -76,16 +80,16 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
     KJ_IF_SOME(s, slot) {
       stmt = &*s;
     } else {
-      stmt = &*slot.emplace(IoContext::current().addObject(kj::heap(sqlite->prepare(sqlCode))));
+      stmt = &*slot.emplace(IoContext::current().addObject(kj::heap(db.prepare(sqlCode))));
     }
     return stmt->run(kj::fwd<Params>(params)...);
   }
 
-  uint64_t getPageSize() {
+  uint64_t getPageSize(SqliteDatabase& db) {
     KJ_IF_SOME(p, pageSize) {
       return p;
     } else {
-      return pageSize.emplace(sqlite->run("PRAGMA page_size;").getInt64(0));
+      return pageSize.emplace(db.run("PRAGMA page_size;").getInt64(0));
     }
   }
 };