Conversation
Moved Unity SDK template files from 'templates/unity/Runtime' and 'templates/unity/Editor' to 'templates/unity/Assets/Runtime' and 'templates/unity/Assets/Editor' for better alignment with Unity project conventions. Updated getFiles() in Unity.php to reflect new paths and added support for copying plugin DLLs and project settings. Improved file upload logic in Client.cs.twig to handle streams and byte arrays more robustly, and removed Unity-specific logging from Exception.cs.twig. Minor fixes in Realtime.cs.twig and Role.cs.twig for namespace and async handling.
Introduces Unity2021 test support by adding a Unity2021Test.php, Unity test source files, and updating the GitHub Actions workflow to include Unity2021 in the test matrix and set the UNITY_LICENSE environment variable. This enables automated testing for the Unity SDK within the CI pipeline.
|
I am also attaching instructions on how to obtain a Unity license painlessly. |
|
The basic functionality tested in the test is guaranteed to work, but I'm not sure about the rest. I will continue to refine it. Also, a question: What to implement next for the client SDK? |
|
The test failed because the secret is not configured in appwrite/sdk-generator. The successful test is here: |
// Cookie support
client.SetCookie() / client.GetCookie()
// Real-time subscriptions
realtime.Subscribe(channels, callback)
// Connection testing
await client.Ping()The successful test is here: |
Refactored AppwriteManager to use a service definitions dictionary for cleaner and more maintainable service initialization. Renamed AcceptAllCertificatesSignedWithASpecificKeyPublicKey to AcceptAllCertificatesHandler for clarity. Enhanced Realtime reconnect logic to properly handle cancellation tokens, ensuring pending retry operations are cancelled and disposed when disconnecting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
templates/unity/Assets/Runtime/Realtime.cs.twig (4)
147-155: WebGL message pump preprocessor guard is inverted.The
#if !UNITY_WEBGL || UNITY_EDITORcondition causesDispatchMessageQueueto run on non-WebGL platforms and in Editor, but NativeWebSocket requires manual dispatching only on WebGL builds. On native platforms, messages are already dispatched on background threads.The guard should be inverted to dispatch on WebGL runtime only:
- #if !UNITY_WEBGL || UNITY_EDITOR - _webSocket?.DispatchMessageQueue(); - #endif + #if UNITY_WEBGL && !UNITY_EDITOR + _webSocket?.DispatchMessageQueue(); + #endif
323-338: Add defensive null check for_client.While
_clientshould be initialized viaInitialize()before any WebSocket operations, a defensive guard would prevent NullReferenceException if the component lifecycle is disrupted.private void SendFallbackAuthentication() { + if (_client == null) return; + var session = _client.Config.GetValueOrDefault("session");
345-363: User callbacks may execute on background thread.On non-WebGL platforms, NativeWebSocket's
OnMessagefires on a background thread. Invoking user callbacks directly can break Unity API calls and cause race conditions.Consider marshaling to the main thread:
private void HandleRealtimeEvent(RealtimeResponseEvent<Dictionary<string, object>> eventData) { try { var subscriptionsCopy = _subscriptions.Values.ToArray(); + // Post to main thread to ensure Unity API safety + UniTask.Post(() => + { foreach (var subscription in subscriptionsCopy) { if (subscription.Channels.Any(subChannel => eventData.Channels.Contains(subChannel))) { subscription.OnMessage?.Invoke(eventData); } } + }); }
540-543: Fire-and-forget in OnDestroy may cause incomplete cleanup.Unity doesn't wait for async operations in
OnDestroy. TheDisconnect().Forget()call may not complete before the object is destroyed, potentially leaving WebSocket connections open.Consider synchronous cleanup:
private void OnDestroy() { - Disconnect().Forget(); + // Perform synchronous cleanup since Unity won't wait for async + _reconnect = false; + StopHeartbeat(); + _cancellationTokenSource?.Cancel(); + _cancellationTokenSource?.Dispose(); + + if (_webSocket != null && _webSocket.State != WebSocketState.Closed) + { + _webSocket.Close(); + } }templates/unity/Assets/Runtime/AppwriteManager.cs.twig (1)
244-259: Fire-and-forget disconnect may leave connections open.The
Disconnect().Forget()call may not complete before the GameObject is destroyed. Given thatShutdownis called fromOnDestroy, async won't help here.If
Realtime.OnDestroyperforms synchronous cleanup (as suggested), this becomes less of a concern. Alternatively, expose a synchronousDisconnectImmediate()method on Realtime:- _realtime.Disconnect().Forget(); + _realtime.DisconnectImmediate(); // Synchronous cleanuptemplates/unity/Assets/Runtime/Core/Client.cs.twig (4)
261-262: Content-Type header lookup is case-sensitive.
headers.TryGetValue("Content-Type", ...)won't match"content-type"keys. While internal headers use lowercase, external callers might pass different casing.- var isMultipart = headers.TryGetValue("Content-Type", out var contentType) && - "multipart/form-data".Equals(contentType, StringComparison.OrdinalIgnoreCase); + var isMultipart = headers.Any(h => + h.Key.Equals("Content-Type", StringComparison.OrdinalIgnoreCase) && + h.Value != null && + h.Value.StartsWith("multipart/form-data", StringComparison.OrdinalIgnoreCase));
461-469: May miss multiple Set-Cookie headers.
GetResponseHeader("Set-Cookie")typically returns only the first header. Consider usingGetResponseHeaders()to capture all cookies:#if !(UNITY_WEBGL && !UNITY_EDITOR) - var setCookieHeader = request.GetResponseHeader("Set-Cookie"); - if (!string.IsNullOrEmpty(setCookieHeader)) - { - var uri = new Uri(request.url); - _cookieContainer.ParseSetCookieHeader(setCookieHeader, uri.Host); - } + var responseHeaders = request.GetResponseHeaders(); + if (responseHeaders != null && responseHeaders.TryGetValue("Set-Cookie", out var setCookieHeader)) + { + var uri = new Uri(request.url); + _cookieContainer.ParseSetCookieHeader(setCookieHeader, uri.Host); + } #endif
658-664: Off-by-one error in chunk size calculation.
ChunkSize - 1causes each chunk to be one byte smaller than intended, potentially causing data corruption over multiple chunks.case "bytes": buffer = ((byte[])input.Data) .Skip((int)offset) - .Take((int)Math.Min(size - offset, ChunkSize - 1)) + .Take((int)Math.Min(size - offset, ChunkSize)) .ToArray(); break;
721-728: Certificate handler name is misleading.
AcceptAllCertificatesHandleraccepts all certificates unconditionally, but the previous class name implied specific key validation. The current name is more accurate. However, this is still security-sensitive and should only be used for development.Consider adding a warning:
// Custom certificate handler for self-signed certificates + // WARNING: This bypasses certificate validation entirely. Use only for development/testing. public class AcceptAllCertificatesHandler : CertificateHandler { protected override bool ValidateCertificate(byte[] certificateData) { - return true; // Accept all certificates + Debug.LogWarning("[Security] Certificate validation bypassed - do not use in production!"); + return true; } }
🧹 Nitpick comments (2)
templates/unity/Assets/Runtime/Realtime.cs.twig (1)
157-182: Subscribe is synchronous but socket creation is fire-and-forget.While the
_creatingSocketflag prevents concurrent socket creation, callers cannot know when the connection is established. Consider returning aUniTask<RealtimeSubscription>or providing anIsConnectingstate so callers can await readiness.If synchronous return is intentional for simplicity, this is acceptable. Otherwise, consider:
public async UniTask<RealtimeSubscription> Subscribe(string[] channels, Action<RealtimeResponseEvent<Dictionary<string, object>>> callback) { // ... setup code ... await CreateSocket(); return subscription; }templates/unity/Assets/Runtime/AppwriteManager.cs.twig (1)
72-78: Consider logging initialization failures from Start.Using
Initialize().Forget()is the correct UniTask pattern, but failures will be silently swallowed. Consider wrapping in an error handler:private void Start() { if (initializeOnStart) { - Initialize().Forget(); + InitializeWithLogging().Forget(); } } + +private async UniTaskVoid InitializeWithLogging() +{ + var success = await Initialize(); + if (!success) + { + Debug.LogError("{{ spec.title | caseUcfirst }}Manager failed to initialize on Start"); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
templates/unity/Assets/Runtime/AppwriteManager.cs.twig(1 hunks)templates/unity/Assets/Runtime/Core/Client.cs.twig(1 hunks)templates/unity/Assets/Runtime/Realtime.cs.twig(1 hunks)
🔇 Additional comments (10)
templates/unity/Assets/Runtime/Realtime.cs.twig (3)
184-208: LGTM!The subscription cleanup logic correctly handles channel reference counting and appropriately closes the connection when no subscriptions remain.
210-258: LGTM!The socket creation logic properly guards against concurrent creation, handles URL changes, and includes appropriate error handling with retry.
423-461: LGTM!The retry logic now properly checks
_reconnectafter the delay and uses cancellation tokens to cancel pending retries when disconnecting, addressing the previous race condition concern.templates/unity/Assets/Runtime/AppwriteManager.cs.twig (3)
57-70: LGTM!The singleton pattern implementation correctly handles duplicate instances with appropriate warnings and cleanup.
80-126: LGTM!The initialization flow has proper validation, error handling, and success indication via return value.
128-156: LGTM!The factory-based service creation pattern avoids the IL2CPP reflection issues previously flagged with
Activator.CreateInstance.templates/unity/Assets/Runtime/Core/Client.cs.twig (4)
36-59: LGTM!The JSON serializer configuration with camelCase naming, null handling, and custom converters follows best practices.
61-87: LGTM!The constructor properly initializes all fields and loads persisted session data with appropriate error handling.
377-432: LGTM!The redirect method correctly disables auto-redirect and returns the Location header for OAuth flow handling.
443-535: LGTM!The
Call<T>method handles both JSON and binary responses appropriately with proper error extraction and optional type conversion.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Unity2021Test.php(1 hunks)tests/languages/unity/Tests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/languages/unity/Tests.cs
🔇 Additional comments (3)
tests/Unity2021Test.php (3)
1-13: LGTM! Configuration is consistent and well-structured.The Unity SDK metadata and language provider configuration are set up correctly.
14-18: Build steps look reasonable.The test asset preparation correctly copies Unity test files into the generated SDK structure.
30-45: LGTM! Comprehensive test coverage.The expected output array appropriately merges all response types from the base test class, ensuring comprehensive validation of Unity SDK functionality including HTTP operations, uploads/downloads, enums, exceptions, realtime, cookies, and helper utilities.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/languages/unity/Tests.cs`:
- Around line 84-116: The tests currently only call
foo.Get/foo.Post/foo.Put/foo.Patch/foo.Delete and bar.Get/... and use
Debug.Log(mock.Result) without verifying outcomes; update each invocation (e.g.,
calls to foo.Get, bar.Post, etc.) to include assertions that the returned Mock
is not null and that mock.Result equals the expected value for that call (use
Assert.IsNotNull(mock) and Assert.AreEqual(expectedValue, mock.Result) or
similar), and add any necessary expected values or helper variables for clarity
so each HTTP method test actually validates correctness.
♻️ Duplicate comments (5)
tests/languages/unity/Tests.cs (1)
127-133: Dispose file streams to avoid handle leaks.The
FileInfo.OpenRead()calls create streams that are never disposed. This was flagged in a previous review.tests/Unity2021Test.php (2)
20-20: Refactor the Unity test command and verify the editor image is still supportedThe inline docker one-liner is hard to maintain and masks license-activation failures. Consider moving this to a script and updating the Unity editor image to a currently supported LTS tag.
What is the current supported Unity LTS version and the recommended unityci/editor Ubuntu tag for headless CI?
24-25: Scope/document UNITY_TEST_MODE exclusionsUsing a global flag to exclude “problematic files” makes test behavior opaque. Prefer a scoped config or explicitly list the excluded files with rationale.
#!/bin/bash # Locate all UNITY_TEST_MODE usage to see what gets excluded and why rg -n "UNITY_TEST_MODE" -C2templates/unity/Assets/Runtime/Realtime.cs.twig (2)
585-588: Avoid async teardown inOnDestroyUnity doesn’t wait for async work in
OnDestroy, which can leave sockets open. Consider synchronous cleanup or move async shutdown toOnDisable/ an explicit teardown method.
147-154: WebGL message pump guard is inverted
DispatchMessageQueueshould run on WebGL builds (not in the Editor). The current guard skips WebGL, so realtime events won’t dispatch there.🐛 Proposed fix
- `#if` !UNITY_WEBGL || UNITY_EDITOR - _webSocket?.DispatchMessageQueue(); - `#endif` + `#if` UNITY_WEBGL && !UNITY_EDITOR + _webSocket?.DispatchMessageQueue(); + `#endif`
🧹 Nitpick comments (5)
tests/languages/unity/Tests.cs (4)
156-174: Error tests should assert expected exception details.The error handling tests catch
AppwriteExceptionbut only log the message. They should assert the expected error code or message to ensure the SDK correctly surfaces error information.💡 Suggested improvement
try { await general.Error400(); + Assert.Fail("Expected AppwriteException was not thrown"); } catch (AppwriteException e) { Debug.Log(e.Message); - Debug.Log(e.Response); + Assert.AreEqual(400, e.Code); + Assert.IsNotNull(e.Response); }
200-206: Cookie roundtrip should assert the retrieved value matches.The cookie tests set and get cookies but don't verify the retrieved cookie matches what was set. This is a critical path for OAuth session persistence mentioned in the PR objectives.
mock = await general.SetCookie(); Assert.AreEqual("success", mock.Result); mock = await general.GetCookie(); Assert.AreEqual("expected_cookie_value", mock.Result);
207-231: Query helper tests should assert generated output format.All query helper calls log output without verifying correctness. Consider adding at least a few representative assertions to catch regressions in query string generation:
Assert.AreEqual("{\"method\":\"equal\",\"attribute\":\"title\",\"values\":[\"Spiderman\",\"Dr. Strange\"]}", Query.Equal("title", new List<string> { "Spiderman", "Dr. Strange" })); Assert.AreEqual("{\"method\":\"limit\",\"values\":[50]}", Query.Limit(50));
24-26: Consider a more descriptive test name or splitting into focused tests.The test method
Test1covers many distinct features (HTTP methods, file uploads, realtime, cookies, query helpers, etc.). Consider:
- Renaming to something descriptive like
TestAppwriteClientIntegration- Splitting into separate test methods per feature area for easier failure diagnosis
This is a minor suggestion since integration tests often bundle operations, but it improves debuggability.
templates/dotnet/Package/Converters/ObjectToInferredTypesConverter.cs.twig (1)
45-51: Preserve numeric precision with a decimal fallback.
Falling back todoublecan lose precision for large integers or decimal values. If precision matters, preferTryGetDecimalbeforeGetDouble.♻️ Suggested tweak
case JsonValueKind.Number: if (element.TryGetInt64(out long l)) { return l; } + if (element.TryGetDecimal(out decimal d)) + { + return d; + } return element.GetDouble();Please verify
JsonElement.TryGetDecimalavailability/behavior in your target Unity/.NET runtime to avoid compatibility issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig`:
- Around line 28-33: The template emits a closing </summary> without ever
outputting an opening <summary>, causing CS1570; update ServiceTemplate.cs.twig
so the entire summary block (the opening <summary>, the description lines that
use method.description | dotnetComment, and the closing </summary>) is emitted
only when method.description is present — i.e., move or add the opening
<summary> inside the {% if method.description %} block and ensure the
corresponding </summary> remains inside that same conditional to keep the XML
well-formed.
- Around line 13-16: The template currently imports System.Web under a
conditional that still targets player platforms; remove System.Web from the
using block in ServiceTemplate.cs.twig and update the code paths that call
HttpUtility.ParseQueryString (notably in WebAuthComponent.cs.twig and
oauth.twig) to use a platform-safe parser: either limit System.Web usage to `#if`
UNITY_EDITOR only, or replace HttpUtility.ParseQueryString calls with a .NET
Standard approach that parses Uri.Query into a NameValueCollection (or a small
custom parser that splits on '?'/'&' and decodes keys/values) so builds on
WebGL/iOS/Android succeed; update references to HttpUtility.ParseQueryString in
the mentioned templates to call the new parser or guard them with UNITY_EDITOR.
♻️ Duplicate comments (1)
templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig (1)
41-75: Return a value fromwebAuthmethods (currentlyUniTaskvoid)
webAuthis emitted asUniTask(void) and the unsupported-platform branch returnsUniTask.CompletedTask, which drops any auth URL/session value. Other SDKs return a string for webAuth. Consider returningUniTask<string>and a typed fallback.✅ Suggested template fix
- public {% if method.type == "webAuth" %}async {% endif ~%} UniTask{% if method.type == "webAuth" %}{% else %}<{{ utils.resultType(spec.title, method) }}>{% endif %} {{ method.name | caseUcfirst }}({{ utils.method_parameters(method.parameters, method.consumes) }}) + public {% if method.type == "webAuth" %}async {% endif ~%} UniTask{% if method.type == "webAuth" %}<string>{% else %}<{{ utils.resultType(spec.title, method) }}>{% endif %} {{ method.name | caseUcfirst }}({{ utils.method_parameters(method.parameters, method.consumes) }}) ... - public UniTask {{ method.name | caseUcfirst }}({{ utils.method_parameters(method.parameters, method.consumes) }}) + public UniTask<string> {{ method.name | caseUcfirst }}({{ utils.method_parameters(method.parameters, method.consumes) }}) { Debug.LogWarning("[{{ spec.title | caseUcfirst }}] OAuth2 authorization is not supported on this platform. Available only in Editor, WebGL, iOS or Android."); - return UniTask.CompletedTask; + return UniTask.FromResult<string>(null); }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig`:
- Around line 41-44: The webAuth methods currently use a void UniTask signature
causing SDK divergence; update the generated method signature logic so when
method.type == "webAuth" the return type is UniTask<string> (use
utils.resultType(spec.title, method) or explicitly string for webAuth) and
ensure the implementation in the generated method (the code that extracts the
auth secret/key from the callback) returns the auth URL/token (the value you
extract) instead of discarding it so callers receive the string result; adjust
any async/await and return paths in the method named via {{ method.name |
caseUcfirst }} accordingly.
In `@templates/unity/Assets/Runtime/Core/WebAuthComponent.cs.twig`:
- Around line 27-28: The debug log prints raw deep link/auth URLs which may
contain sensitive query parameters; create and call a helper like
RedactSensitiveQueryParams(string url) from WebAuthComponent (used where
Debug.Log currently logs the URL and where auth URL is logged at the other
occurrence) to produce a redactedUrl for logging while keeping the original url
passed to OnDeepLink.Invoke. The helper should parse the URI, identify common
sensitive keys (e.g., access_token, id_token, refresh_token, token, code,
secret, api_key, auth) and replace their values with a fixed mask (e.g.,
"[REDACTED]") or remove them, then return the reconstructed URL for Debug.Log;
update both Debug.Log calls to log the redactedUrl instead of the raw url.
♻️ Duplicate comments (3)
templates/unity/Assets/Runtime/Core/WebAuthComponent.cs.twig (2)
30-33: Static event subscription lacks editor cleanup for domain reloadsThe static constructor subscribes
HandleCallbacktoOnDeepLink, but in Editor domain reloads, this can lead to duplicate subscriptions or stale references. Consider adding editor-specific cleanup.♻️ Proposed fix for editor cleanup
static WebAuthComponent() { + OnDeepLink -= HandleCallback; // Prevent duplicate subscriptions on domain reload OnDeepLink += HandleCallback; } + +#if UNITY_EDITOR + [UnityEditor.InitializeOnLoadMethod] + private static void EditorCleanup() + { + UnityEditor.AssemblyReloadEvents.beforeAssemblyReload += () => + { + OnDeepLink -= HandleCallback; + }; + } +#endif
61-68: Consider adding authentication timeout to prevent indefinite hangsIf the user abandons the OAuth flow (closes browser, loses network), the
await tcs.Taskwill hang indefinitely. A timeout would allow the application to recover gracefully.♻️ Proposed timeout implementation
+ using var timeoutCts = new System.Threading.CancellationTokenSource(); + timeoutCts.CancelAfter(TimeSpan.FromMinutes(5)); + + timeoutCts.Token.Register(() => + { + if (PendingAuth.TryRemove(callbackScheme, out var timedOutTcs)) + { + timedOutTcs.TrySetException(new TimeoutException("OAuth authentication timed out after 5 minutes.")); + } + }); + try { return await tcs.Task; }templates/unity/base/requests/oauth.twig (1)
20-22: Validate callbackDomain to prevent cookie scope hijackingThe
callbackDomainis taken directly from the callback query parameter without validation. A malicious OAuth callback could specify an arbitrary domain, potentially affecting cookie scope in unintended ways.Restrict accepted domains to the endpoint host or its subdomains:
🔒 Proposed fix for domain validation
// Use domain from callback if available, otherwise fallback to endpoint host - var domain = !string.IsNullOrEmpty(callbackDomain) ? callbackDomain : new Uri(_client.Endpoint).Host; + var endpointHost = new Uri(_client.Endpoint).Host; + var domain = endpointHost; + if (!string.IsNullOrEmpty(callbackDomain)) + { + var normalizedCallback = callbackDomain.TrimStart('.'); + if (string.Equals(normalizedCallback, endpointHost, StringComparison.OrdinalIgnoreCase) || + normalizedCallback.EndsWith("." + endpointHost, StringComparison.OrdinalIgnoreCase)) + { + domain = normalizedCallback; + } + } var parsedDomain = domain.StartsWith(".") ? domain.Substring(1) : domain;
🧹 Nitpick comments (3)
templates/unity/base/requests/oauth.twig (1)
27-31: Skip cookie persistence in Editor since it's documented as non-functionalThe warning states cookies cannot be obtained in Editor, yet
ParseSetCookieHeaderis still called. This is misleading and may cause confusion during development.♻️ Proposed refactor to gate cookie write
- _client.CookieContainer.ParseSetCookieHeader(setCookieHeader, parsedDomain); - -#if UNITY_EDITOR +#if UNITY_EDITOR Debug.LogWarning("[{{ spec.title | caseUcfirst }}] OAuth authorization in Editor: you can open and authorize, but cookies cannot be obtained. The session will not be set."); +#else + _client.CookieContainer.ParseSetCookieHeader(setCookieHeader, parsedDomain); `#endif`templates/dotnet/Package/Extensions/Extensions.cs.twig (2)
16-24: Guard against unexpectedstring/JsonElementinputs inToEnumerable.
stringimplementsIEnumerable, so this will currently yield characters, which is usually surprising for list semantics. Also, anyJsonElementarrays will now throw. Please verify call sites and consider explicit handling.♻️ Suggested adjustment
public static IEnumerable<object> ToEnumerable(this object value) { return value switch { + string s => new object[] { s }, object[] array => array, IEnumerable<object> enumerable => enumerable, IEnumerable nonGeneric => nonGeneric.Cast<object>(), _ => throw new InvalidCastException($"Cannot convert {value?.GetType().Name ?? "null"} to IEnumerable<object>") }; }
52-69: Use URL-decoding that handles+and skip empty pairs.If the input is form-urlencoded (common for OAuth/query strings),
+should decode to space. Also, removing empty entries avoids inserting empty keys for trailing&.♻️ Suggested adjustment
- if (string.IsNullOrEmpty(query)) return dict; - if (query.StartsWith("?")) query = query.Substring(1); - var pairs = query.Split('&'); + if (string.IsNullOrEmpty(query)) return dict; + if (query.StartsWith("?")) query = query.Substring(1); + var pairs = query.Split('&', StringSplitOptions.RemoveEmptyEntries); foreach (var pair in pairs) { var kv = pair.Split(new char[] { '=' }, 2); if (kv.Length >= 1) { - var key = Uri.UnescapeDataString(kv[0]); - var value = kv.Length > 1 ? Uri.UnescapeDataString(kv[1]) : ""; + var key = WebUtility.UrlDecode(kv[0]); + var value = kv.Length > 1 ? WebUtility.UrlDecode(kv[1]) : ""; dict[key] = value; } }If you take this approach, add:
using System.Net;
| {% if method.type == "webAuth" %} | ||
| #if UNITY_EDITOR || UNITY_IOS || UNITY_ANDROID || UNITY_WEBGL | ||
| {% endif %} | ||
| public {% if method.type == "webAuth" %}async {% endif ~%} UniTask{% if method.type == "webAuth" %}{% else %}<{{ utils.resultType(spec.title, method) }}>{% endif %} {{ method.name | caseUcfirst }}({{ utils.method_parameters(method.parameters, method.consumes) }}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check webAuth handling in DotNet template
echo "=== DotNet webAuth handling ==="
find templates/dotnet -name "*.twig" -o -name "*.cs" | head -5
rg -B 2 -A 5 'webAuth' templates/dotnet --type twig 2>/dev/null || echo "Searching DotNet templates for webAuth..."Repository: appwrite/sdk-generator
Length of output: 4095
🏁 Script executed:
#!/bin/bash
# Check all SDK templates for webAuth return types
echo "=== Search webAuth across SDK templates ==="
find templates -maxdepth 2 -type d | head -10
rg -l 'webAuth' templates --type twig | head -10Repository: appwrite/sdk-generator
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Check the spec.json for webAuth method definitions
echo "=== WebAuth in spec.json ==="
if [ -f "tests/resources/spec.json" ]; then
rg -B 3 -A 10 '"type":\s*"webAuth"' tests/resources/spec.json | head -50
elif [ -f "spec.json" ]; then
rg -B 3 -A 10 '"type":\s*"webAuth"' spec.json | head -50
else
find . -name "spec.json" -type f 2>/dev/null | head -3
fiRepository: appwrite/sdk-generator
Length of output: 563
🏁 Script executed:
#!/bin/bash
# Check Dart and Python webAuth return types
echo "=== Dart webAuth handling ==="
rg -B 2 -A 5 'webAuth' templates/dart --type twig 2>/dev/null | head -50
echo -e "\n=== Python webAuth handling ==="
rg -B 2 -A 5 'webAuth' templates/python --type twig 2>/dev/null | head -50
echo -e "\n=== Web (TypeScript) webAuth handling ==="
rg -B 2 -A 5 'webAuth' templates/web/src/services --type twig 2>/dev/null | head -50Repository: appwrite/sdk-generator
Length of output: 7246
🏁 Script executed:
#!/bin/bash
# Check the actual oauth.twig implementations
echo "=== DotNet oauth.twig ==="
find templates/dotnet -name "oauth.twig" -exec cat {} \;
echo -e "\n=== Unity oauth.twig ==="
find templates/unity -name "oauth.twig" -exec cat {} \;Repository: appwrite/sdk-generator
Length of output: 2280
🏁 Script executed:
#!/bin/bash
# Verify the exact line 44 signature in Unity ServiceTemplate.cs.twig
echo "=== Unity ServiceTemplate.cs.twig around line 44 ==="
sed -n '40,50p' templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twigRepository: appwrite/sdk-generator
Length of output: 731
WebAuth methods return void (UniTask) instead of UniTask<string>, inconsistent with other SDKs
DotNet returns Task<String>, Dart returns Future<String?>, Python returns str, and Web returns the auth URL as a string. Unity's webAuth methods emit UniTask (void return) instead, creating API divergence across SDKs.
The Unity oauth.twig already extracts authentication data (secret/key) from the callback but discards it. Update the signature to UniTask<string> and return the authentication URL or token to align with other SDK implementations.
🤖 Prompt for AI Agents
In `@templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig` around
lines 41 - 44, The webAuth methods currently use a void UniTask signature
causing SDK divergence; update the generated method signature logic so when
method.type == "webAuth" the return type is UniTask<string> (use
utils.resultType(spec.title, method) or explicitly string for webAuth) and
ensure the implementation in the generated method (the code that extracts the
auth secret/key from the callback) returns the auth URL/token (the value you
extract) instead of discarding it so callers receive the string result; adjust
any async/await and return paths in the method named via {{ method.name |
caseUcfirst }} accordingly.
There was a problem hiding this comment.
What should be returned? The OAuth2 process is configured automatically, it catches deep links itself, and then saves the result in cookies. In WebGL, it is not possible to use links for success errors, since this application is located at a single address.
There was a problem hiding this comment.
@Fellmonkey, thank you for the clarification on Unity's OAuth2 architecture. I understand the design now:
- Deep link handling is automatic via
WebAuthComponent - Session credentials are extracted from the callback and stored in cookies transparently
- The client is already configured with the session after the method completes
- WebGL's single-address constraint prevents traditional redirect-based flows
This makes sense for Unity's async/await model where the caller simply awaits completion and the session is automatically available in the Client instance afterward.
Question: Are there any developer scenarios where returning the session data (e.g., the secret/key or a success indicator) would be useful? For example:
- Inspecting which account was authenticated without an additional API call
- Logging/debugging the auth flow
- Cross-SDK consistency for developers familiar with other Appwrite SDKs
If the void return is intentional for Unity's "automatic session management" philosophy and there's no practical use case for a return value, I'm happy to close this comment. The exception-based error handling already covers failure cases, so the current design seems reasonable for Unity's architecture.
Let me know your thoughts!
There was a problem hiding this comment.
close this and remember it for the next commits
There was a problem hiding this comment.
@Fellmonkey, understood! I've recorded this for future reference. The Unity SDK's webAuth methods intentionally return UniTask (void) rather than a string/token because:
- Deep link handling and session management are automatic via
WebAuthComponent - Session credentials are transparently extracted and stored in cookies
- The
Clientinstance is already configured with the session after the method completes - This design accommodates WebGL's single-address constraint
This architectural difference from other SDKs is by design and won't be flagged in future reviews. Closing this thread.
✏️ Learnings added
Learnt from: Fellmonkey
Repo: appwrite/sdk-generator PR: 1105
File: templates/unity/Assets/Runtime/Core/Services/ServiceTemplate.cs.twig:41-44
Timestamp: 2026-01-19T19:12:41.055Z
Learning: Unity SDK webAuth methods intentionally return UniTask (void) instead of UniTask<string> or string like other SDKs (DotNet, Dart, Python, Web) because Unity implements automatic session management via WebAuthComponent with transparent deep link handling and cookie storage. The Client instance is already configured with the session after the method completes, and this design accommodates WebGL's single-address constraint.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Introduce centralized property conversion and mapping helpers for the .NET SDK generator and update model template to use them. Changes: - Add Twig filters: propertyAssignment and toMapValue. - Make getPropertyType optionally return non-fully-qualified names. - Replace inline parsing logic with new methods: getPropertyName, getResolvedPropertyName, getPropertyAssignment, convertValue, getToMapExpression. These handle enums, sub-schemas, arrays, primitive conversions and null-safety in a unified way. - Simplify getFunctions() implementation to delegate to the new helpers. - Update templates/dotnet/Package/Models/Model.cs.twig to use the new filters (propertyAssignment, toMapValue) and clean up From/ToMap generation. Reason: centralizes and standardizes property (de)serialization logic, improves null handling, supports property overrides, and simplifies the model template.
Add query support and improve realtime subscription handling; refactor Channel API to use nullable IDs and introduce IChannelValue. - Realtime: support per-subscription queries, normalize channel/query inputs, track subscription slot ↔ id mappings, route messages by subscription id when present, and include queries in websocket URL. Handle pending socket rebuilds and populate subscription maps on connect. - Channel: change Normalize to accept null, use null as default for ID parameters instead of "*", add IChannelValue interface and make IActionable inherit it, remove old Function.Execution pattern and expose Function/Execution as channel values. Add Account/Documents/Rows/Files/Executions/Teams/Memberships helpers. - Tests: add helper WaitForRealtimeMessage and update realtime and Channel-related test calls to match the new APIs. These changes enable more flexible realtime queries and simplify channel construction semantics.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/SDK/Language/DotNet.php`:
- Around line 528-530: The toMapValue TwigFilter callback is missing its closing
brace and comma, causing a PHP parse error where the next new
TwigFilter('enumExample'...) starts inside the unclosed closure; locate the new
TwigFilter('toMapValue', function (array $property, string $definitionName) {
... return $this->getToMapExpression($property, $definitionName); }) entry in
DotNet.php (uses getToMapExpression and TwigFilter) and close the anonymous
function and array element by adding the missing "})," (i.e., close the function
body with } and end the TwigFilter call with ,) so the following new
TwigFilter('enumExample'...) is a separate array item.
In `@templates/unity/Assets/Runtime/Channel.cs.twig`:
- Around line 88-96: ChannelBuilder.Next currently always appends both the
segment and ChannelHelper.Normalize(id), which turns a null id into "*" and
causes extra wildcard segments; change Next (in ChannelBuilder.Next) to only
append the id segment when id is non-null/non-empty (i.e., call
ChannelHelper.Normalize(id) and add it to newSegments only if id is provided) so
document()/row()/file() produce the same path format as other SDKs (omit the id
segment when id is null). Ensure the method still returns a new
ChannelBuilder(newSegments).
In `@tests/languages/unity/Tests.cs`:
- Around line 199-207: The current check using WaitForRealtimeMessage stores the
result in realtimeFailureResponse and then logs "Realtime failed!" in both
branches; update the branches so the timeout case (realtimeFailureResponse ==
"No realtime message received within timeout") logs a pass message (e.g.,
"Realtime passed!") and the non-timeout case logs the failure; modify the
Debug.Log calls around WaitForRealtimeMessage / realtimeFailureResponse /
Query.Equal so the if branch reports success and the else branch reports
failure.
🧹 Nitpick comments (3)
templates/dotnet/Package/Extensions/Extensions.cs.twig (1)
52-69:FromQueryString: empty pairs from consecutive&delimiters will insert an empty-string key.
"a&&b".Split('&')yields["a", "", "b"]. The empty element passes thekv.Length >= 1guard (sinceSplitalways returns ≥ 1 element) and insertsdict[""] = "". If the callers can receive malformed query strings, consider skipping empty/whitespace pairs:Suggested guard
foreach (var pair in pairs) { + if (string.IsNullOrEmpty(pair)) continue; var kv = pair.Split(new char[] { '=' }, 2);tests/languages/unity/Tests.cs (2)
63-79: RepeatedSetProjecttoggling is confusing and fragile.The client project is switched between
"123456"and"console"multiple times (lines 55, 63, 74, 79). Since all service instances share the sameClientreference, any reordering or insertion of tests could silently run against the wrong project. Consider either documenting the reason for each switch inline or using separate client instances for different project contexts.
71-71: Unexplained 5-second hard delay.
await Task.Delay(5000)with no comment explaining what it's waiting for. If it's for the realtime WebSocket to connect, document it; if it's a workaround, add a TODO.
4 participants
What does this PR do?
This PR adds comprehensive Unity SDK support to the SDK generator by:
Unity Language Implementation: Introduces a new
Unitylanguage class (src/SDK/Language/Unity.php) that extends the baseLanguageclass, providing Unity-specific type mappings, keywords, and code generation logic for C# in Unity environment.Unity Template System: Adds a complete set of Unity-specific templates under
templates/unity/Assets/including:Automated Testing Integration: Introduces Unity2021 test support with:
Unity2021Test.phptest class that integrates with the existing test frameworkTests.cs,Tests.asmdef) for comprehensive SDK testingunityci/editor:ubuntu-2021.3.45f1-base-3.1.0CI/CD Integration: Updates the GitHub Actions workflow to:
UNITY_LICENSEenvironment variable for Unity Editor automationUnity Project Structure: Implements proper Unity package structure with:
Assets/Runtime/andAssets/Editor/Test Plan
The testing strategy includes:
Unit Testing: The
Unity2021Test.phpruns comprehensive tests covering:Docker-based CI Testing: Tests run in a controlled Unity environment using:
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores