LibSQL+SQLServer: Return a NonnullRefPtr from Database::get_schema

Database::get_schema currently either returns a RefPtr to an existing
schema, a nullptr if the schema doesn't exist, or an Error if some
internal error occured. Change this to return a NonnullRefPtr to an
exisiting schema, or a SQL::Result with any error, including if the
schema was not found. Callers can then handle that specific error code
if they want.

Returning a NonnullRefPtr will enable some further cleanup. This had
some fallout of needing to change some other methods' return types from
AK::ErrorOr to SQL::Result so that TRY may continue to be used.

Tests/LibSQL/TestSqlDatabase.cpp

@@ -172,7 +172,6 @@ TEST_CASE(get_schema_from_database)
         EXPECT(!db->open().is_error());
         auto schema_or_error = db->get_schema("TestSchema");
         EXPECT(!schema_or_error.is_error());
-        EXPECT(schema_or_error.value());
     }
 }
 

Tests/LibSQL/TestSqlStatementExecution.cpp

@@ -71,7 +71,6 @@ TEST_CASE(create_schema)
     create_schema(database);
     auto schema_or_error = database->get_schema("TESTSCHEMA");
     EXPECT(!schema_or_error.is_error());
-    EXPECT(schema_or_error.value());
 }
 
 TEST_CASE(create_table)

Userland/Libraries/LibSQL/AST/CreateSchema.cpp

