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: add id and type to match interface #164

Conversation

TimoGlastra
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Introduces a new id and type property on the SubmissionRequirementMatch interface that allows better matching of a match object to the related input_descriptor or submission_requirement.

If the match is for an input descriptor (in case there are no submission_requirements) the type property will be SubmissionRequirementMatchType.InputDescriptor and the id property will be the id of the input descriptor

I the match is for a submission requirement, the type property will be SubmissionRequirementMatchType.SubmissionRequirement and the id property will be the index of the submission requirement in the submission_requirements array (this is the only unique thing I could find to link back to the submission requirement entry, there is no required id field or something). This also works for nested submission requirements. In this case the outer match id is the outer index in the submission requirement, and the inner match id is the index within the from_nested array (this can recurse endlessly).

Although adding these properties is not a breaking change, I did change the behaviour of the name property as well. Currently this was set to the name or id of one of the input descriptors, even if the match is for a submission requirement, which leads to confusion and doesn't make sense IMO. So if the match is for a submission requirement (for input descriptor behaviour stays the same) I now updated that to be either the name of the submission requirement, or undefined if there submission_requirement has no name. You can use the id property to locate the submission requirement associated with the match.

One thing we lose here (but basically wasn't present before as well) is that you now don't know which input descriptor(s) the credentials in a submission requirement match are from. So if a group "E" can either accept a cred from input descriptor "A" or "B" you now just know that the credentials in the list match group "E", not which input descriptor they came from

I see this as a great additional feature for a follow PR (it will be non-breaking extra metadata, but should somehow be linked to the vc_paths, as there can be vcs from different input descriptors in the match list)

This fixes #116 as well as #161 (on the PEX level, there will be changes required in Credo as well)

The changes in tests mostly come from now also expecting the new properties, the change in code are quite minimal

});
}
}
}
return this.removeDuplicateSubmissionRequirementMatches(submissionRequirementMatches);
}

private mapMatchingDescriptors(
private getMatchingVcPatchsForSubmissionRequirement(
Copy link

Choose a reason for hiding this comment

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

Suggested change
private getMatchingVcPatchsForSubmissionRequirement(
private getMatchingVcPathsForSubmissionRequirement(

Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra
Copy link
Contributor Author

Hey @nklomp any chance you can take a look at this PR? :)

@nklomp
Copy link
Contributor

nklomp commented Sep 13, 2024

As I am on holiday, will defer to @sksadjad who should be back after the weekend, or @sanderPostma

@TimoGlastra
Copy link
Contributor Author

Thanks!

@sanderPostma
Copy link
Contributor

@TimoGlastra your PRs are up next on my to do list, I expect to start after lunch,

Copy link
Contributor

@sksadjad sksadjad left a comment

Choose a reason for hiding this comment

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

looks good to me. added a few typo fixes. the part about the name of the submission_requirement.index vs input_descriptor.id I think it's a good addition. but for the whole input_descriptor.name vs input_descriptor.id I think we should drop the name and just return the id. but that is a discussion for another time.

lib/evaluation/core/submissionRequirementMatch.ts Outdated Show resolved Hide resolved
lib/evaluation/core/submissionRequirementMatch.ts Outdated Show resolved Hide resolved
lib/evaluation/core/submissionRequirementMatch.ts Outdated Show resolved Hide resolved
@TimoGlastra
Copy link
Contributor Author

input_descriptor.name vs input_descriptor.id I think we should drop the name and just return the id

Yeah i think that makes sense. Didn't want to remove anything for now

@sanderPostma
Copy link
Contributor

sanderPostma commented Sep 17, 2024

@TimoGlastra
The CI pipeline is not passing, something with codecov. Perhaps because this PR is coming from a fork.
Can you please rebase against https://github.com/Sphereon-Opensource/PEX/tree/fix/bugs-selection ?
Will merge it in there and when CI passes I will merge it into develop

@TimoGlastra TimoGlastra changed the base branch from develop to fix/bugs-selection September 17, 2024 15:15
@TimoGlastra
Copy link
Contributor Author

It's now pointed at fix/bugs-selection!

@sanderPostma sanderPostma merged commit b8eb1d8 into Sphereon-Opensource:fix/bugs-selection Sep 17, 2024
1 check failed
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.

Difficult/impossible to match matches against the input_descriptors that satisfied them
5 participants