fix: Deep save nested files#461
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates ensureDeepSave to deduplicate ParseFile uploads within a batch and to enqueue unsaved ParseEncodable children into nextBatch, and adds tests (GameWithNestedFile + testDeepSaveNestedObjectWithFile) validating a nested object with a ParseFile is saved correctly. ChangesNested Object Deep Save Fix
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.11][ERROR]: Error: exception Unix_error: No such file or directory stat Sources/ParseSwift/Objects/ParseObject+async.swift 🔧 SwiftLint (0.63.2)Sources/ParseSwift/Objects/ParseObject+async.swiftError: No lintable files found at paths: 'Sources/ParseSwift/Objects/ParseObject+async.swift' Sources/ParseSwift/Objects/ParseObject.swiftError: No lintable files found at paths: 'Sources/ParseSwift/Objects/ParseObject.swift' 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 |
|
Ready for review. This is scoped to #425: it preserves nested unsaved children across deep-save batches and adds callback/async regression coverage for a nested object containing a ParseFile. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/ParseSwift/Objects/ParseObject.swift (1)
1453-1456:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent duplicate file uploads after nested-child requeueing
With the new requeue at Line 1470, a
ParseFilecan reappear in a later loop after it was already saved; Line 1453 currently schedules it again. That can cause duplicate uploads/orphaned files.Suggested fix
- if let parseFile = parseType as? ParseFile { - // ParseFiles can be saved now - savableFiles.append(parseFile) + if let parseFile = parseType as? ParseFile { + // ParseFiles can be saved now, unless they were already saved in a prior batch + if filesFinishedSaving[parseFile.id] == nil { + savableFiles.append(parseFile) + }Also applies to: 1469-1475
🤖 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 `@Sources/ParseSwift/Objects/ParseObject.swift` around lines 1453 - 1456, The loop that builds savableFiles is adding ParseFile instances multiple times because requeued nested children can reintroduce the same parseType; update the logic around parseType handling (where ParseFile is cast and appended to savableFiles) to deduplicate before appending — e.g., check existing savableFiles for the same ParseFile identity/unique key (file name, URL or objectId) and only append if not already present; apply the same dedupe check where requeueing occurs so the block handling ParseFile append (the ParseFile cast and savableFiles array) and the requeue branch avoid scheduling duplicates.Sources/ParseSwift/Objects/ParseObject+async.swift (1)
317-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror fix in async deep-save to avoid repeat file saves
Line 333 now requeues nested children; without checking already-saved files at Line 317, async deep-save can upload the same file more than once.
Suggested fix
- if let parseFile = parseType as? ParseFile { - // ParseFiles can be saved now - savableFiles.append(parseFile) + if let parseFile = parseType as? ParseFile { + // ParseFiles can be saved now, unless they were already saved in a prior batch + if filesFinishedSaving[parseFile.id] == nil { + savableFiles.append(parseFile) + }Also applies to: 332-339
🤖 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 `@Sources/ParseSwift/Objects/ParseObject`+async.swift around lines 317 - 320, The async deep-save path is requeuing nested children later, so adding parseFile to savableFiles unconditionally (in the block handling parseType as? ParseFile) can cause the same file to be uploaded multiple times; update the block that handles parseType/parseFile (where savableFiles.append(parseFile) is called) to first check whether that file is already scheduled—e.g., use savableFiles.contains(where: { existing.isSameFile(as: parseFile) }) or maintain a Set/unique collection keyed by the file's stable identifier (name/url/objectId) and only append when not already present—so duplicate file entries are avoided during the async deep-save flow.
🧹 Nitpick comments (2)
Tests/ParseSwiftTests/ParseObjectTests.swift (1)
2111-2116: ⚡ Quick winAssert saved file metadata, not just count
At Line 2112, consider asserting the saved file
name/urlto verify payload preservation, not only that one file exists.Suggested assertion additions
game.ensureDeepSave { (savedChildren, savedChildFiles, parseError) in XCTAssertNil(parseError) XCTAssertEqual(savedChildFiles.count, 1) + XCTAssertEqual(savedChildFiles.first?.value.name, fileResponse.name) + XCTAssertEqual(savedChildFiles.first?.value.url, fileResponse.url) XCTAssertEqual(savedChildren.count, 1) XCTAssertEqual(savedChildren.first?.value.className, "Game2") XCTAssertEqual(savedChildren.first?.value.objectId, "nested") expectation1.fulfill() }🤖 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 `@Tests/ParseSwiftTests/ParseObjectTests.swift` around lines 2111 - 2116, The test currently only checks savedChildFiles.count but doesn't assert the file metadata; update the assertions around XCTAssertNil(parseError), savedChildFiles and savedChildren in ParseObjectTests to verify the saved file's name and URL (e.g., assert savedChildFiles.first?.name == "<expectedName>" and savedChildFiles.first?.url?.absoluteString == "<expectedURL>") before calling expectation1.fulfill(), so the payload's file metadata is preserved; locate the checks using the identifiers savedChildFiles, savedChildren, XCTAssertNil(parseError) and expectation1.fulfill() and add the metadata assertions there.Tests/ParseSwiftTests/ParseObjectAsyncTests.swift (1)
1892-1895: ⚡ Quick winStrengthen async assertion for saved file contents
At Line 1892, please also assert saved file
nameandurlto lock in file data integrity for this regression.Suggested assertion additions
let (savedChildren, savedChildFiles) = try await game.ensureDeepSave() XCTAssertEqual(savedChildFiles.count, 1) + XCTAssertEqual(savedChildFiles.first?.value.name, fileResponse.name) + XCTAssertEqual(savedChildFiles.first?.value.url, fileResponse.url) XCTAssertEqual(savedChildren.count, 1) XCTAssertEqual(savedChildren.first?.value.className, "Game2") XCTAssertEqual(savedChildren.first?.value.objectId, "nested")🤖 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 `@Tests/ParseSwiftTests/ParseObjectAsyncTests.swift` around lines 1892 - 1895, The test currently only checks counts and the saved child object's className/objectId (savedChildFiles, savedChildren, savedChildren.first?.value); add assertions to also verify the saved file's name and url to ensure file data integrity—locate the savedChildFiles/savedChildren results in ParseObjectAsyncTests.swift (the block using savedChildFiles.count and savedChildren.first?.value) and assert that the saved file entry has the expected name and that its url matches the expected upload URL (or is non-empty/valid) to lock in the file contents for this regression.
🤖 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.
Outside diff comments:
In `@Sources/ParseSwift/Objects/ParseObject.swift`:
- Around line 1453-1456: The loop that builds savableFiles is adding ParseFile
instances multiple times because requeued nested children can reintroduce the
same parseType; update the logic around parseType handling (where ParseFile is
cast and appended to savableFiles) to deduplicate before appending — e.g., check
existing savableFiles for the same ParseFile identity/unique key (file name, URL
or objectId) and only append if not already present; apply the same dedupe check
where requeueing occurs so the block handling ParseFile append (the ParseFile
cast and savableFiles array) and the requeue branch avoid scheduling duplicates.
In `@Sources/ParseSwift/Objects/ParseObject`+async.swift:
- Around line 317-320: The async deep-save path is requeuing nested children
later, so adding parseFile to savableFiles unconditionally (in the block
handling parseType as? ParseFile) can cause the same file to be uploaded
multiple times; update the block that handles parseType/parseFile (where
savableFiles.append(parseFile) is called) to first check whether that file is
already scheduled—e.g., use savableFiles.contains(where: {
existing.isSameFile(as: parseFile) }) or maintain a Set/unique collection keyed
by the file's stable identifier (name/url/objectId) and only append when not
already present—so duplicate file entries are avoided during the async deep-save
flow.
---
Nitpick comments:
In `@Tests/ParseSwiftTests/ParseObjectAsyncTests.swift`:
- Around line 1892-1895: The test currently only checks counts and the saved
child object's className/objectId (savedChildFiles, savedChildren,
savedChildren.first?.value); add assertions to also verify the saved file's name
and url to ensure file data integrity—locate the savedChildFiles/savedChildren
results in ParseObjectAsyncTests.swift (the block using savedChildFiles.count
and savedChildren.first?.value) and assert that the saved file entry has the
expected name and that its url matches the expected upload URL (or is
non-empty/valid) to lock in the file contents for this regression.
In `@Tests/ParseSwiftTests/ParseObjectTests.swift`:
- Around line 2111-2116: The test currently only checks savedChildFiles.count
but doesn't assert the file metadata; update the assertions around
XCTAssertNil(parseError), savedChildFiles and savedChildren in ParseObjectTests
to verify the saved file's name and URL (e.g., assert
savedChildFiles.first?.name == "<expectedName>" and
savedChildFiles.first?.url?.absoluteString == "<expectedURL>") before calling
expectation1.fulfill(), so the payload's file metadata is preserved; locate the
checks using the identifiers savedChildFiles, savedChildren,
XCTAssertNil(parseError) and expectation1.fulfill() and add the metadata
assertions there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d5c0f75-d3f8-40ea-9b2c-7bddb55b118b
📒 Files selected for processing (4)
Sources/ParseSwift/Objects/ParseObject+async.swiftSources/ParseSwift/Objects/ParseObject.swiftTests/ParseSwiftTests/ParseObjectAsyncTests.swiftTests/ParseSwiftTests/ParseObjectTests.swift
|
Addressed the CodeRabbit follow-up in b6743d2: both sync and async deep-save paths now skip ParseFiles that were already saved or already scheduled in the current batch, and the nested-file regression tests assert the saved file name and URL as well as count.\n\nVerification:\n- |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ParseSwift/Objects/ParseObject+async.swift (1)
336-340: ⚡ Quick winThe optional cast pattern here is defensive but safe in practice.
Both
ParseObject(viaObjectable) andParseFile(viaFileable) conform toParseEncodable, and these are the only types added tounsavedChildrenby the encoder. Theas? ParseEncodablecast will always succeed for items in this array. While the encoder's return type signature is[Encodable]rather than[ParseEncodable], creating a type safety gap, the code works correctly in practice.If you'd like to document this invariant, consider adding a comment clarifying that unsavedChildren should only contain
ParseEncodabletypes, though defensive logging is optional.🤖 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 `@Sources/ParseSwift/Objects/ParseObject`+async.swift around lines 336 - 340, The cast to ParseEncodable inside the loop over waitingObjectInfo.unsavedChildren is defensive but always succeeds because only ParseObject (via Objectable) and ParseFile (via Fileable) are ever appended there; add a short clarifying comment above the loop referencing waitingObjectInfo.unsavedChildren and the invariant ("unsavedChildren only contains ParseEncodable (ParseObject/File) instances") so future readers understand why the as? cast is safe, and leave the current code (including nextBatch.append(parseEncodable)) unchanged.
🤖 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.
Nitpick comments:
In `@Sources/ParseSwift/Objects/ParseObject`+async.swift:
- Around line 336-340: The cast to ParseEncodable inside the loop over
waitingObjectInfo.unsavedChildren is defensive but always succeeds because only
ParseObject (via Objectable) and ParseFile (via Fileable) are ever appended
there; add a short clarifying comment above the loop referencing
waitingObjectInfo.unsavedChildren and the invariant ("unsavedChildren only
contains ParseEncodable (ParseObject/File) instances") so future readers
understand why the as? cast is safe, and leave the current code (including
nextBatch.append(parseEncodable)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68bc947e-776a-4c33-8756-f887e35de708
📒 Files selected for processing (4)
Sources/ParseSwift/Objects/ParseObject+async.swiftSources/ParseSwift/Objects/ParseObject.swiftTests/ParseSwiftTests/ParseObjectAsyncTests.swiftTests/ParseSwiftTests/ParseObjectTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/ParseSwift/Objects/ParseObject.swift
- Tests/ParseSwiftTests/ParseObjectAsyncTests.swift
- Tests/ParseSwiftTests/ParseObjectTests.swift
|
Addressed the remaining CodeRabbit nit in 39c6f78 by documenting the Validation:
|
1 participant
Closes #425.
What changed
ensureDeepSavepaths.ParseFile.Verification
git diff --checkswift build -Xswiftc -suppress-warningsswift test --filter ParseObjectTests.testDeepSaveNestedObjectWithFile -Xswiftc -suppress-warningscould not complete in this local environment because the installed Swift toolchain cannot importXCTest.Summary by CodeRabbit
Bug Fixes
Tests