-
Notifications
You must be signed in to change notification settings - Fork 3
Fallback dates #137
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
Fallback dates #137
Conversation
WalkthroughThe pull request modifies the Appwrite migration destination to implement date normalization during row creation. It computes missing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Migration/Destinations/Appwrite.php:
- Around line 1044-1048: The current Unix timestamp detection around $resolved
uses a strict 10-character check and misses 9-digit (pre-2001) timestamps and
13-digit millisecond timestamps; update the logic in the method that returns
DateTime::format(new \DateTime($resolved)) to: detect numeric values
(ctype_digit or is_numeric), normalize 13-digit millisecond timestamps by
dividing by 1000, treat any numeric value within a reasonable Unix seconds range
(e.g., between -2147483648 and ~4102444800) as a seconds timestamp and prefix
with '@' before calling new \DateTime($resolved), and otherwise fall back to the
existing parse path so older 9-digit timestamps and millisecond inputs are
handled correctly.
🧹 Nitpick comments (1)
src/Migration/Destinations/Appwrite.php (1)
946-976: Date normalization logic looks good.The fallback chain for extracting timestamps from various sources is well thought out for migration scenarios.
Minor nit: the
(string)cast on line 967 is redundant since$data['$createdAt']is already a string returned fromnormalizeDateTime().Optional cleanup
if (empty($updatedAt)) { - $data['$updatedAt'] = (string) $data['$createdAt']; + $data['$updatedAt'] = $data['$createdAt']; } else {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Migration/Destinations/Appwrite.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 81
File: src/Migration/Sources/CSV.php:215-233
Timestamp: 2025-07-02T06:24:50.844Z
Learning: In the utopia-php/migration codebase, invalid numeric values (like null) in parsed data are acceptable because the underlying database structure validator will catch and handle these validation errors, so explicit filtering during parsing is not required.
🧬 Code graph analysis (1)
src/Migration/Destinations/Appwrite.php (2)
src/Migration/Resources/Database/Columns/DateTime.php (1)
DateTime(8-79)src/Migration/Resource.php (4)
getCreatedAt(209-212)getUpdatedAt(217-220)getId(127-130)getPermissions(191-194)
🔇 Additional comments (2)
src/Migration/Destinations/Appwrite.php (2)
17-17: LGTM!Import correctly added for the
DateTimeclass used in the new normalization helpers.
1051-1061: LGTM!Clean helper that correctly handles type coercion and returns
nullfor unsupported types.
| if (\is_numeric($resolved) && \strlen($resolved) === 10 && (int) $resolved > 0) { // Unix timestamp | ||
| $resolved = '@' . $resolved; | ||
| } | ||
|
|
||
| return DateTime::format(new \DateTime($resolved)); |
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.
Unix timestamp detection has edge case limitations.
The 10-character length check (\strlen($resolved) === 10) only handles timestamps from ~Sep 2001 onwards. Timestamps for dates before Sep 9, 2001 (e.g., 946684800 for Jan 1, 2000) are 9 digits and would be passed to new \DateTime() without the @ prefix, likely causing parse failures.
If migrating data from older systems is a concern, consider a more robust check:
Suggested improvement
- if (\is_numeric($resolved) && \strlen($resolved) === 10 && (int) $resolved > 0) { // Unix timestamp
+ // Unix timestamps: 9-10 digits for years 1970-2286
+ if (\is_numeric($resolved) && \preg_match('/^\d{9,10}$/', $resolved)) {
$resolved = '@' . $resolved;
}Additionally, some source systems (e.g., JavaScript) may provide millisecond timestamps (13 digits). If that's a potential source, you might also want to handle those by dividing by 1000.
📝 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.
| if (\is_numeric($resolved) && \strlen($resolved) === 10 && (int) $resolved > 0) { // Unix timestamp | |
| $resolved = '@' . $resolved; | |
| } | |
| return DateTime::format(new \DateTime($resolved)); | |
| if (\is_numeric($resolved) && \preg_match('/^\d{9,10}$/', $resolved)) { | |
| $resolved = '@' . $resolved; | |
| } | |
| return DateTime::format(new \DateTime($resolved)); |
🤖 Prompt for AI Agents
In @src/Migration/Destinations/Appwrite.php around lines 1044 - 1048, The
current Unix timestamp detection around $resolved uses a strict 10-character
check and misses 9-digit (pre-2001) timestamps and 13-digit millisecond
timestamps; update the logic in the method that returns DateTime::format(new
\DateTime($resolved)) to: detect numeric values (ctype_digit or is_numeric),
normalize 13-digit millisecond timestamps by dividing by 1000, treat any numeric
value within a reasonable Unix seconds range (e.g., between -2147483648 and
~4102444800) as a seconds timestamp and prefix with '@' before calling new
\DateTime($resolved), and otherwise fall back to the existing parse path so
older 9-digit timestamps and millisecond inputs are handled correctly.
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.