[1] Feat: Add context, settings, and JSON APIs#47
[1] Feat: Add context, settings, and JSON APIs#47redaranj wants to merge 2 commits intocontentauth:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
============================================
+ Coverage 59.82% 65.78% +5.96%
Complexity 9 9
============================================
Files 52 56 +4
Lines 1558 1920 +362
Branches 162 218 +56
============================================
+ Hits 932 1263 +331
+ Misses 526 510 -16
- Partials 100 147 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
redaranj
force-pushed
the
feature/context-settings-api
branch
from
f43629c to
3e2acab
Compare
Introduce C2PAContext and C2PASettings for shared configuration across readers and builders. Add C2PAJson for centralized JSON serialization. Add SettingsValidator for validating C2PA settings. Rewrite Builder to use context-based creation flow. Add Reader context support with withStream and withFragment. Update native library to v0.75.19.
redaranj
force-pushed
the
feature/context-settings-api
branch
from
3e2acab to
22a7f35
Compare
| import org.contentauth.c2pa.SigningAlgorithm | ||
|
|
||
| /** | ||
| * Validates C2PA settings JSON/TOML for schema compliance and provides warnings for common issues. |
There was a problem hiding this comment.
This is very cool, but … it's at risk of getting out of date as we evolve the set of settings. Is there a straightforward way to track such changes?
There was a problem hiding this comment.
@scouten-adobe Perhaps I could extend the current CI c2pa-rs version check (or maybe better, add another scheduled job), that checks for changes in specific files (FFI, settings, etc) and alerts or opens a PR when something new is detected.
Overall, this brings up an issue that I'm working through: where is the line where I defer to c2pa-rs? My general goal has been that devs can look just at the mobile library (not c2pa-rs or the spec) and understand what's possible. The options I considered here are:
- accept any settings string and pass it straight through, refer users to c2pa-rs docs for details (no maintenance, but not great developer experience)
- same thing, but include docs on the currently available settings (light maintenance)
- create a struct that mirrors the shape of the settings and require that as an argument everywhere (very nice, informative developer experience, but creates a hard requirement to update the struct when the allowable settings change)
- middle path 1: create the struct, but accept string settings everywhere, users can either pass a raw string or a serialized struct (requires maintenance, but doesn't block people from adopting new settings if the struct hasn't been updated)
- middle path 2: accept raw settings string, but give users a way to validate their structure against the current schema (similar tradeoffs as previous one)
Let me know if you have a preference here. This issue will come up again in a subsequent PR where we have created a similar manifest validator.
There was a problem hiding this comment.
I think my preference is for your "middle path 1" but I'll ask a couple other people on the team for their opinions as well.
There was a problem hiding this comment.
For our JS APIs we added a tool that generates JSON schema and we generated the client side apis using htat. You could leverage the schema to validate and update it whenever it changes.
There was a problem hiding this comment.
Yes I prefer Gavin's suggestion. Is it possible to generate types from the JSON schema?
There was a problem hiding this comment.
I exported the schema using the export_schema CLI tool in c2pa-rs, then created a Kotlin data class using quicktype. It needed some significant changes to make it fit with the rest of the project, so I don't think I'd probably do it that way again (though I'm happy with how it turned out after the tweaks). I removed all of the validation code, and now we can just pass a nice SettingsDefinition struct to the Settings class and it'll serialize it for us. Passing a raw JSON or TOML string is still allowed too.
I also created a CI job that runs the schema exports for current vs. previous tag and then makes an issue if it detects a change. So there will be some level of manual labor on schema change, but at least we'll know when we have to do it.
| * @see ManifestValidator | ||
| * @see SettingsValidator | ||
| */ | ||
| data class ValidationResult( |
There was a problem hiding this comment.
I think this class should have a different name since "validation result" has a specific meaning in C2PA: https://spec.c2pa.org/specifications/specifications/2.3/specs/C2PA_Specification.html#_returning_validation_results.
There was a problem hiding this comment.
Makes sense, I will rename it.
There was a problem hiding this comment.
In the end I removed it completely because I went with the data class route for the settings instead of string validation.
library/src/main/kotlin/org/contentauth/c2pa/manifest/ValidationResult.kt
Outdated
Show resolved
Hide resolved
|
@redaranj I finished reading the PR and have no further comments; ping me when you're ready for re-review. |
…eck for schema changes
4 participants
Changes in this pull request
Types of changes
Checklist
TO DOitems (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment