feat: add support for libraries that are shared with the user#122
feat: add support for libraries that are shared with the user#122rhaidiz wants to merge 3 commits intomandarons:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for accessing shared iCloud Photo libraries by introducing a new SharedPhotosService class that handles photos shared with the user (where they are not the owner).
- Adds
SharedPhotosServiceclass that extendsPhotoLibraryfor accessing shared photo zones - Exposes
shared_photosproperty onICloudPyServicefor accessing the shared photos service - Updates service exports to include the new
SharedPhotosService
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| icloudpy/services/photos.py | Adds SharedPhotosService class with initialization logic and libraries property for shared photo zones |
| icloudpy/services/init.py | Exports SharedPhotosService alongside existing service classes |
| icloudpy/base.py | Imports SharedPhotosService and adds shared_photos property to lazily initialize the service |
Comments suppressed due to low confidence (1)
icloudpy/services/photos.py:318
- Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
def libraries(self):
|
|
||
| self._service_endpoint = f"{self._service_root}/database/1/com.apple.photos.cloud/production/shared" | ||
|
|
||
| self._libraries = None |
There was a problem hiding this comment.
self._libraries is assigned None twice (lines 288 and 294). The duplicate assignment on line 294 should be removed.
| self._libraries = None |
| try: | ||
| # url = f"{self._service_endpoint}/zones/list" | ||
| url = f"{self._service_root}/database/1/com.apple.photos.cloud/production/shared/zones/list" | ||
| request = self.session.post( | ||
| url, | ||
| data="{}", | ||
| headers={"Content-type": "text/plain"}, | ||
| ) | ||
| response = request.json() | ||
| zones = response["zones"] | ||
| except Exception as e: | ||
| LOGGER.error(f"library exception: {str(e)}") | ||
|
|
||
| # The call to `/records/query` requires the `ownerRecordName` to be provided, which is known only after obtaining it from the API. | ||
|
|
||
| if not zones: |
There was a problem hiding this comment.
The variable zones may not be defined if the exception occurs on line 308. This will cause a NameError when checking if not zones: on line 313. Initialize zones = [] before the try block to ensure the variable is always defined.
| self._photo_assets = {} | ||
|
|
||
| try: | ||
| # url = f"{self._service_endpoint}/zones/list" |
There was a problem hiding this comment.
Commented-out code should be removed. If the alternative URL is needed for reference, it should be documented in a comment explaining why the current URL is used instead.
| # url = f"{self._service_endpoint}/zones/list" |
| def libraries(self): | ||
| if not self._libraries: | ||
| try: | ||
| url = f"{self._service_endpoint}/zones/list" | ||
| request = self.session.post( | ||
| url, | ||
| data="{}", | ||
| headers={"Content-type": "text/plain"}, | ||
| ) | ||
| response = request.json() | ||
| zones = response["zones"] | ||
| except Exception as e: | ||
| LOGGER.error(f"library exception: {str(e)}") | ||
|
|
||
| libraries = {} | ||
| for zone in zones: | ||
| if not zone.get("deleted"): | ||
| zone_name = zone["zoneID"]["zoneName"] | ||
| libraries[zone_name] = PhotoLibrary(self, zone_id=zone["zoneID"]) | ||
| # obj_type='CPLAssetByAssetDateWithoutHiddenOrDeleted', | ||
| # list_type="CPLAssetAndMasterByAssetDateWithoutHiddenOrDeleted", | ||
| # direction="ASCENDING", query_filter=None, | ||
| # zone_id=zone['zoneID']) | ||
| return libraries |
There was a problem hiding this comment.
The libraries property has multiple issues:
- If an exception occurs on line 329,
zonesmay not be defined, causing aNameErrorwhen iterating withfor zone in zones:on line 333. Initializezones = []before the try block. - The method doesn't assign
self._libraries = librariesbefore returning (compare with PhotosService.libraries line 273), which means subsequent calls won't use the cached value. - Missing explicit return statement - should
return librariesat the end.
| """The 'Photos' iCloud service. | ||
|
|
||
| This also acts as the shared library if I am not the owner of the library. |
There was a problem hiding this comment.
The docstring says "The 'Photos' iCloud service" which is identical to PhotosService. This should be more specific, such as "The 'Shared Photos' iCloud service" to clearly differentiate it from the regular PhotosService and accurately describe its purpose.
| """The 'Photos' iCloud service. | |
| This also acts as the shared library if I am not the owner of the library. | |
| """The 'Shared Photos' iCloud service. | |
| Provides access to shared photo libraries, such as shared albums or libraries | |
| that the user does not own. This is distinct from the regular PhotosService, | |
| which provides access to the user's primary photo library. |
| @property | ||
| def shared_photos(self): | ||
| """Gets the Shared 'Photo' service.""" | ||
| if not self._photos: |
There was a problem hiding this comment.
The condition if not self._photos: should check if not self._shared_photos: instead. This bug causes the method to incorrectly check the wrong instance variable, which would prevent proper lazy initialization of the shared photos service and could cause the regular photos service to be incorrectly overwritten.
| if not self._photos: | |
| if not self._shared_photos: |
| class SharedPhotosService(PhotoLibrary): | ||
| """The 'Photos' iCloud service. | ||
|
|
||
| This also acts as the shared library if I am not the owner of the library. | ||
| """ | ||
|
|
||
| def __init__(self, service_root, session, params): | ||
| self.session = session | ||
| self.params = dict(params) | ||
| self._service_root = service_root | ||
|
|
||
| self._libraries = None | ||
|
|
||
| self.params.update({"remapEnums": True, "getCurrentSyncToken": True}) | ||
|
|
||
| self._service_endpoint = f"{self._service_root}/database/1/com.apple.photos.cloud/production/shared" | ||
|
|
||
| self._libraries = None | ||
|
|
||
| self._photo_assets = {} | ||
|
|
||
| try: | ||
| # url = f"{self._service_endpoint}/zones/list" | ||
| url = f"{self._service_root}/database/1/com.apple.photos.cloud/production/shared/zones/list" | ||
| request = self.session.post( | ||
| url, | ||
| data="{}", | ||
| headers={"Content-type": "text/plain"}, | ||
| ) | ||
| response = request.json() | ||
| zones = response["zones"] | ||
| except Exception as e: | ||
| LOGGER.error(f"library exception: {str(e)}") | ||
|
|
||
| # The call to `/records/query` requires the `ownerRecordName` to be provided, which is known only after obtaining it from the API. | ||
|
|
||
| if not zones: | ||
| return | ||
| super().__init__(service=self, zone_id=zones[0]["zoneID"]) | ||
|
|
||
| @property | ||
| def libraries(self): | ||
| if not self._libraries: | ||
| try: | ||
| url = f"{self._service_endpoint}/zones/list" | ||
| request = self.session.post( | ||
| url, | ||
| data="{}", | ||
| headers={"Content-type": "text/plain"}, | ||
| ) | ||
| response = request.json() | ||
| zones = response["zones"] | ||
| except Exception as e: | ||
| LOGGER.error(f"library exception: {str(e)}") | ||
|
|
||
| libraries = {} | ||
| for zone in zones: | ||
| if not zone.get("deleted"): | ||
| zone_name = zone["zoneID"]["zoneName"] | ||
| libraries[zone_name] = PhotoLibrary(self, zone_id=zone["zoneID"]) | ||
| # obj_type='CPLAssetByAssetDateWithoutHiddenOrDeleted', | ||
| # list_type="CPLAssetAndMasterByAssetDateWithoutHiddenOrDeleted", | ||
| # direction="ASCENDING", query_filter=None, | ||
| # zone_id=zone['zoneID']) | ||
| return libraries |
There was a problem hiding this comment.
The new SharedPhotosService class lacks test coverage. Following the project's testing pattern (as seen in tests/test_photos.py for PhotosService), tests should be added to verify:
- Service initialization with correct params
- Zone ID handling for shared photos
- The
librariesproperty - Exception handling in
__init__when zones cannot be fetched - The service endpoint structure for shared photos
Add fixtures to tests/const_photos.py for shared photo zones/list responses and update tests/__init__.py::ICloudPySessionMock.request() to handle the shared photo URLs.
|
|
||
| # The call to `/records/query` requires the `ownerRecordName` to be provided, which is known only after obtaining it from the API. | ||
|
|
||
| if not zones: | ||
| return |
There was a problem hiding this comment.
The super().__init__() call only happens if zones is truthy (line 315). This means if there are no zones or an exception occurs, the SharedPhotosService instance will not be properly initialized as a PhotoLibrary, lacking essential attributes like self.zone_id and self._albums. This could cause AttributeError when accessing properties inherited from PhotoLibrary. Consider how to handle the no-zones case - either raise an exception or initialize with a default/empty zone_id.
| # The call to `/records/query` requires the `ownerRecordName` to be provided, which is known only after obtaining it from the API. | |
| if not zones: | |
| return | |
| raise ICloudPyServiceNotActivatedException( | |
| "Unable to fetch shared photo zones: " + str(e), None | |
| ) | |
| # The call to `/records/query` requires the `ownerRecordName` to be provided, which is known only after obtaining it from the API. | |
| if not zones: | |
| raise ICloudPyServiceNotActivatedException( | |
| "No shared photo zones found for this account.", None | |
| ) |
| @property | ||
| def shared_photos(self): | ||
| """Gets the Shared 'Photo' service.""" | ||
| if not self._photos: | ||
| service_root = self._get_webservice_url("ckdatabasews") | ||
| self._shared_photos = SharedPhotosService(service_root, self.session, self.params) | ||
| return self._shared_photos |
There was a problem hiding this comment.
The instance variable self._shared_photos is never initialized. It should be initialized to None in the __init__ method (around line 273 where self._photos = None is set) to follow the same pattern as other lazy-loaded services like _photos and _drive.
This PR tries to address issue #115.
I looked at the current structure of the Photos service. From my understanding, the way that the code is structured at the moment is such that
PhotosServicenot only acts as the 'Photos' iCloud service, but behaves like the user's primary library. In doing so, the assumption is that this service is tied to a specific API end-point specified in init. However, the libraries that are shared with me are available on a different end-point.My initial hope was to change the service so that I could do something like
api.photos.librariesand obtain the list of all my libraries, the ones I own and the ones I am shared with. However, I could not find a way to achieve this while reusing as much of the current code as possible and also maintaining compatibility without introducing braking changes.So here are my 2 cents: I am not sure why
PhotosServiceinherit fromPhotosLibrary, it feel conceptually wrong. I do understand that it might make it nice to havePhotosServicebehave like the user's primary library, but since this also ties the service to a specific end-point I really could not find a way to work around it.This PR is the best solution that I could think of while trying to follow the current philosophy of one service tied to one end-point: a new service,
SharedPhotosService, which essentially behaves likePhotosServicebut is dedicated to the library shared with the user.Note: unlike for queries to the private library, queries to the shared library need to have the
ownerRecordNamespecified, which means that it is not possible to initializeSharedPhotosServicethe same way as you would withPhotosServicewhich does not requireownerRecordName.Let me know what you think of this solution, I can also add some pytests to cover it and update the README to clarify the difference in the two services.