Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[develop] Opening exploded archive audiobook inside App Group fails with Publication.OpeningError.unsupportedFormat in (Designed for iPad) Mac app #434

Closed
domkm opened this issue May 22, 2024 · 9 comments · Fixed by #441
Labels
bug Something isn't working triage Triage needed by maintainers

Comments

@domkm
Copy link
Contributor

domkm commented May 22, 2024

Describe the bug

Warning: This is weird.

I am transitioning from normal local URLs created by appending path components to FileManager.default.url(for: .applicationSupportDirectory, in: .userDomainMask, appropriateFor: nil, create: false) to app group URLs created by appending path components to FileManager.default.containerURL(forSecurityApplicationGroupIdentifier: APP_GROUP_IDENTIFIER) but found it broke some of my tests. After some investigation, I found that exploded archive audiobooks fail to open when the directory is in an app group. After some more investigation, I found that it only fails while running on a Mac ( iOS simulator and hardware device work correctly).

In other words:

  • "file:///path/to/dir/file.mp3" -> success
  • "file:///path/to/dir/"-> success
  • "file:///AppGroup/path/to/dir/file.mp3" -> success
  • "file:///AppGroup/path/to/dir/" -> iPadAppOnMacOS ? failure : success

Any idea why this might be happening? If not, I'll work on a reproducible example.

How to reproduce?

Attempt to open a directory of audio files inside an App Group from a Designed for iPad app running on MacOS

Readium version

develop 2fdfea3

OS version

MacOS 14.5

Testing device

MacOS 14.5

Environment

macOS: 14.5
platform: arm64
Xcode 15.4
Build version 15F31d

Additional context

No response

@domkm domkm added bug Something isn't working triage Triage needed by maintainers labels May 22, 2024
@mickael-menu
Copy link
Member

I would expect maybe permission issues to get the children of the directory URLs? But hard to say without debugging. Do you have an error message?

@domkm
Copy link
Contributor Author

domkm commented May 22, 2024

I don't see any error message, just the .unsupportedFormat failure from Streamer.open. The contents of the directory are found when I try in a breakpoint. Could you direct me to the part of the codebase which is failing to create a Publication from the directory so I can dig into what's happening there?

@mickael-menu
Copy link
Member

You could take a look at ExplodedArchive and see if it contains the expected entries.

@domkm
Copy link
Contributor Author

domkm commented May 22, 2024

Thanks for the tip! That was what I needed to figure it out.

This is failing due to an insane behavior of App Group URLs on MacOS combined with what I think may be an unnecessary behavior in Readium.

The insane behavior is that there are apparently two types of App Group URLs. When one calls FileManager.containerURL(forSecurityApplicationGroupIdentifier: APP_GROUP_IDENTIFIER), it generates a URL pointing to ~/Library/GroupContainersAlias/APP_GROUP_IDENTIFIER/. However, that GroupContainersAlias component is, unsurprisingly, an alias. The true directory is ~/Library/Group Containers/APP_GROUP_IDENTIFIER/. Even weirder, I haven't been able to find any documentation, even unofficially, on this specific behavior. When listing the contents of a directory in an app group container, the results have aliases resolved. For example, the contents of ~/Library/GroupContainersAlias/APP_GROUP_IDENTIFIER/Assets/ID/Publication/ look like ~/Library/Group Containers/APP_GROUP_IDENTIFIER/Assets/ID/Publication/file.mp3.

The possibly unnecessary Readium behavior is that ExplodedArchive checks that each entry is a child of the root directory when making entries. I think this may be unnecessary because makeEntry is a private function that is only called by the lazy entries var when iterating over the contents of the root directory, so I think every time makeEntry is called the URL passed in must be a child of the root URL. Commenting out root.isParent(of: url) from the guard in makeEntry fixes the failure I am seeing. Regardless of this, perhaps it's worth reconsidering the functionality of FileURL.isParent. Checking the path prefix to determine parentage is not accurate without first resolving aliases in each component of both URLs. If you'd like, I'll contribute either or both of these changes.

@mickael-menu
Copy link
Member

The possibly unnecessary Readium behavior is that ExplodedArchive checks that each entry is a child of the root directory when making entries. I think this may be unnecessary because makeEntry is a private function that is only called by the lazy entries var when iterating over the contents of the root directory

Agreed, it looks unnecessary. Feel free to remove the check if that helps with this issue.

It is useful when getting an entry though, to make sure we don't request for example ../../../private/file, so fixing FileURL.isParent might be useful as well. I think you could use URL.resolvingSymlinksInPath() for that. As this will query the filesystem, could you add a FIXME comment to make sure we change FileURL.isParent to async later? Thanks!

@domkm
Copy link
Contributor Author

domkm commented May 23, 2024

URL.resolvingSymlinksInPath() would work and is what I used in my project to work around this issue. Would it make sense to add that in FileURL.init instead of .isParent since there is already normalization occurring in .init, that normalization is relevant for other functionality as well, and it would prevent symlink resolution from needing to be done repeatedly?

- let url = url.standardizedFileURL
+ let url = url.standardizedFileURL.resolvingSymlinksInPath()

@mickael-menu
Copy link
Member

I'd prefer to keep it outside the constructor because it will reach the file system and should be asynchronous.

In fact, maybe it would be better to keep isParent as is, and add instead a fileURL.resolvingSymlinks() async -> FileURL to be called before isParent().

@domkm
Copy link
Contributor Author

domkm commented May 26, 2024

Sure, I can do that if you want. I just thought that URL.standardizedFileURL (already called in init) might need to touch the file system to resolve "." and "..". Does it not do that?
Anyway, I'll proceed with the separate async version you described unless you say otherwise.

@mickael-menu
Copy link
Member

AFAIK it doesn't need to access the file system to resolve . and .., as it's purely syntactic. For example a/b/../c -> a/c.

domkm added a commit to domkm/readium-swift-toolkit that referenced this issue May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage Triage needed by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants