Feature/metadata management, rating to favorite, favorite updates, and metadata merge logic #1301
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit c82491d.
The test_handle_session_error_during_download now uses a cassette with an actual session error response instead of mocking PyiCloudAPIResponseException. This works because the error handling code in session.py (before commit c82491d) properly parses error responses and raises exceptions. Changes: - Created listing_photos_session_error_download.yml cassette with 401 "Invalid global session" response - Removed all mocks from test_handle_session_error_during_download - Test now relies entirely on cassette for session error simulation - Demonstrates that cassettes can trigger PyiCloudAPIResponseException without mocks This is a cleaner approach that tests the actual error handling path rather than bypassing it with mocks. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…tests - Converted 4 session error tests from using mocks to using VCR cassettes - test_handle_session_error_during_download: uses cassette with 401 response during download - test_handle_session_error_during_photo_iteration: uses cassette with 401 response during photo listing - test_handle_session_error_during_download_name_id7: uses cassette for name-id7 file match policy - test_handle_session_error_during_photo_iteration_name_id7: uses cassette for name-id7 with listing error All tests properly trigger PyiCloudAPIResponseException from HTTP responses in cassettes without needing to mock the exception directly. This proves that cassettes can simulate session errors when the error handling code is active. Note: One test (download_name_id7) needs cassette completion for the second download attempt after re-authentication. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Since MAX_RETRIES is set to 0 and retry logic was removed, the time.sleep mocks and sleep count assertions are no longer needed. The tests now only verify that re-authentication happens (by counting 'Authenticating...' messages) and that the session error message appears in the output. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove mock.patch.object for send_verification_code in test_2sa_flow_failed_send_code - Create dedicated VCR cassette (2sa_flow_failed_send_code.yml) with failed response - Cassette simulates service unavailable (success:false) response - Remove unnecessary mock import from test file This demonstrates that mocks can be replaced with more realistic VCR cassettes that test the full HTTP request/response handling stack. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Use consistent error response format with errorTitle, errorMessage, and errorCode - Match the structure used in other Apple API error responses - Remove incorrectly added 'status' field from headers section - Update content-length to match new response size The response now matches Apple's actual API format as seen in other captured cassettes like 2sa_flow_invalid_code.yml. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
… mocks - Created listing_albums_error.yml cassette with 500 status and API error response - Removed PhotoLibrary._fetch_folders mock and authenticate mock - Test now relies on cassette to trigger PyiCloudAPIResponseException - Removed unused PhotoLibrary import 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The session.py code has two paths for error handling:
1. HTTP errors (non-OK status codes)
2. API errors in JSON payload with 200 status
The test_handle_albums_error was originally mocking an API error, so the
cassette should return HTTP 200 with error in the JSON payload, not HTTP 500.
Updated cassette response to: {"success":false,"error":"Api Error","errorCode":"100"}
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
- test_handle_albums_error_name_id7: uses same cassette as albums_error test - test_handle_internal_error_during_download: uses cassette with 500 status during photo download Key learnings: - API errors (200 with error JSON) vs HTTP errors (4xx/5xx status) are handled differently in session.py - Download errors need HTTP status errors (500) since downloads use streaming and the error handling differs from regular JSON API calls - Removed unused PhotoLibrary and constants imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Download errors behave differently than API errors: - API endpoints (albums, photo listing): Can use HTTP 200 with error JSON - Download endpoints: Must use HTTP error status (4xx/5xx) because downloads use streaming and don't parse JSON responses The test behavior changes slightly: - Original mock: Raised PyiCloudAPIResponseException with "INTERNAL_ERROR" - Cassette: HTTP 500 results in "Could not find URL to download" message - Exit code changes from 1 to 0 as error is handled gracefully This is an acceptable difference as the error is still caught and handled, just reported differently. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove is_streaming_response check that was preventing exceptions for streaming downloads - Add special handling to exclude 404 errors from raising exceptions - Update cassette to return HTTP 500 for download errors - Ensure download errors raise exceptions and cause exit code 1 The is_streaming_response check was preventing the session from raising exceptions for HTTP errors on streaming downloads, causing inconsistent behavior between mock tests and cassette tests. Checking response status doesn't load the stream into memory since it's part of the HTTP headers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Convert test_handle_internal_error_during_photo_iteration to use cassette - Convert test_handle_internal_error_during_download_name_id7 to use cassette - Add cassettes for simulating HTTP 500 errors during photo operations - Remove mock dependencies from converted tests - Clean up unused imports These tests now use actual HTTP responses via cassettes instead of mocks, providing more realistic testing of error handling behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The session error cassettes now include successful download after re-authentication, demonstrating proper retry behavior. Updated test comments to accurately reflect cassette contents and expected behavior. Both tests now: - Experience session error on first download attempt - Re-authenticate automatically - Successfully download on retry - Exit with code 0 (success) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Minor formatting improvements to session error test comments and file download expectations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…cassettes The listing_photos_session_error_iteration.yml and listing_photos_session_error_iteration_name_id7.yml cassettes had delete file requests at the end that shouldn't be there. These have been removed. Both tests still pass after the cleanup. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Changed line 1012 from raising PyiCloudServiceNotActivatedException to returning ADT - All tests passing (19 passed, 1 skipped)
- Changed line 1016 from raising PyiCloudAPIResponseException to returning ADT - Skipped AuthServiceUnavailable as it breaks watch functionality - All tests passing (19 passed, 1 skipped)
- Replace PyiCloudServiceNotActivatedException with ADT return - Add special logging in pattern matching to preserve error message - All tests passing (218 passed)
- Document successful conversion of 7/10 authentication errors - Identify 3 cases that must remain as exceptions - Add learnings about watch mode and MFA requirements - Note that service access errors need separate refactoring
- Replace AuthUnexpectedError with specific AuthConnectionError ADT - Only catch PyiCloudConnectionErrorException (network errors) with ADT - Let all other unexpected exceptions propagate and crash the program - This aligns with the principle that we only handle expected IO errors - Removes unnecessary complexity from error handling - All 218 tests passing
- Authentication now returns ADT results instead of throwing exceptions - Moved authentication code outside the try block - Removed PyiCloudFailedLoginException handler (no longer raised) - Converted PyiCloudFailedMFAException to direct ADT handling - Kept necessary exception handlers for service/IO errors - This completes the authentication exception-to-ADT refactoring - All 218 tests passing
…e_run - Replace PyiCloud2SARequiredException with Response2SARequired return in private libraries check - Replace PyiCloudServiceNotActivatedException with ResponseServiceNotActivated return in private libraries check - All tests pass with these minimal changes
…es check - Replace PyiCloudAPIResponseException with ResponseAPIError return - Replace PyiCloudServiceUnavailableException with ResponseServiceUnavailable return - All tests continue to pass
- Replace all four exception types with corresponding ADT returns - Maintains consistent error handling pattern - All tests pass
…h ADT return at line 1331
- Removed get_album_count function that raised exceptions - Replaced functional programming approach with direct for loop - Added inline error handling for session errors - Invalid global session errors now trigger re-authentication - Other API errors are logged and return error code directly - All tests pass, mypy strict mode passes
- Added error_occurred flag to track when retry is needed - Replaced exception raising in photo iteration with direct handling - Session errors trigger retry via flag instead of exception - WebUI errors that can be retried also use flag mechanism - Other errors return error code directly - All tests pass, mypy strict mode passes
- Replaced exception raising in download result handling with direct error handling - Used needs_retry flag to coordinate retry logic across nested loops - Session errors during download now trigger retry via flag mechanism - 500 errors and service unavailable return error code directly - Other API errors are logged but continue processing - Fixed retry logic to properly break from albums loop - All tests pass, mypy strict mode passes
- Replaced exception raising in delete photo result handling with direct error handling - Session errors during delete now trigger retry via needs_retry flag - 500 errors return error code 1 (important for delete operations) - Service unavailable returns error code 1 - Other API errors are logged but continue processing - All tests pass, mypy strict mode passes
- Replaced exception raising in autodelete result handling with direct error handling - Session errors trigger retry by continuing the while loop - WebUI errors that can be retried also continue the loop - Service unavailable returns error code 1 - Other errors return appropriate ADT results or error codes - Removed unused PyiCloud2SARequiredException import - All tests pass, mypy strict mode passes
Replace try-except block with direct ADT error handling throughout core_single_run function. All error cases now use pattern matching directly without exception catching. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ting favorites, and overwrite metadata cli options. Combine the EXIF and XMP sidecar code into one class with tests.
…f and xmp files as funcitonality merged
Author
|
Reference issue: #1296 |
This was referenced Jan 2, 2026
AndreyNikiforov
force-pushed
the
master
branch
from
f09784b to
dd5a421
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature/metadata management, rating to favorite, favorite updates, and metadata merge logic
"As a iPhotos user I often favorite my photos days after I take them, yet sync my photos daily so need to be able to update the favorite status post downloading. Favoriting is an important piece of metadata and should be included in files where possible for completeness"
"As a user I make changes to both EXIF and XMP metadata and need to ensure that syncing Favorites and other metadata from icloudpd will not overwrite the metadata changes I have made"
In this PR I
Add user facing CLI options:
--favorite-to-rating: Specify howisFavoriteis expressed in the EXIF and XMPratingtag,--process-existing-favorites: Whether to process favorites on existing downloads, best if used in combination with--until-foundusing a sensible window which accounts for the number of photos between taking and rating--metadata-overwrite: Whether to overwrite existing metadata or not. Defaults to not overwriting metadata. For example if I use--favorite-to-rating 1and update the rating to 3 (I really like the photo), then a following run with--process-existing-favoriteswill not overwrite it. Wheras if the rating is unset then it is safe to set.I have combined the
xmp_sidecar.pyandexif_datetime.pyfunctionality (and tests) intometadata_management.py(and associatedtest_metadata_management.pyto keep the metadata concerns together (XMP and EXIF are just i/o implementations of the same thing essentially), and cleaned up some of the calling logic inbase.py, moving it intometadata_management.pyto keep the concerns more distinct.I have formatted, linted, and ensured high test , whilst reducing some complexity in
base.py, I hope you enjoy 🤗