Skip to content

Conversation

@Asphaltian
Copy link

This resolves Facepunch/sbox-issues#9262.

Adds persistent SQLite storage via Microsoft.Data.Sqlite. Databases are stored in the game's data folder (sv.db for server, cl.db for client)

New classes

Sql - Static API
SqlDatabase - SQLite connection wrapper
SqlBuilder - Fluent query builder for dynamic SQL construction
SqlTest - Unit tests

Usage

// DDL and writes use Execute()
Sql.Execute( "CREATE TABLE IF NOT EXISTS players ( steamid INTEGER PRIMARY KEY, name TEXT, kills INTEGER DEFAULT 0 )" );
Sql.Execute( "INSERT OR REPLACE INTO players (steamid, name, kills) VALUES (@id, @name, @kills)", 
    new { id = 76561198012345678, name = "Player", kills = 5 } );

// Reads use Query()
var rows = Sql.Query( "SELECT * FROM players WHERE kills > @min", new { min = 10 } );
foreach ( var row in rows )
    Log.Info( $"{row["name"]} has {row["kills"]} kills" );

// Single value
var count = Sql.QueryValue<int>( "SELECT COUNT(*) FROM players" );
var topPlayer = Sql.QueryValue<string>( "SELECT name FROM players ORDER BY kills DESC LIMIT 1" );

// Explicit database when you need both client and server storage
Sql.Server.Execute( "INSERT INTO server_data ..." );
Sql.Client.Execute( "INSERT INTO client_prefs ..." );

// Transactions - manual
Sql.Begin();
Sql.Execute( "UPDATE players SET kills = kills + 1 WHERE steamid = @id", new { id } );
Sql.Execute( "INSERT INTO kill_log ..." );
Sql.Commit(); // or Sql.Rollback();

// Transactions - automatic (commits on dispose, rolls back on exception)
using ( var tx = Sql.Transaction() )
{
    Sql.Execute( "..." );
    Sql.Execute( "..." );
}

// Fluent query builder for dynamic queries
var query = new SqlBuilder()
    .Select( "steamid", "name", "kills" )
    .From( "players" )
    .Where( "kills > @min", new { min = 0 } )
    .OrderBy( "kills", descending: true )
    .Limit( 10 );

var topPlayers = query.ExecuteQuery( Sql.Server );

// Helpers
if ( !Sql.TableExists( "players" ) ) { ... }
if ( !Sql.IndexExists( "players", "idx_name" ) ) { ... }
var safe = Sql.Escape( "O'Brien" ); // O''Brien
var cols = Sql.GetTableColumns( "players" ); // ["steamid", "name", "kills"]

Error handling

Methods return null (Query), -1 (Execute), or default (QueryValue) on failure. Check Sql.LastError for the error message.

var result = Sql.Query( "SELECT * FROM nonexistent" );
if ( result == null )
    Log.Warning( $"Query failed: {Sql.LastError}" );

Copilot AI review requested due to automatic review settings December 23, 2025 06:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds persistent SQLite database support to the Sandbox engine, allowing games to store data locally using SQL queries. The implementation provides both a static API for convenience and direct database instances for more control, with separate databases for server and client contexts.

  • Introduces SQLite integration via Microsoft.Data.Sqlite package (version 10.0.1)
  • Provides three-tier API: static Sql class for convenience, SqlDatabase for direct connections, and SqlBuilder for fluent query construction
  • Implements transaction support, error handling with LastError property, and helper methods for common operations

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.

Show a summary per file
File Description
engine/Sandbox.Engine/Sandbox.Engine.csproj Adds Microsoft.Data.Sqlite package reference for SQLite functionality
engine/Sandbox.Engine/Utility/Sql/Sql.cs Static API providing convenient access to server/client databases with error handling and helper methods
engine/Sandbox.Engine/Utility/Sql/SqlDatabase.cs Core database wrapper managing connections, transactions, and native library initialization
engine/Sandbox.Engine/Utility/Sql/SqlBuilder.cs Fluent query builder for constructing dynamic SQL queries with parameterization
engine/Sandbox.Test.Unit/Sql/SqlTest.cs Comprehensive unit tests covering database operations, transactions, and the query builder

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@garrynewman garrynewman requested a review from Copilot December 27, 2025 10:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +282 to +320
public async Task<List<Dictionary<string, object>>> QueryAsync( string query, object parameters = null )
{
ThrowIfDisposed();

// Note: We acquire the lock synchronously to avoid deadlocks.
// The actual database operation is async.
lock ( _lock )
{
// Create command inside lock to ensure thread safety
var cmd = CreateCommand( query, parameters );
return QueryAsyncInternal( cmd ).GetAwaiter().GetResult();
}
}

