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

Feature Suggestion: Include a Reason When Constraints Cannot Be Applied #2630

Conversation

Ennovate-com
Copy link
Contributor

@Ennovate-com Ennovate-com commented Nov 25, 2023

When the verifier agent is unable to apply a constraint filter, it would be helpful for the Problem Report to include a reason why it failed. For example, instead of:

Constraint specified for input_descriptor_123 does not apply to the enclosed credential in $.verifiableCredential[0]

... the message could include the reason that the constraint could not be applied, with the specific path or item that did not match. For example:

Constraint specified for input_descriptor_123 does not apply to the enclosed credential in $.verifiableCredential[0]
Reason: Credential is not applicable for field 11111111-2222-3333-4444-56789abcdef0 with paths ['$.credentialSubject.part1.part2']

This indicates that the field indicated in the message was not found in the Reveal Document or the full Credential.

or

Constraint specified for input_descriptor_123 does not apply to the enclosed credential in $.verifiableCredential[0]
Reason: No field in constraints for part2 under parent path "$.credentialSubject.part1"

This indicates that the "part2" property found in the Reveal Document under "$.credentialSubject.part1" was not found in the list of fields in the constraints.

Robert Simpson added 7 commits November 25, 2023 06:19
@dbluhm
Copy link
Contributor

dbluhm commented Nov 28, 2023

I like this suggestion; however, the implementation is a little less "Pythonic" than it ought to be. First, the nitpicky comment: variables should be snake_case (failReason is what I'm referring to specifically). And second, I think the fail reasons should not be reported as a string from the constraint application method. The semantics are a bit too loose, IMO. I would recommend reporting the fail reason in an exception that is caught by the caller.

Robert Simpson added 3 commits November 29, 2023 03:31
Signed-off-by: Robert Simpson <[email protected]>
Signed-off-by: Robert Simpson <[email protected]>
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and for the prompt turn around on feedback!

@dbluhm dbluhm merged commit 634adc3 into openwallet-foundation:main Nov 29, 2023
9 checks passed
@Ennovate-com
Copy link
Contributor Author

@dbluhm And, likewise, thanks for the quick feedback and merge. Much appreciated.

@Ennovate-com Ennovate-com deleted the feature/apply-constraint-fail-reason branch December 25, 2023 18:28
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.

2 participants