Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
81d792d
refactor: make AssetVersion immutable by moving filename disambiguati…
AndreyNikiforov Aug 30, 2025
d6088ca
refactor: extract calculate_version_filename into static function
AndreyNikiforov Aug 30, 2025
0bb6517
refactor: remove lp_filename_generator dependency from PhotosService
AndreyNikiforov Aug 30, 2025
a07b350
refactor: remove lp_filename_generator from PyiCloudService completely
AndreyNikiforov Aug 30, 2025
af76bdd
refactor: restore keep_unicode_in_filenames functionality with clean …
AndreyNikiforov Aug 30, 2025
890c958
refactor: remove file_match_policy parameter from PyiCloudService
AndreyNikiforov Aug 30, 2025
526c7f8
refactor: extract file_match_policy logic from calculate_filename int…
AndreyNikiforov Aug 30, 2025
f069bbf
refactor: extract filename_cleaner logic from calculate_filename into…
AndreyNikiforov Aug 30, 2025
caf2a92
Extract fingerprint hash logic from calculate_filename() into separat…
AndreyNikiforov Aug 30, 2025
ad452c3
Extract raw_policy from PyiCloudService using dependency injection pa…
AndreyNikiforov Aug 30, 2025
a6f362c
Implement fromMaybe function and replace bind_filename_with_fallback …
AndreyNikiforov Aug 30, 2025
0344d94
refactor: use base Session class instead of PyiCloudSession in PhotoA…
AndreyNikiforov Aug 30, 2025
26c96b2
refactor: extract service dependencies from PhotoLibrary and PhotoAlb…
AndreyNikiforov Aug 30, 2025
8745a5f
docs: add comprehensive refactoring context documentation
AndreyNikiforov Aug 30, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 157 additions & 0 deletions REFACTORING_CONTEXT.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
# Dependency Injection and Functional Composition Refactoring

## Summary
This refactoring series implements dependency injection patterns and functional composition throughout the icloud_photos_downloader codebase, removing tight coupling between service layers and improving testability.

## Completed Refactorings

### 1. Extract Fingerprint Hash Logic from calculate_filename()
**Commit**: `caf2a92 - Extract fingerprint hash logic from calculate_filename() into separate fallback function`

**Changes**:
- Made `calculate_filename()` return `Optional[str]` (None when filenameEnc missing)
- Created `generate_fingerprint_filename()` for fallback logic
- Created `bind_filename_with_fallback()` to handle None returns
- Updated all callers to use binding pattern

**Pattern**: Optional/Maybe pattern with fallback composition

### 2. Extract raw_policy from PyiCloudService
**Commit**: `ad452c3 - Extract raw_policy from PyiCloudService using dependency injection pattern`

**Changes**:
- Removed `raw_policy` parameter from PyiCloudService and PhotosService constructors
- Created `apply_raw_policy()` standalone function
- Added `versions_with_raw_policy()` method to PhotoAsset
- Updated all callers to pass `raw_policy` explicitly
- Updated download_builder signature to include raw_policy parameter

**Pattern**: Dependency injection - pass parameters explicitly instead of storing in service

### 3. Implement fromMaybe Function (Haskell-style)
**Commit**: `a6f362c - Implement fromMaybe function and replace bind_filename_with_fallback with functional composition`

**Changes**:
- Created `foundation.core.optional.fromMaybe()` function similar to Haskell's fromMaybe
- Replaced `bind_filename_with_fallback` with `filename_with_fallback` using fromMaybe
- Applied functional composition pattern: `fromMaybe(default)(maybe_value)`

**Pattern**: Functional composition with Higher-Order Functions

### 4. Extract Download Function from PhotoAsset
**Commit**: `0344d94 - refactor: use base Session class instead of PyiCloudSession in PhotoAsset.download`

**Changes**:
- Created standalone `download_asset()` function taking session parameter
- Updated `PhotoAsset.download()` to accept session parameter
- Removed service dependency from PhotoAsset constructor
- Used base `Session` class instead of `PyiCloudSession` for better abstraction
- Fixed all test mock functions to match new signatures

**Pattern**: Dependency injection + Interface Segregation (using base Session type)

### 5. Extract Service Dependencies from PhotoLibrary and PhotoAlbum
**Commit**: `26c96b2 - refactor: extract service dependencies from PhotoLibrary and PhotoAlbum constructors`