private async Task<List<Dictionary<string, object>>> QueryAsyncInternal( SqliteCommand cmd )
{
using ( cmd )
using ( var reader = await cmd.ExecuteReaderAsync() )
{
var results = new List<Dictionary<string, object>>();

while ( await reader.ReadAsync() )
{
var row = new Dictionary<string, object>( StringComparer.OrdinalIgnoreCase );

for ( int i = 0; i < reader.FieldCount; i++ )
{
var name = reader.GetName( i );
var value = reader.IsDBNull( i ) ? null : reader.GetValue( i );
row[name] = value;
}

results.Add( row );
}

return results;
}
}

Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using GetAwaiter().GetResult() blocks the thread synchronously, defeating the purpose of async code and potentially causing deadlocks. Consider making the entire method chain async or use synchronous operations inside the lock instead.

Suggested change
public async Task<List<Dictionary<string, object>>> QueryAsync( string query, object parameters = null )
{
ThrowIfDisposed();
// Note: We acquire the lock synchronously to avoid deadlocks.
// The actual database operation is async.
lock ( _lock )
{
// Create command inside lock to ensure thread safety
var cmd = CreateCommand( query, parameters );
return QueryAsyncInternal( cmd ).GetAwaiter().GetResult();
}
}
private async Task<List<Dictionary<string, object>>> QueryAsyncInternal( SqliteCommand cmd )
{
using ( cmd )
using ( var reader = await cmd.ExecuteReaderAsync() )
{
var results = new List<Dictionary<string, object>>();
while ( await reader.ReadAsync() )
{
var row = new Dictionary<string, object>( StringComparer.OrdinalIgnoreCase );
for ( int i = 0; i < reader.FieldCount; i++ )
{
var name = reader.GetName( i );
var value = reader.IsDBNull( i ) ? null : reader.GetValue( i );
row[name] = value;
}
results.Add( row );
}
return results;
}
}
public Task<List<Dictionary<string, object>>> QueryAsync( string query, object parameters = null )
{
ThrowIfDisposed();
// Delegate to the synchronous implementation, which handles locking/thread safety.
return Task.FromResult( Query( query, parameters ) );
}

Copilot uses AI. Check for mistakes.
/// </remarks>
public static List<Dictionary<string, object>> GetTableColumns( string tableName )
{
return Query( $"PRAGMA table_info({tableName})" );
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct string interpolation of table name creates SQL injection vulnerability despite the comment warning. Consider validating the table name against a whitelist or using sqlite_master to verify it exists before using it in the PRAGMA statement.

Copilot uses AI. Check for mistakes.
/// <code>
/// if ( !Sql.TableExists( "users" ) )
/// {
/// Sql.Query( "CREATE TABLE users ( id INTEGER PRIMARY KEY, name TEXT )" );
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'Query' to 'Execute' in the documentation example.

Suggested change
/// Sql.Query( "CREATE TABLE users ( id INTEGER PRIMARY KEY, name TEXT )" );
/// Sql.Execute( "CREATE TABLE users ( id INTEGER PRIMARY KEY, name TEXT )" );

Copilot uses AI. Check for mistakes.
/// {
/// for ( int i = 0; i &lt; 1000; i++ )
/// {
/// Sql.Query( "INSERT INTO data (value) VALUES (@v)", new { v = i } );
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'Query' to 'Execute' in the documentation example.

Suggested change
/// Sql.Query( "INSERT INTO data (value) VALUES (@v)", new { v = i } );
/// Sql.Execute( "INSERT INTO data (value) VALUES (@v)", new { v = i } );

Copilot uses AI. Check for mistakes.

if ( providerType == null || rawType == null )
{
throw new InvalidOperationException( "SQLitePCL assemblies not found. Did you check if Microsoft.Data.Sqlite is referenced?" );
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message contains a confusing rhetorical question. Consider rephrasing to a more direct statement like "SQLitePCL assemblies not found. Ensure Microsoft.Data.Sqlite is referenced."

Suggested change
throw new InvalidOperationException( "SQLitePCL assemblies not found. Did you check if Microsoft.Data.Sqlite is referenced?" );
throw new InvalidOperationException( "SQLitePCL assemblies not found. Ensure Microsoft.Data.Sqlite is referenced." );

Copilot uses AI. Check for mistakes.
@garrynewman
Copy link
Member

I struggle with this. I struggle whether it's a good idea or not, whether we should be thinking more advanced. Should we be adding a layer above this, like entity framework, that means that the database can be sqlite, mssql, mysql? Should we be having people write raw sql queries?

@Asphaltian
Copy link
Author

I struggle with this. I struggle whether it's a good idea or not, whether we should be thinking more advanced. Should we be adding a layer above this, like entity framework, that means that the database can be sqlite, mssql, mysql? Should we be having people write raw sql queries?

I wrote this at first because I wanted to enable people to do simple SQL queries like you would in GMod, but an entity framework would be an interesting idea to expand upon. I'm fine with being limited to SQLite though

@jusvit
Copy link
Contributor

jusvit commented Jan 7, 2026

I personally would prefer exposing LiteDB (or a similar API) to be more useful and beginner friendly than direct Sqlite.

e.g.
image

Entity framework would be ideal with the migration support but it's a bunch to abstract and probably shouldn't be abstracted since itself is an abstraction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for Sqlite,

3 participants