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

feat!: Return actionable errors from Manifest validation #552

Open
wants to merge 1 commit into
base: feat/html-extractors#536
Choose a base branch
from

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Mar 7, 2025

Based on #547 - Please review it first

Related Issues

Changes

  • Introduced ManifestValidationResult to aggregate validation errors and extracted values
  • Collect multiple errors instead of failing fast

Required Reviews

Minimum number of reviews before merge: 2

@piotr-roslaniec piotr-roslaniec force-pushed the feat/error-handling#511 branch 4 times, most recently from 03eccc4 to 43fa48b Compare March 7, 2025 16:47
@piotr-roslaniec piotr-roslaniec changed the base branch from main to feat/html-extractors#536 March 7, 2025 16:52
@piotr-roslaniec piotr-roslaniec force-pushed the feat/error-handling#511 branch from 43fa48b to 6fb4745 Compare March 7, 2025 16:58
Comment on lines +499 to +509
let mut expected = vec![
"Invalid manifest: Invalid HTTP method",
"Invalid manifest: Unsupported HTTP status",
"Invalid manifest: Token `<% MISSING_TOKEN %>` not declared in `vars`",
"Manifest HTTP error: HTTP header mismatch: expected Bearer <% MISSING_TOKEN %>, actual Bearer invalid-token",
"Manifest HTTP error: HTTP header mismatch: expected invalid/type, actual application/json",
"Template error: Variable missing for key: UNUSED_TOKEN",
"Manifest HTTP error: HTTP method mismatch: expected POST, actual INVALID_METHOD",
"Manifest HTTP error: HTTP URL mismatch: expected https://api.example.com, actual ftp://invalid-scheme.com",
"Extraction failed: No data to match against"
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the kind of feedback the end-user can expect to receive from the notary.

cc: @drewjenkins

@piotr-roslaniec piotr-roslaniec force-pushed the feat/error-handling#511 branch from 6fb4745 to d50f079 Compare March 7, 2025 18:13
@piotr-roslaniec piotr-roslaniec marked this pull request as ready for review March 7, 2025 18:13
@mattes
Copy link
Contributor

mattes commented Mar 7, 2025

The failing tests can be ignored, because they don't exist in main anymore.

@Autoparallel
Copy link
Contributor

@piotr-roslaniec #556 may be worth considering alongside this PR and with the rebase that will happen. What do you think?

@piotr-roslaniec
Copy link
Contributor Author

@Autoparallel I can address this in the next PR

@Autoparallel
Copy link
Contributor

@piotr-roslaniec sounds good, sir!

@piotr-roslaniec piotr-roslaniec force-pushed the feat/error-handling#511 branch from 57741d9 to e6399b1 Compare March 10, 2025 08:56
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.

feat: Error Codes/Messaging
3 participants