**Changes**:
- Updated PhotoLibrary constructor: `(service, zone_id, library_type)` → `(service_endpoint, params, session, zone_id, library_type)`
- Updated PhotoAlbum constructor: `(service, service_endpoint, ...)` → `(params, session, service_endpoint, ...)`
- Updated all callers to pass dependencies explicitly
- Fixed PhotosService inheritance with proper parent constructor
- Updated base.py to use direct properties instead of `service.*`

**Pattern**: Constructor Dependency Injection

## Key Patterns Implemented

### 1. **Dependency Injection**
- **Before**: Classes stored entire service objects and accessed nested properties
- **After**: Classes receive only the specific dependencies they need
- **Benefits**: Better testability, reduced coupling, clearer dependencies

### 2. **Functional Composition**
- **Before**: Imperative if/else logic for handling optional values
- **After**: Functional composition with higher-order functions like `fromMaybe`
- **Benefits**: More declarative code, better composability

### 3. **Optional/Maybe Pattern**
- **Before**: Methods threw exceptions or returned default values internally
- **After**: Methods return `Optional[T]` and callers handle None explicitly
- **Benefits**: Explicit error handling, no hidden defaults

### 4. **Interface Segregation**
- **Before**: Depended on concrete `PyiCloudSession` class
- **After**: Depend on base `Session` interface from requests library
- **Benefits**: More flexible, easier to mock in tests

## Architecture Impact

### Service Layer Decoupling
```
Before: PyiCloudService → PhotosService → PhotoLibrary → PhotoAlbum → PhotoAsset
(Tight coupling - each layer stores references to parent services)

After: PyiCloudService → PhotosService → PhotoLibrary → PhotoAlbum → PhotoAsset
(Loose coupling - dependencies passed explicitly as needed)
```

### Parameter Flow
```
Before: raw_policy stored in PyiCloudService, accessed via service.raw_policy
After: raw_policy passed explicitly: download_builder(raw_policy, ...) → photo.versions_with_raw_policy(raw_policy)

Before: session accessed via PhotoAsset.service.session
After: session passed explicitly: photo.download(session, url, start)

Before: PhotoLibrary accesses service.params, service.session, service.get_service_endpoint()
After: PhotoLibrary receives params, session, service_endpoint directly
```

## Quality Metrics

- **Tests**: All 203 tests pass (200 passed, 3 skipped)
- **Type Safety**: mypy strict mode passes on 73 files (src + tests)
- **Code Quality**: ruff linting passes with no issues
- **Formatting**: ruff format applied consistently

## File Changes Summary

### Core Service Files
- `src/pyicloud_ipd/services/photos.py` - Major refactoring of PhotoAsset, PhotoLibrary, PhotoAlbum, PhotosService
- `src/pyicloud_ipd/base.py` - Updated PyiCloudService and download_builder
- `src/icloudpd/base.py` - Updated callers to use new dependency injection patterns
- `src/icloudpd/download.py` - Updated to pass session explicitly to PhotoAsset.download()

### Foundation Layer
- `src/foundation/core/optional/__init__.py` - Added fromMaybe function for functional composition

### Test Files
- `tests/test_download_photos.py` - Updated mock functions for new PhotoAsset.download signature
- `tests/test_download_photos_id.py` - Updated mock functions for new PhotoAsset.download signature

## Parameters Passed to PhotosService
PhotosService receives params from PyiCloudService (initially empty dict) and adds:
```python
{
'remapEnums': True, # Tells iCloud API to remap enumeration values
'getCurrentSyncToken': True # Requests current synchronization token
}
```

These params are used in `urlencode(params)` for building iCloud API query strings throughout the photo library operations.

## Migration Guide for Future Development

### Adding New Dependencies
1. **Identify the minimal dependency needed** (not entire service objects)
2. **Pass dependencies through constructor parameters**
3. **Update all callers** to provide the dependency explicitly
4. **Write tests** that can mock the dependency easily

### Following the Patterns
1. **Use Optional[T] return types** for methods that might not find results
2. **Use fromMaybe()** for providing defaults to Optional values
3. **Pass session parameters explicitly** instead of storing in services
4. **Use base interface types** (like Session) instead of concrete implementations

This refactoring establishes a foundation for more maintainable, testable, and loosely-coupled code following functional programming and dependency injection principles.
30 changes: 30 additions & 0 deletions src/foundation/core/optional/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,33 @@ def _intern(input: _Tin | None, input2: _Tin2 | None, input3: _Tin3 | None) -> _
return None

return _intern


