-
Notifications
You must be signed in to change notification settings - Fork 15
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
Open
TimoGlastra
wants to merge
3
commits into
Sphereon-Opensource:develop
Choose a base branch
from
TimoGlastra:fix/bugs-selection
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
feat: add id and type to match interface #164
TimoGlastra
wants to merge
3
commits into
Sphereon-Opensource:develop
from
TimoGlastra:fix/bugs-selection
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
DJHunn39
reviewed
Sep 5, 2024
}); | ||
} | ||
} | ||
} | ||
return this.removeDuplicateSubmissionRequirementMatches(submissionRequirementMatches); | ||
} | ||
|
||
private mapMatchingDescriptors( | ||
private getMatchingVcPatchsForSubmissionRequirement( |
There was a problem hiding this comment.
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 <timo@animo.id>
Hey @nklomp any chance you can take a look at this PR? :) |
As I am on holiday, will defer to @sksadjad who should be back after the weekend, or @sanderPostma |
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces a new
id
andtype
property on theSubmissionRequirementMatch
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 beSubmissionRequirementMatchType.InputDescriptor
and theid
property will be the id of the input descriptorI the match is for a submission requirement, the
type
property will beSubmissionRequirementMatchType.SubmissionRequirement
and theid
property will be the index of the submission requirement in thesubmission_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 matchid
is the outer index in the submission requirement, and the inner matchid
is the index within thefrom_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 theid
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