Add a method to retrieve an entity by upstream ID with a hint #4519
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
JAORMX
reviewed
Sep 18, 2024
jhrozek
force-pushed
the
entity_by_upstream_with_hint
branch
from
f70e4db
to
1e1f715
Compare
We're about to add a different `EntityWithPropertiesByUpstreamID` so let's make the names more descriptive.
JAORMX
reviewed
Sep 19, 2024
|
|
||
| l.Debug().Str("lookupProp", loopupProp).Msg("fetching by property") | ||
| ents, err = getAllByProperty(ctx, loopupProp, prop.RawValue(), entType, hint.projectID, hint.providerID, qtx) | ||
| if errors.Is(err, ErrEntityNotFound) { |
There was a problem hiding this comment.
will we ever get this? wouldn´t we just get an empty list instead?
There was a problem hiding this comment.
thanks, I moved the logic around so it hopefully reads better
When processing a webhook payload, we typically only have the information from the webhook payload available which is to say information about the upstream entity. We even don't know exactly which provider and which project are we handling. We used to sort of wing it in the github webhook handler and were just searching by upstream ID as if it was globally unique, but that no longer works with the introduction of multiple providers where several entities might have the same upstream ID or the same name, just in different providers. To handle that, we add a new method to the properties service that searches by upstream properties with a hint. At the moment, the interface supports providerID, projectID and provider implements as a hint. The immediate use is that the github provider will search by the upstream ID with the hint set to ProviderTypeGithub as that's implemented both by the app and OAuth github providers. If any attribute of the hint is set, then the entity's attribute must match the hint, otherwise the entity is filtered out. Additionally, once the entity is found, the hint might contain a property with a value that must match. The use-case there would be deleting entities where we want to ensure that we are deleting an entity with an appropriate upstream hook ID. If no entities match the hints or if multiple entities match a hint, an error is returned. Related: mindersec#4327
jhrozek
force-pushed
the
entity_by_upstream_with_hint
branch
from
1e1f715
to
c088808
Compare
…callers convert it to ErrEntityNotFound if needed
JAORMX
approved these changes
Sep 19, 2024
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.
Summary
When processing a webhook payload, we typically only have the
information from the webhook payload available which is to say
information about the upstream entity. We even don't know exactly which
provider and which project are we handling.
We used to sort of wing it in the github webhook handler and were just
searching by upstream ID as if it was globally unique, but that no
longer works with the introduction of multiple providers where several
entities might have the same upstream ID, just in different providers.
To handle that, we add a new method to the properties service that
searches by upstream ID with a hint. At the moment, the interface
supports providerID, projectID and provider implements as a hint.
The immediate use is that the github provider will search by the
upstream ID with the hint set to ProviderTypeGithub as that's
implemented both by the app and OAuth github providers.
If any attribute of the hint is set, then the entity's attribute must
match the hint, otherwise the entity is filtered out. Additionally, once
the entity is found, the hint might contain a property with a value that
must match. The use-case there would be deleting entities where we want
to ensure that we are deleting an entity with an appropriate upstream
hook ID.
If no entities match the hints or if multiple entities match a hint, an
error is returned.
Related: #4327
Change Type
Testing
as part of a larger branch + unit tests
Review Checklist: