Skip to content

feat: add support for libraries that are shared with the user - #122

Open
rhaidiz wants to merge 13 commits into
mandarons:mainfrom
rhaidiz:shared-libraries
Open

feat: add support for libraries that are shared with the user#122
rhaidiz wants to merge 13 commits into
mandarons:mainfrom
rhaidiz:shared-libraries

Conversation

@rhaidiz

@rhaidiz rhaidiz commented Nov 4, 2025

Copy link
Copy Markdown

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 PhotosService not 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.libraries and 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 PhotosService inherit from PhotosLibrary, it feel conceptually wrong. I do understand that it might make it nice to have PhotosService behave 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 like PhotosService but 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 ownerRecordName specified, which means that it is not possible to initialize SharedPhotosService the same way as you would with PhotosService which does not require ownerRecordName.

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.

@mandarons
mandarons requested a review from Copilot November 6, 2025 23:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SharedPhotosService class that extends PhotoLibrary for accessing shared photo zones
  • Exposes shared_photos property on ICloudPyService for 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):

Comment thread icloudpy/services/photos.py Outdated
Comment on lines +298 to +313
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:

Copilot AI Nov 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread icloudpy/services/photos.py Outdated
Comment on lines +318 to +341
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

Copilot AI Nov 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The libraries property has multiple issues:

  1. If an exception occurs on line 329, zones may not be defined, causing a NameError when iterating with for zone in zones: on line 333. Initialize zones = [] before the try block.
  2. The method doesn't assign self._libraries = libraries before returning (compare with PhotosService.libraries line 273), which means subsequent calls won't use the cached value.
  3. Missing explicit return statement - should return libraries at the end.

Copilot uses AI. Check for mistakes.
Comment thread icloudpy/services/photos.py Outdated
Comment thread icloudpy/base.py Outdated
Comment on lines +277 to +341
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

Copilot AI Nov 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Service initialization with correct params
  2. Zone ID handling for shared photos
  3. The libraries property
  4. Exception handling in __init__ when zones cannot be fetched
  5. 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.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +310 to +314

# 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

Copilot AI Nov 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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
)

Copilot uses AI. Check for mistakes.
Comment thread icloudpy/base.py
Comment on lines +646 to +652
@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

Copilot AI Nov 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
mandarons and others added 7 commits July 17, 2026 13:22
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@mandarons

Copy link
Copy Markdown
Owner

Code Review: PR #122 - feat: add support for libraries that are shared with the user

Critical Bugs

  1. _shared_photos never initialized (base.py:691) — The shared_photos property checks if not self._shared_photos: but self._shared_photos is never initialized to None in __init__ (line 272-273). This will raise AttributeError on first access. Needs self._shared_photos = None added at line 273.

  2. libraries property never caches (photos.py:341) — The SharedPhotosService.libraries property returns libraries at line 341 but never assigns it to self._libraries. Compare with PhotosService.libraries (line 273) which does self._libraries = libraries. The caching check at line 319 (if not self._libraries) will never short-circuit, re-fetching every time.

  3. zones may be undefined (photos.py:313) — If the try block (line 299-308) fails with an exception, zones is never assigned, so the if not zones: check at line 313 will raise NameError. The except block should initialize zones = [].

Design Concerns

  1. ownerRecordName potentially missing from zone ID (photos.py:315) — The PR body states that shared library queries require ownerRecordName in the zone ID, yet super().__init__() is called with zones[0]["zoneID"] which may not contain it. This could cause API failures at query time. The code needs to verify whether the zone ID from /zones/list already contains ownerRecordName or if it needs to be obtained separately.

  2. Significant code duplicationSharedPhotosService is nearly identical to PhotosService (same __init__ pattern, same libraries property logic). Consider extracting the shared logic into a base class or mixin to reduce maintenance burden.

  3. Bare except Exception (lines 308, 330) — These catch all exceptions including programming errors (e.g., TypeError, KeyError). These should either catch a more specific exception or re-raise after logging, so bugs don't get silently swallowed.

Minor

  1. Inconsistent namingapi.shared_photos works but the PR description mentions api.photos.libraries as the ideal API. Consider whether this should be api.photos.shared_libraries to keep photo-related APIs grouped, or document the rationale clearly.

  2. Missing tests — No test coverage included. The PR author mentions willingness to add pytest; this should be a requirement before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Photos shared library does not work if I am not the owner of the shared library

3 participants