def fromMaybe(default: _Tin) -> Callable[[_Tin | None], _Tin]:
"""
FromMaybe function similar to Haskell's fromMaybe.
Takes a default value and returns a function that extracts the value from Maybe,
using the default if the Maybe is None.

Signature: a -> Maybe a -> a

Args:
default: The default value to return when input is None

Returns:
A function that takes an optional input and returns the value or default

Example usage:
>>> extract_or_zero = fromMaybe(0)
>>> extract_or_zero(5) == 5
True
>>> extract_or_zero(None) == 0
True
"""

def _intern(maybe_value: _Tin | None) -> _Tin:
if maybe_value is not None:
return maybe_value
return default

return _intern
10 changes: 0 additions & 10 deletions src/icloudpd/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
from icloudpd.status import Status, StatusExchange
from pyicloud_ipd.base import PyiCloudService
from pyicloud_ipd.exceptions import PyiCloudFailedMFAException
from pyicloud_ipd.file_match import FileMatchPolicy
from pyicloud_ipd.raw_policy import RawTreatmentPolicy


def prompt_int_range(message: str, default: str, min_val: int, max_val: int) -> int:
Expand Down Expand Up @@ -41,10 +39,6 @@ def echo(message: str) -> None:
def authenticator(
logger: logging.Logger,
domain: str,
filename_cleaner: Callable[[str], str],
lp_filename_generator: Callable[[str], str],
raw_policy: RawTreatmentPolicy,
file_match_policy: FileMatchPolicy,
password_providers: Dict[str, Tuple[Callable[[str], str | None], Callable[[str, str], None]]],
mfa_provider: MFAProvider,
status_exchange: StatusExchange,
Expand All @@ -68,11 +62,7 @@ def password_provider(username: str, valid_password: List[str]) -> str | None:
return None

icloud = PyiCloudService(
filename_cleaner,
lp_filename_generator,
domain,
raw_policy,
file_match_policy,
username,
partial(password_provider, username, valid_password),
response_observer,
Expand Down
30 changes: 25 additions & 5 deletions src/icloudpd/autodelete.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
import datetime
import logging
import os
from typing import Sequence, Set
from typing import Callable, Sequence, Set

from tzlocal import get_localzone

from icloudpd.paths import local_download_path
from pyicloud_ipd.asset_version import calculate_version_filename
from pyicloud_ipd.raw_policy import RawTreatmentPolicy
from pyicloud_ipd.services.photos import PhotoLibrary
from pyicloud_ipd.utils import disambiguate_filenames
from pyicloud_ipd.version_size import AssetVersionSize, VersionSize
Expand All @@ -35,6 +37,8 @@ def autodelete_photos(
folder_structure: str,
directory: str,
_sizes: Sequence[AssetVersionSize],
lp_filename_generator: Callable[[str], str],
raw_policy: RawTreatmentPolicy,
) -> None:
"""
Scans the "Recently Deleted" folder and deletes any matching files
Expand Down Expand Up @@ -70,16 +74,32 @@ def autodelete_photos(

paths: Set[str] = set({})
_size: VersionSize
for _size, _version in disambiguate_filenames(media.versions, _sizes, media).items():
versions, filename_overrides = disambiguate_filenames(
media.versions_with_raw_policy(raw_policy), _sizes, media, lp_filename_generator
)
for _size, _version in versions.items():
if _size in [AssetVersionSize.ALTERNATIVE, AssetVersionSize.ADJUSTED]:
version_filename = media.calculate_version_filename(_version, _size)
version_filename = calculate_version_filename(
media.filename,
_version,
_size,
lp_filename_generator,
media.item_type,
filename_overrides.get(_size),
)
paths.add(os.path.normpath(local_download_path(version_filename, download_dir)))
paths.add(
os.path.normpath(local_download_path(version_filename, download_dir)) + ".xmp"
)
for _size, _version in media.versions.items():
for _size, _version in media.versions_with_raw_policy(raw_policy).items():
if _size not in [AssetVersionSize.ALTERNATIVE, AssetVersionSize.ADJUSTED]:
version_filename = media.calculate_version_filename(_version, _size)
version_filename = calculate_version_filename(
media.filename,
_version,
_size,
lp_filename_generator,
media.item_type,
)
paths.add(os.path.normpath(local_download_path(version_filename, download_dir)))
paths.add(
os.path.normpath(local_download_path(version_filename, download_dir)) + ".xmp"
Expand Down
Loading
Loading