Fix memories endpoint 400 error due to missing timezone in datetime#663
Fix memories endpoint 400 error due to missing timezone in datetime#663bbalz wants to merge 1 commit into
Conversation
The NSwag-generated SearchMemoriesAsync serializes DateTimeOffset using the "s" (sortable) format which omits the timezone offset. Immich v3 requires full ISO 8601 datetime strings with timezone information, causing 400 "Invalid input: expected ISO 8601 datetime string" errors. Add SearchMemoriesWithProperDateAsync to the partial ImmichApi class that manually constructs the request URL with the datetime formatted as "yyyy-MM-ddTHH:mm:sszzz" (e.g. 2026-07-03T07:47:17+00:00). Fixes immichFrame#659
📝 WalkthroughWalkthroughThis PR adds a new ChangesMemories search date fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryAssetsPool
participant ImmichApi
participant ImmichServer
MemoryAssetsPool->>ImmichApi: SearchMemoriesWithProperDateAsync(searchDate, cancellationToken)
ImmichApi->>ImmichApi: format searchDate as ISO 8601 with timezone offset
ImmichApi->>ImmichServer: GET memories/?for=...
ImmichServer-->>ImmichApi: 200 OK with MemoryResponseDto list
ImmichApi-->>MemoryAssetsPool: ICollection<MemoryResponseDto>
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ImmichFrame.Core/Api/ImmichApi.cs (1)
44-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo direct test coverage for the URL/date-format this method produces.
The existing tests (
MemoryAssetsPoolTests) only mockSearchMemoriesWithProperDateAsync; none exercise the real implementation to verify thefor=query value is well-formed ISO 8601 with offset. Since malformed formatting is exactly what caused the original 400 error, consider adding a test (e.g. via a fakeHttpMessageHandler) that asserts the captured request URI'sforparameter matches the expected pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ImmichFrame.Core/Api/ImmichApi.cs` around lines 44 - 123, The new SearchMemoriesWithProperDateAsync implementation in ImmichApi is not covered by a test that verifies its actual request URI formatting. Add a test that exercises the real method (for example with a fake HttpMessageHandler or equivalent) and assert the captured request’s for query parameter is a full ISO 8601 datetime including the timezone offset. Use the SearchMemoriesWithProperDateAsync method name and the request-building logic in ImmichApi to locate the code to cover.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImmichFrame.Core/Api/ImmichApi.cs`:
- Around line 100-123: The HttpResponseMessage in the memories fetch path is
only disposed in the non-200 branch, so the successful path in the API method
leaks response resources. Update the response handling in ImmichApi’s memories
request flow so the response is disposed on both success and error, preferably
by wrapping the SendAsync result in a using/try-finally and keeping the
deserialization return path intact.
- Line 66: The `searchDate` query formatting in `ImmichApi` is culture-sensitive
because the `ToString("yyyy-MM-ddTHH:mm:sszzz")` call uses the current culture’s
time separator. Update the `searchDate` formatting in the same area where
`urlBuilder.Append` is used to pass `CultureInfo.InvariantCulture`, matching the
existing invariant handling for `size`, so the emitted timestamp stays valid ISO
8601 across locales.
---
Nitpick comments:
In `@ImmichFrame.Core/Api/ImmichApi.cs`:
- Around line 44-123: The new SearchMemoriesWithProperDateAsync implementation
in ImmichApi is not covered by a test that verifies its actual request URI
formatting. Add a test that exercises the real method (for example with a fake
HttpMessageHandler or equivalent) and assert the captured request’s for query
parameter is a full ISO 8601 datetime including the timezone offset. Use the
SearchMemoriesWithProperDateAsync method name and the request-building logic in
ImmichApi to locate the code to cover.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b525ca4-0f43-4324-978b-9241bc786e96
📒 Files selected for processing (3)
ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.csImmichFrame.Core/Api/ImmichApi.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs
|
|
||
| // Format with full ISO 8601 including timezone offset | ||
| urlBuilder.Append("for="); | ||
| urlBuilder.Append(Uri.EscapeDataString(searchDate.ToString("yyyy-MM-ddTHH:mm:sszzz"))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant code region.
file="ImmichFrame.Core/Api/ImmichApi.cs"
nl -ba "$file" | sed -n '55,100p'
# Check whether the repository already contains the expected invariant-culture pattern nearby.
rg -n "InvariantCulture|TimeSeparator|yyyy-MM-ddTHH:mm:sszzz|searchDate" "$file"Repository: immichFrame/ImmichFrame
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ImmichFrame.Core/Api/ImmichApi.cs"
# Show the relevant slice with portable tools.
sed -n '55,100p' "$file"
printf '\n---\n'
# Search for related formatting usage in the same file.
grep -nE 'InvariantCulture|TimeSeparator|yyyy-MM-ddTHH:mm:sszzz|searchDate|size' "$file" || trueRepository: immichFrame/ImmichFrame
Length of output: 2606
🌐 Web query:
.NET custom date and time format string colon time separator CultureInfo.CurrentCulture TimeSeparator literal colon yyyy-MM-ddTHH:mm:sszzz
💡 Result:
In.NET custom date and time format strings, the colon (:) character acts as a special format specifier that represents the culture-sensitive time separator [1][2][3]. When a format string contains a ":",.NET replaces it with the value of the TimeSeparator property defined in the current culture's DateTimeFormatInfo (e.g., CultureInfo.CurrentCulture.DateTimeFormat.TimeSeparator) [2][3][4]. If you wish to include a literal colon character in your output string—regardless of the culture settings—you must escape it using literal string delimiters, typically single quotes (') [1][5][6]. For example, comparing the two approaches: 1. Using the culture-sensitive separator: If you use the format string "yyyy-MM-ddTHH:mm:ss", the colon between hours, minutes, and seconds will be replaced by the current culture's TimeSeparator [2][3]. If the culture uses a different separator (such as a dot in some locales), the output will reflect that change [2][5]. 2. Using a literal colon: If you use the format string "yyyy-MM-ddTHH':'mm':'ss", the single quotes tell the.NET formatter to treat the colon as a literal character [5][6][7]. This ensures the output always contains a colon, even if the current culture's defined TimeSeparator is something else [6][7]. Regarding your specific example string "yyyy-MM-ddTHH:mm:sszzz": - The colons used here act as placeholders for the culture's time separator [2][3]. - To force literal colons in this pattern, you would use "yyyy-MM-ddTHH':'mm':'sszzz" [5][6]. It is generally recommended to use the ":" placeholder when you want your application to respect the user's regional formatting preferences [2][8]. Conversely, if you are formatting data for a machine-readable format like ISO 8601, you should use literal delimiters (e.g., 'T', ':') to ensure consistency across all environments [9][6].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/standard/base-types/custom-date-and-time-format-strings
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.globalization.datetimeformatinfo.timeseparator?view=net-9.0
- 3: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-globalization-datetimeformatinfo
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.globalization.datetimeformatinfo.timeseparator?view=net-10.0
- 5: https://github.com/dotnet/docs/blob/main/docs/standard/base-types/custom-date-and-time-format-strings.md
- 6: https://stackoverflow.com/questions/58487644/what-does-a-single-quote-inside-a-c-date-time-format-mean
- 7: https://stackoverflow.com/questions/58487644/what-does-a-single-quote-inside-a-c-sharp-date-time-format-mean
- 8: Changing DateTimeFormatInfo.TimeSeparator on some cultures doesn't work because DateTimeFormatInfo's time patterns doesn't use ':' dotnet/runtime#77037
- 9: https://learn.microsoft.com/en-us/dotnet/standard/base-types/standard-date-and-time-format-strings
Use invariant culture for searchDate formatting
searchDate.ToString("yyyy-MM-ddTHH:mm:sszzz") is culture-sensitive here: the : tokens are treated as the current culture’s time separator, so locales like it-IT can emit 2026-07-03T14.30.00+02:00 instead of valid ISO 8601. Use CultureInfo.InvariantCulture here, matching the size formatting below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Api/ImmichApi.cs` at line 66, The `searchDate` query
formatting in `ImmichApi` is culture-sensitive because the
`ToString("yyyy-MM-ddTHH:mm:sszzz")` call uses the current culture’s time
separator. Update the `searchDate` formatting in the same area where
`urlBuilder.Append` is used to pass `CultureInfo.InvariantCulture`, matching the
existing invariant handling for `size`, so the emitted timestamp stays valid ISO
8601 across locales.
| using var request = new HttpRequestMessage(HttpMethod.Get, urlBuilder.ToString()); | ||
| request.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json")); | ||
|
|
||
| var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| var headers = new Dictionary<string, IEnumerable<string>>(); | ||
| foreach (var item in response.Headers) | ||
| headers[item.Key] = item.Value; | ||
| if (response.Content?.Headers != null) | ||
| foreach (var item in response.Content.Headers) | ||
| headers[item.Key] = item.Value; | ||
|
|
||
| var status = (int)response.StatusCode; | ||
| if (status == 200) | ||
| { | ||
| var json = response.Content == null ? string.Empty : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | ||
| var result = Newtonsoft.Json.JsonConvert.DeserializeObject<ICollection<MemoryResponseDto>>(json, JsonSerializerSettings); | ||
| return result ?? new List<MemoryResponseDto>(); | ||
| } | ||
|
|
||
| var error = response.Content == null ? null : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | ||
| response.Dispose(); | ||
| throw new ApiException($"Unexpected status code ({status}).", status, error, headers, null); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
response is never disposed on the success path.
response.Dispose() is only called in the error branch (Line 121). On the status == 200 path (Lines 113-118), the HttpResponseMessage (and its Content) is never disposed, leaking the underlying resources on every successful memories fetch — and this runs on a recurring polling path (MemoryAssetsPool.LoadAssets).
🐛 Proposed fix
- using var request = new HttpRequestMessage(HttpMethod.Get, urlBuilder.ToString());
+ using var request = new HttpRequestMessage(HttpMethod.Get, urlBuilder.ToString());
request.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json"));
- var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
+ using var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
var headers = new Dictionary<string, IEnumerable<string>>();
foreach (var item in response.Headers)
headers[item.Key] = item.Value;
if (response.Content?.Headers != null)
foreach (var item in response.Content.Headers)
headers[item.Key] = item.Value;
var status = (int)response.StatusCode;
if (status == 200)
{
var json = response.Content == null ? string.Empty : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
var result = Newtonsoft.Json.JsonConvert.DeserializeObject<ICollection<MemoryResponseDto>>(json, JsonSerializerSettings);
return result ?? new List<MemoryResponseDto>();
}
var error = response.Content == null ? null : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
- response.Dispose();
throw new ApiException($"Unexpected status code ({status}).", status, error, headers, null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using var request = new HttpRequestMessage(HttpMethod.Get, urlBuilder.ToString()); | |
| request.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json")); | |
| var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); | |
| var headers = new Dictionary<string, IEnumerable<string>>(); | |
| foreach (var item in response.Headers) | |
| headers[item.Key] = item.Value; | |
| if (response.Content?.Headers != null) | |
| foreach (var item in response.Content.Headers) | |
| headers[item.Key] = item.Value; | |
| var status = (int)response.StatusCode; | |
| if (status == 200) | |
| { | |
| var json = response.Content == null ? string.Empty : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | |
| var result = Newtonsoft.Json.JsonConvert.DeserializeObject<ICollection<MemoryResponseDto>>(json, JsonSerializerSettings); | |
| return result ?? new List<MemoryResponseDto>(); | |
| } | |
| var error = response.Content == null ? null : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | |
| response.Dispose(); | |
| throw new ApiException($"Unexpected status code ({status}).", status, error, headers, null); | |
| } | |
| using var request = new HttpRequestMessage(HttpMethod.Get, urlBuilder.ToString()); | |
| request.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json")); | |
| using var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); | |
| var headers = new Dictionary<string, IEnumerable<string>>(); | |
| foreach (var item in response.Headers) | |
| headers[item.Key] = item.Value; | |
| if (response.Content?.Headers != null) | |
| foreach (var item in response.Content.Headers) | |
| headers[item.Key] = item.Value; | |
| var status = (int)response.StatusCode; | |
| if (status == 200) | |
| { | |
| var json = response.Content == null ? string.Empty : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | |
| var result = Newtonsoft.Json.JsonConvert.DeserializeObject<ICollection<MemoryResponseDto>>(json, JsonSerializerSettings); | |
| return result ?? new List<MemoryResponseDto>(); | |
| } | |
| var error = response.Content == null ? null : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | |
| throw new ApiException($"Unexpected status code ({status}).", status, error, headers, null); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Api/ImmichApi.cs` around lines 100 - 123, The
HttpResponseMessage in the memories fetch path is only disposed in the non-200
branch, so the successful path in the API method leaks response resources.
Update the response handling in ImmichApi’s memories request flow so the
response is disposed on both success and error, preferably by wrapping the
SendAsync result in a using/try-finally and keeping the deserialization return
path intact.
|
Closing in favor of #660, which fixes the same issue at the NSwag serialization layer via the PrepareRequest hook — a cleaner approach than adding a parallel method. Thanks! |
1 participant
Summary
400: "Invalid input: expected ISO 8601 datetime string"when used with Immich v3SearchMemoriesAsyncserializesDateTimeOffsetusing the"s"(sortable) format, which produces datetimes like2026-07-03T07:47:17without timezone offset. Immich v3 now requires full ISO 8601 with timezone (e.g.2026-07-03T07:47:17+00:00)SearchMemoriesWithProperDateAsyncto the partialImmichApiclass (following the existingPlayAssetVideoWithRangeAsyncpattern) that manually constructs the request URL with theforparameter formatted using"yyyy-MM-ddTHH:mm:sszzz"MemoryAssetsPool.LoadAssetsto call the new method instead of the generated oneFixes #659
Test plan
forquery parameter in the HTTP request includes timezone offset (e.g.2026-07-03T07:47:17%2B00:00or2026-07-03T07:47:17Z)Summary by CodeRabbit