@@ -12,17 +12,13 @@ namespace SQL::AST {
 
 ResultOr<ResultSet> CreateSchema::execute(ExecutionContext& context) const
 {
-    auto schema_def = TRY(context.database->get_schema(m_schema_name));
+    auto schema_def = SchemaDef::construct(m_schema_name);
 
-    if (schema_def) {
-        if (m_is_error_if_schema_exists)
-            return Result { SQLCommand::Create, SQLErrorCode::SchemaExists, m_schema_name };
-        return ResultSet { SQLCommand::Create };
+    if (auto result = context.database->add_schema(*schema_def); result.is_error()) {
+        if (result.error().error() != SQLErrorCode::SchemaExists || m_is_error_if_schema_exists)
+            return result.release_error();
     }
 
-    schema_def = SchemaDef::construct(m_schema_name);
-    TRY(context.database->add_schema(*schema_def));
-
     return ResultSet { SQLCommand::Create };
 }
 

Userland/Libraries/LibSQL/AST/CreateTable.cpp

@@ -11,13 +11,8 @@ namespace SQL::AST {
 
 ResultOr<ResultSet> CreateTable::execute(ExecutionContext& context) const
 {
-    auto schema_name = m_schema_name.is_empty() ? String { "default"sv } : m_schema_name;
-
-    auto schema_def = TRY(context.database->get_schema(schema_name));
-    if (!schema_def)
-        return Result { SQLCommand::Create, SQLErrorCode::SchemaDoesNotExist, schema_name };
-
-    auto table_def = TRY(context.database->get_table(schema_name, m_table_name));
+    auto schema_def = TRY(context.database->get_schema(m_schema_name));
+    auto table_def = TRY(context.database->get_table(m_schema_name, m_table_name));
     if (table_def) {
         if (m_is_error_if_table_exists)
             return Result { SQLCommand::Create, SQLErrorCode::TableExists, m_table_name };

Userland/Libraries/LibSQL/Database.cpp

@@ -24,9 +24,10 @@ Database::Database(String name)
 {
 }
 
-ErrorOr<void> Database::open()
+ResultOr<void> Database::open()
 {
     TRY(m_heap->open());
+
     m_schemas = BTree::construct(m_serializer, SchemaDef::index_def()->to_tuple_descriptor(), m_heap->schemas_root());
     m_schemas->on_new_root = [&]() {
         m_heap->set_schemas_root(m_schemas->root());
@@ -43,17 +44,22 @@ ErrorOr<void> Database::open()
     };
 
     m_open = true;
-    auto default_schema = TRY(get_schema("default"));
-    if (!default_schema) {
-        default_schema = SchemaDef::construct("default");
-        TRY(add_schema(*default_schema));
-    }
 
-    auto master_schema = TRY(get_schema("master"));
-    if (!master_schema) {
-        master_schema = SchemaDef::construct("master");
-        TRY(add_schema(*master_schema));
-    }
+    auto ensure_schema_exists = [&](auto schema_name) -> ResultOr<NonnullRefPtr<SchemaDef>> {
+        if (auto result = get_schema(schema_name); result.is_error()) {
+            if (result.error().error() != SQLErrorCode::SchemaDoesNotExist)
+                return result.release_error();
+
+            auto schema_def = SchemaDef::construct(schema_name);
+            TRY(add_schema(*schema_def));
+            return schema_def;
+        } else {
+            return result.release_value();
+        }
+    };
+
+    (void)TRY(ensure_schema_exists("default"sv));
+    auto master_schema = TRY(ensure_schema_exists("master"sv));
 
     auto table_def = TRY(get_table("master", "internal_describe_table"));
     if (!table_def) {
@@ -75,13 +81,12 @@ ErrorOr<void> Database::commit()
     return {};
 }
 
-ErrorOr<void> Database::add_schema(SchemaDef const& schema)
+ResultOr<void> Database::add_schema(SchemaDef const& schema)
 {
     VERIFY(is_open());
-    if (!m_schemas->insert(schema.key())) {
-        warnln("Duplicate schema name {}"sv, schema.name());
-        return Error::from_string_literal("Duplicate schema name");
-    }
+
+    if (!m_schemas->insert(schema.key()))
+        return Result { SQLCommand::Unknown, SQLErrorCode::SchemaExists, schema.name() };
     return {};
 }
 
@@ -92,24 +97,25 @@ Key Database::get_schema_key(String const& schema_name)
     return key;
 }
 
-ErrorOr<RefPtr<SchemaDef>> Database::get_schema(String const& schema)
+ResultOr<NonnullRefPtr<SchemaDef>> Database::get_schema(String const& schema)
 {
     VERIFY(is_open());
+
     auto schema_name = schema;
-    if (schema.is_null() || schema.is_empty())
-        schema_name = "default";
+    if (schema.is_empty())
+        schema_name = "default"sv;
+
     Key key = get_schema_key(schema_name);
-    auto schema_def_opt = m_schema_cache.get(key.hash());
-    if (schema_def_opt.has_value()) {
-        return RefPtr<SchemaDef>(schema_def_opt.value());
-    }
+    if (auto it = m_schema_cache.find(key.hash()); it != m_schema_cache.end())
+        return it->value;
+
     auto schema_iterator = m_schemas->find(key);
-    if (schema_iterator.is_end() || (*schema_iterator != key)) {
-        return RefPtr<SchemaDef>(nullptr);
-    }
-    auto ret = SchemaDef::construct(*schema_iterator);
-    m_schema_cache.set(key.hash(), ret);
-    return RefPtr<SchemaDef>(ret);
+    if (schema_iterator.is_end() || (*schema_iterator != key))
+        return Result { SQLCommand::Unknown, SQLErrorCode::SchemaDoesNotExist, schema_name };
+
+    auto schema_def = SchemaDef::construct(*schema_iterator);
+    m_schema_cache.set(key.hash(), schema_def);
+    return schema_def;
 }
 
 ErrorOr<void> Database::add_table(TableDef& table)
@@ -132,7 +138,7 @@ Key Database::get_table_key(String const& schema_name, String const& table_name)
     return key;
 }
 
-ErrorOr<RefPtr<TableDef>> Database::get_table(String const& schema, String const& name)
+ResultOr<RefPtr<TableDef>> Database::get_table(String const& schema, String const& name)
 {
     VERIFY(is_open());
     auto schema_name = schema;
@@ -147,10 +153,6 @@ ErrorOr<RefPtr<TableDef>> Database::get_table(String const& schema, String const
         return RefPtr<TableDef>(nullptr);
     }
     auto schema_def = TRY(get_schema(schema));
-    if (!schema_def) {
-        warnln("Schema '{}' does not exist"sv, schema);
-        return Error::from_string_literal("Schema does not exist");
-    }
     auto ret = TableDef::construct(schema_def, name);
     ret->set_pointer((*table_iterator).pointer());
     m_table_cache.set(key.hash(), ret);

Userland/Libraries/LibSQL/Database.h

@@ -13,6 +13,7 @@
 #include <LibSQL/Forward.h>
 #include <LibSQL/Heap.h>
 #include <LibSQL/Meta.h>
+#include <LibSQL/Result.h>
 #include <LibSQL/Serializer.h>
 
 namespace SQL {
@@ -28,17 +29,17 @@ class Database : public Core::Object {
 public:
     ~Database() override;
 
-    ErrorOr<void> open();
+    ResultOr<void> open();
     bool is_open() const { return m_open; }
     ErrorOr<void> commit();
 
-    ErrorOr<void> add_schema(SchemaDef const&);
+    ResultOr<void> add_schema(SchemaDef const&);
     static Key get_schema_key(String const&);
-    ErrorOr<RefPtr<SchemaDef>> get_schema(String const&);
+    ResultOr<NonnullRefPtr<SchemaDef>> get_schema(String const&);
 
     ErrorOr<void> add_table(TableDef& table);
     static Key get_table_key(String const&, String const&);
-    ErrorOr<RefPtr<TableDef>> get_table(String const&, String const&);
+    ResultOr<RefPtr<TableDef>> get_table(String const&, String const&);
 
     ErrorOr<Vector<Row>> select_all(TableDef const&);
     ErrorOr<Vector<Row>> match(TableDef const&, Key const&);
@@ -55,7 +56,7 @@ private:
     RefPtr<BTree> m_tables;
     RefPtr<BTree> m_table_columns;
 
-    HashMap<u32, RefPtr<SchemaDef>> m_schema_cache;
+    HashMap<u32, NonnullRefPtr<SchemaDef>> m_schema_cache;
     HashMap<u32, RefPtr<TableDef>> m_table_cache;
 };
 

Userland/Services/SQLServer/DatabaseConnection.cpp

@@ -41,7 +41,7 @@ DatabaseConnection::DatabaseConnection(String database_name, int client_id)
         m_database = SQL::Database::construct(String::formatted("/home/anon/sql/{}.db", m_database_name));
         auto client_connection = ConnectionFromClient::client_connection_for(m_client_id);
         if (auto maybe_error = m_database->open(); maybe_error.is_error()) {
-            client_connection->async_connection_error(m_connection_id, (int)SQL::SQLErrorCode::InternalError, maybe_error.error().string_literal());
+            client_connection->async_connection_error(m_connection_id, to_underlying(maybe_error.error().error()), maybe_error.error().error_string());
             return;
         }
         m_accept_statements = true;