Various improvements

[tladesignz]

@redaranj, let's discuss this, esp. my Additional findings!

Changes in this pull request

• Improved SigningAlgorithm enum to avoid switching. Strengthens guarantees already at compile time.
• Use an enum for settings format (json, toml) to avoid misuse of API.
• Replace print with proper logging.
• Use LocalizedError instead of CustomStringConvertible on Error types.
• Got rid of global namespace pollution for C2PA version evaluation.
• Fixed mentions of outdated Stream(fileURL:) in documentation.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• All applicable changes have been documented
• Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment

Additional findings

URL arguments

All URL arguments seem to be of type String. To improve clarity and discourage wrong usage
of the API, I would suggest, to make all of them of type URL.

Even though, many of them are converted back to String on usage.

You could then also drop the "URL" suffix on the argument names.

In WebServiceSigner, you could then also drop these lines:

    private func fetchConfiguration() async throws -> SignerConfiguration {
        guard let url = URL(string: configurationURL) else {
            throw SignerError.invalidURL
        }

URLSession

WebServiceSigner hardcodes URLSession.shared usage.

This can be problematic for consuming apps, as they might also use some tunneling (e.g. Tor…)

I suggest to add an optional init arg, so apps can hand over their preconfigured URLSession.

C2PAError.api

It seems, someone got lazy and used C2PAError.api(String) instead of adding more error cases.

In at least one place, I found this:

                    throw C2PAError.api("Secure Enclave signing failed: \(error)")

I suggest to replace them with custom errors resp. throw the actual error directly.

In case a consuming app wants to something special on specific errors, they would otherwise
resort to string matching, which is seriously ugly.

Also, this is misleading debugging.

[codecov]

Codecov Report

❌ Patch coverage is 63.79310% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.61%. Comparing base (a69c193) to head (1919013).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
Library/Sources/C2PA.swift	63.88%	13 Missing ⚠️
Library/Sources/SecureEnclaveSigner.swift	0.00%	9 Missing ⚠️
Library/Sources/KeychainSigner.swift	0.00%	6 Missing ⚠️
Library/Sources/Signer.swift	53.84%	6 Missing ⚠️
Library/Sources/WebServiceSigner.swift	83.33%	5 Missing ⚠️
Library/Sources/Builder.swift	66.66%	1 Missing ⚠️
Library/Sources/Reader.swift	50.00%	1 Missing ⚠️
Library/Sources/SigningAlgorithm.swift	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   54.26%   55.61%   +1.34%     
==========================================
  Files          39       39              
  Lines        1301     1291      -10     
==========================================
+ Hits          706      718      +12     
+ Misses        595      573      -22     

Flag	Coverage Δ
unittests	55.61% <63.79%> (+1.34%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tladesignz]

Aaaah, almost forgot this one:

C2PA namespace overload

Unfortunately, the package namespace C2PA and the helper enum C2PA have identical names. This is a problem in Swift, as the enum shadows the namespace.

As a consuming app, I get an unresolvable problem under the following conditions:

• I have objects/structs of the names Builder, Reader, Stream, CertificateManager in my own project. (Esp. these ones are super generic names and tend to collide with other things.)
• I import C2PA.
• I cannot reference C2PA.Stream now, as the enum C2PA captures the namespace identifier and enum C2PA doesn't have the Stream object referenced.
• I also cannot reference my own Stream object now, as the compiler is confused about which one to use. And there's no way to distinguish, since the C2PA namespace name was overridden with the enum C2PA.
• There's no way to partially import things in Swift or rename namespaces on import.

I completely get the idea of the C2PA enum, it's a convenient helper. And i also cannot recommend to instead pollute the global namespace.

TBH: I myself would need some more time to come up with the clean solution here.

Well, 2 hard things in programming: null pointers and naming. (And I think the modern languages pretty much solved the null pointer problem.)

Anyway, the situation as it is now is very much not good and needs to change.

Let me know, if I should spend some more time to find that good solution.

[redaranj]

@tladesignz Per our discussion in the chat, please proceed with fixes for the ‘additional findings’ and suggest a plan for how to deal with the C2PA namespace problem.

[tladesignz]

Since this library is working on macOS, too, the Github handle c2pa-ios is a misnomer.

How about renaming the library to swift-c2pa (Github handle) resp. SwiftC2PA?

This would kill 2 birds with one stone.

[tladesignz]

At least, the repo should be renamed. I use apple as a prefix for my libs, which support multiple environments: c2pa-apple.