-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add SessionFs sqlite support for runtime sqlite routing #1299
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.
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
Changes from all commits
1243200
9746832
375b38a
5489ea9
fe58b83
340fea9
aece0f0
76a1885
992d863
7bd8d0a
5b7d12f
ee5d33a
9c6d4c0
6169962
8723765
132d70b
48a810d
b37d69e
24027e8
05ce059
90c53d0
80b4291
5387c54
220d29d
8d34029
b5030f4
d853a4d
aa324a6
ec88807
b41e979
1ae46d6
2950a1a
0a5f123
6ab9e8e
a222c29
5533304
d5a3b91
aaec17b
eaf0994
e2efe84
424f184
39ba0fd
01eb131
209e35f
fe4bbd3
b9f2cf4
63eb8d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,60 @@ | |
|
|
||
| namespace GitHub.Copilot.SDK; | ||
|
|
||
| /// <summary> | ||
| /// Result of a SQLite query execution via <see cref="ISessionFsSqliteProvider"/>. | ||
| /// Same shape as <see cref="SessionFsSqliteQueryResult"/> but without the <c>Error</c> field, | ||
| /// since providers signal errors by throwing. | ||
| /// </summary> | ||
| public class SessionFsSqliteResult | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-SDK naming inconsistency: All other SDK implementations name the provider-facing query result type
The .NET type here is named // Suggested rename:
public class SessionFsSqliteQueryResult { ... } |
||
| { | ||
| /// <summary>Column names from the result set.</summary> | ||
| public IList<string> Columns { get; set; } = []; | ||
|
|
||
| /// <summary>For SELECT: rows as column-keyed dictionaries. For others: empty.</summary> | ||
| public IList<IDictionary<string, object>> Rows { get; set; } = []; | ||
|
|
||
| /// <summary>Number of rows affected (for INSERT/UPDATE/DELETE).</summary> | ||
| public long RowsAffected { get; set; } | ||
|
|
||
| /// <summary>Last inserted row ID (for INSERT).</summary> | ||
| public long? LastInsertRowid { get; set; } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cross-SDK type inconsistency:
Using Suggestion: Use |
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Optional interface for <see cref="SessionFsProvider"/> subclasses that support | ||
| /// per-session SQLite databases. Implement this interface on your provider to enable | ||
| /// the runtime's SQL tool to route queries through your SessionFs implementation. | ||
| /// </summary> | ||
| public interface ISessionFsSqliteProvider | ||
| { | ||
| /// <summary> | ||
| /// Executes a SQLite query against the per-session database. | ||
| /// </summary> | ||
| /// <param name="queryType">How to execute: <c>"exec"</c> for DDL/multi-statement, <c>"query"</c> for SELECT, <c>"run"</c> for INSERT/UPDATE/DELETE.</param> | ||
| /// <param name="query">SQL query to execute.</param> | ||
| /// <param name="bindParams">Optional named bind parameters.</param> | ||
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| /// <returns>The query result, or <c>null</c> for exec-type queries.</returns> | ||
| Task<SessionFsSqliteResult?> QueryAsync( | ||
| SessionFsSqliteQueryType queryType, | ||
| string query, | ||
| IDictionary<string, object>? bindParams, | ||
| CancellationToken cancellationToken); | ||
|
|
||
| /// <summary> | ||
| /// Checks whether the per-session SQLite database already exists, without creating it. | ||
| /// </summary> | ||
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| Task<bool> ExistsAsync(CancellationToken cancellationToken); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Base class for session filesystem providers. Subclasses override the | ||
| /// virtual methods and use normal C# patterns (return values, throw exceptions). | ||
| /// The base class catches exceptions and converts them to <see cref="SessionFsError"/> | ||
| /// results expected by the runtime. | ||
| /// To add SQLite support, also implement <see cref="ISessionFsSqliteProvider"/>. | ||
| /// </summary> | ||
| public abstract class SessionFsProvider : ISessionFsHandler | ||
| { | ||
|
|
@@ -75,24 +124,6 @@ public abstract class SessionFsProvider : ISessionFsHandler | |
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| protected abstract Task RenameAsync(string src, string dest, CancellationToken cancellationToken); | ||
|
|
||
| /// <summary>Executes a SQLite query against the per-session database.</summary> | ||
| /// <param name="sessionId">Target session identifier.</param> | ||
| /// <param name="query">SQL query to execute.</param> | ||
| /// <param name="queryType">How to execute the query.</param> | ||
| /// <param name="parameters">Optional named bind parameters.</param> | ||
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| protected abstract Task<SessionFsSqliteQueryResult> SqliteQueryAsync( | ||
| string sessionId, | ||
| string query, | ||
| SessionFsSqliteQueryType queryType, | ||
| IDictionary<string, object>? parameters, | ||
| CancellationToken cancellationToken); | ||
|
|
||
| /// <summary>Checks whether the per-session SQLite database already exists.</summary> | ||
| /// <param name="sessionId">Target session identifier.</param> | ||
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| protected abstract Task<bool> SqliteExistsAsync(string sessionId, CancellationToken cancellationToken); | ||
|
|
||
| // ---- ISessionFsHandler implementation (private, handles error mapping) ---- | ||
|
|
||
| async Task<SessionFsReadFileResult> ISessionFsHandler.ReadFileAsync(SessionFsReadFileRequest request, CancellationToken cancellationToken) | ||
|
|
@@ -246,42 +277,50 @@ async Task<SessionFsReaddirWithTypesResult> ISessionFsHandler.ReaddirWithTypesAs | |
|
|
||
| async Task<SessionFsSqliteQueryResult> ISessionFsHandler.SqliteQueryAsync(SessionFsSqliteQueryRequest request, CancellationToken cancellationToken) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(request); | ||
| if (this is not ISessionFsSqliteProvider sqliteProvider) | ||
| { | ||
| return new SessionFsSqliteQueryResult | ||
| { | ||
| Error = new SessionFsError { Code = SessionFsErrorCode.UNKNOWN, Message = "SQLite is not supported by this provider." }, | ||
| }; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| return await SqliteQueryAsync(request.SessionId, request.Query, request.QueryType, request.Params, cancellationToken).ConfigureAwait(false); | ||
| var result = await sqliteProvider.QueryAsync(request.QueryType, request.Query, request.Params, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| return new SessionFsSqliteQueryResult | ||
| { | ||
| Rows = result?.Rows ?? [], | ||
| Columns = result?.Columns ?? [], | ||
| RowsAffected = result?.RowsAffected ?? 0, | ||
| LastInsertRowid = result?.LastInsertRowid, | ||
| }; | ||
| } | ||
| catch (Exception ex) when (!IsCriticalException(ex)) | ||
| catch (Exception ex) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behavioral inconsistency with Node.js and Python for SQL execution errors (same issue as
If the intent is for SQL errors to propagate (preserving original error detail in the JSON-RPC response), this Note: the previous implementation had |
||
| { | ||
| return new SessionFsSqliteQueryResult { Error = ToSessionFsError(ex) }; | ||
| } | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||
| } | ||
|
|
||
| async Task<SessionFsSqliteExistsResult> ISessionFsHandler.SqliteExistsAsync(SessionFsSqliteExistsRequest request, CancellationToken cancellationToken) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(request); | ||
| if (this is not ISessionFsSqliteProvider sqliteProvider) | ||
| { | ||
| return new SessionFsSqliteExistsResult { Exists = false }; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| var exists = await SqliteExistsAsync(request.SessionId, cancellationToken).ConfigureAwait(false); | ||
| var exists = await sqliteProvider.ExistsAsync(cancellationToken).ConfigureAwait(false); | ||
| return new SessionFsSqliteExistsResult { Exists = exists }; | ||
| } | ||
| catch (Exception ex) when (!IsCriticalException(ex)) | ||
| catch | ||
| { | ||
| return new SessionFsSqliteExistsResult { Exists = false }; | ||
| } | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||
| } | ||
|
|
||
| private static bool IsCriticalException(Exception ex) => | ||
| ex is OperationCanceledException | ||
| or OutOfMemoryException | ||
| or StackOverflowException | ||
| or AccessViolationException | ||
| or AppDomainUnloadedException | ||
| or BadImageFormatException | ||
| or CannotUnloadAppDomainException | ||
| or InvalidProgramException; | ||
|
|
||
| private static SessionFsError ToSessionFsError(Exception ex) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-SDK naming inconsistency: This class is named
SessionFsSqliteResult, but all other SDKs name the equivalent typeSessionFsSqliteQueryResult:SessionFsSqliteQueryResult(go/session_fs_provider.go)SessionFsSqliteQueryResult(nodejs/src/sessionFsProvider.ts)SessionFsSqliteQueryResult(python/copilot/session_fs_provider.py)SessionFsSqliteQueryResult(rust/src/session_fs.rs)SessionFsSqliteResult← missing "Query"Suggestion: Rename to
SessionFsSqliteQueryResultto align with the other SDKs. This is a public API type and the name divergence could confuse developers working across languages.