Skip to content

feat: Add cr_json() and cr_json_value() to Reader; remove separate CrJsonReader#1919

Merged
gpeacock merged 28 commits intomainfrom
gpeacock/cr_json_tests
Mar 23, 2026
Merged

feat: Add cr_json() and cr_json_value() to Reader; remove separate CrJsonReader#1919
gpeacock merged 28 commits intomainfrom
gpeacock/cr_json_tests

Conversation

@gpeacock
Copy link
Collaborator

This avoids a requirement for a separate Reader and porting that to other languages.
Another option would be to add a setting to default to crJson format, in which case there are no new language porting requirements.

@gpeacock gpeacock requested a review from lrosenthol March 10, 2026 18:37
Copy link
Contributor

@lrosenthol lrosenthol left a comment

Choose a reason for hiding this comment

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

A bit confused since I don't see changes to Reader, but still see CrJsonReader...

Copy link
Contributor

@lrosenthol lrosenthol left a comment

Choose a reason for hiding this comment

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

Ah, I see what you. You moved all the crJSON code inot a new file with two new exported methods that then callable from the current Reader.

Fine with me.

This is to not introduce the new class that needs to be bound to other langs?

Copy link
Contributor

@lrosenthol lrosenthol left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lrosenthol lrosenthol left a comment

Choose a reason for hiding this comment

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

Doesn't build

   Compiling c2pa v0.77.0 (/Users/lrosenth/Development/c2pa-rs/sdk)
error[E0432]: unresolved import `sigtst::timestamp_token_bytes_from_sign1`
  --> sdk/src/crypto/cose/mod.rs:50:5
   |
50 |     timestamp_token_bytes_from_sign1, timestamptoken_from_timestamprsp, validate_cose_tst_info,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `timestamp_token_bytes_from_sign1` in `crypto::cose::sigtst`

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
   --> sdk/src/crjson.rs:526:28
    |
526 |                     if let Some(token_bytes) = timestamp_token_bytes_from_sign1(&sign1) {
    |                            ^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: the trait `Sized` is not implemented for `[u8]`
note: required by a bound in `std::prelude::v1::Some`
   --> /Users/lrosenth/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:600:17
    |
600 | pub enum Option<T> {
    |                 ^ required by this bound in `std::prelude::v1::Some`
...
608 |     Some(#[stable(feature = "rust1", since = "1.0.0")] T),
    |     ---- required by a bound in this tuple variant

Some errors have detailed explanations: E0277, E0432.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `c2pa` (lib) due to 2 previous errors

@lrosenthol
Copy link
Contributor

It also appears that you took an older version of my code - as this has the old validation stuff, and needs my newer one.

lrosenthol and others added 2 commits March 10, 2026 17:28
- Renamed "validationResults" to "validationInfo" in the schema to better reflect its purpose.
- Enhanced the validationInfo to include a summary of validation information and validation time.
- Updated the CrJsonExporter to build and include validationInfo in the output.
- Modified the validationResults structure to clarify the distinction between document-level and per-manifest validation results.
- Added support for ingredient deltas in the manifest validation results.
- Updated tests to ensure compliance with the new schema structure.
@gpeacock
Copy link
Collaborator Author

This is a request to merge into the crjson branch, but if you have a newer branch we could merge into that.

@lrosenthol
Copy link
Contributor

@gpeacock do we need to down merge this to crJSON or can we just merge it right into main? I think we're good on this branch - I am ready to merge it to main if you are!

@scouten-adobe scouten-adobe changed the title Adds cr_json() and cr_json_value() to Reader, removes separate CrJsonReader. feat: Add cr_json() and cr_json_value() to Reader; remove separate CrJsonReader Mar 12, 2026
lrosenthol and others added 3 commits March 16, 2026 15:04
- Removed the document-level `validationInfo` in favor of per-manifest `validationResults`.
- Introduced `manifestValidationResults` to encapsulate validation status codes and validation time for each manifest.
- Updated `CrJsonExporter` to build validation results per manifest, ensuring each manifest's validation status is accurately represented.
- Adjusted tests to reflect the new schema structure and validation results handling.
…ompliance tests

- Rewrite crjson.rs using typed serde::Serialize structs (CrJsonClaim,
  CrJsonManifest, CrJsonSignature, etc.) for explicit schema compliance
- Eliminate Manifest dependency; drive output directly from Store/Claim APIs
- Add Reader::crjson() / crjson_checked() methods
- Add --crjson flag to c2patool (conflicts with --detailed)
- Add jsonschema dev-dependency (default-features = false for WASM compat)
  and rewrite schema_compliance tests to validate against the crJSON spec
- Consolidate hash_assertions tests; delete redundant hash_encoding.rs
- Revert validation_results.rs regression from commit 26fa080
@gpeacock gpeacock changed the base branch from crJson to main March 17, 2026 03:44
@lrosenthol
Copy link
Contributor

I should have one more set of changes to make based on latest feedback...

lrosenthol and others added 3 commits March 18, 2026 10:21
- Removed the `date` field from the `jsonGenerator` object in the crJSON schema.
- Added `specVersion` to the validation results, ensuring it follows the SemVer format.
- Updated the `validationTime` field to be required in the validation results.
- Adjusted the `CrJsonExporter` to reflect these schema changes, including updates to the handling of validation results and hash encoding.
- Modified tests to ensure compliance with the new schema, including checks for the `b64'` prefix in hash fields.
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 19, 2026

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing gpeacock/cr_json_tests (8a14a6d) with main (b6d4ebc)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@lrosenthol lrosenthol self-requested a review March 19, 2026 23:03
Copy link
Contributor

@lrosenthol lrosenthol left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 81.08614% with 101 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.57%. Comparing base (b6d4ebc) to head (8a14a6d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
sdk/src/crjson.rs 83.66% 75 Missing ⚠️
sdk/src/crypto/time_stamp/verify.rs 54.54% 20 Missing ⚠️
cli/src/main.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1919      +/-   ##
==========================================
+ Coverage   76.50%   76.57%   +0.07%     
==========================================
  Files         172      173       +1     
  Lines       41142    41671     +529     
==========================================
+ Hits        31475    31909     +434     
- Misses       9667     9762      +95     

☔ 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.

…ndling

Change crjson module from pub to pub(crate) (internal implementation detail)
Remove manual hash/alg preservation from resource_store.rs; use add_with instead
Move crJSON-schema.json from cli/schemas/ to sdk/tests/fixtures/schemas/ and update the reference
Add CLI integration test for --crjson flag output
Add check-format, check-docs, and clippy as prerequisites to test-sdk Makefile target
Minor code formatting fixes (rustfmt-style rewraps across crjson.rs, hash_assertions.rs, schema_compliance.rs)
Copy link
Collaborator

@scouten-adobe scouten-adobe left a comment

Choose a reason for hiding this comment

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

No blockers, but a couple of things to please consider.

@gpeacock gpeacock merged commit 9a48a33 into main Mar 23, 2026
28 checks passed
@gpeacock gpeacock deleted the gpeacock/cr_json_tests branch March 23, 2026 16:59
This was referenced Mar 23, 2026
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.

4 participants