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

SALTO-6715: Return early from type fetcher on depends on context #7010

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shir-reifenberg
Copy link
Contributor

@shir-reifenberg shir-reifenberg commented Dec 28, 2024

This fixes a bug in dependsOn, when the previous request returned no results


Additional context for reviewer
If type A dependsOn type B, and the request of type B returned no results (either cause it's failed or empty in the service), we would have still make the request to type A, although there is not point as the will be missing context.

I added another check to catch this scenario in advance, the alternative (as today) is throwing an error when calculating the request args (in contextPossibleArgs)

This was causing noisy logs of "failure" though it's not really a failure

Didn't add tests as the entire file doesn't have any :(


Release Notes:
None


User Notifications:
None

@Nurdok
Copy link
Contributor

Nurdok commented Dec 30, 2024

Can you share the failure logs you found and the scenario?

I'm a little worried that this can cause issues where there's some legitimate reason the context is empty (perhaps it's used in a condition, and it can be empty sometimes), or even misconfiguration by an adapter developer where they specified dependsOn but don't always use it - I think a confusing error log might be better than not sending a request where we need to. In any case, an error seems appropriate if the resource depends on a context arg that isn't found - aren't you just hiding an actual error here?

@shir-reifenberg
Copy link
Contributor Author

Can you share the failure logs you found and the scenario?

I'm a little worried that this can cause issues where there's some legitimate reason the context is empty (perhaps it's used in a condition, and it can be empty sometimes), or even misconfiguration by an adapter developer where they specified dependsOn but don't always use it - I think a confusing error log might be better than not sending a request where we need to. In any case, an error seems appropriate if the resource depends on a context arg that isn't found - aren't you just hiding an actual error here?

The error log -
error adapter-components/fetch/element/element adapterName="jamf" typeName="mac_application" unexpectedly failed to fetch type jamf:mac_application: value /JSSResource/macapplications/id/{id} still contains unresolved args: id

This can happen in two cases -

  1. when the "parent" request (in this case /JSSResource/macapplications returned no result)
  2. when the "parent request failed - if this is the case, we'll also have a similar log for unexpectedly failed to fetch type jamf:parent_type, so for your question, we're not hiding the real error.

re legitimate empty context, seems like the code is is throwing on unresolved arg anyway, I agree in case of condition it might be overlooked, but I think there are other ways to implement it?

The motivation for this change is to allow us to monitor types failure, and this log is currently "noisy"

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