elastic / connectors

Source code for all Elastic connectors, developed by the Search team at Elastic, and home of our Python connector development framework
https://www.elastic.co/guide/en/enterprise-search/master/index.html
Other
59 stars 118 forks source link

Improve dependency handling for configurable fields #1445

Open artem-shelkovnikov opened 10 months ago

artem-shelkovnikov commented 10 months ago

Problem Description

Some fields can depend on other fields. For example, consider that there's a setting "Fetch Permissions". If it's on, there's another setting "Fetch granular file permission".

So the code that checks that granular file permissions should be enabled will look like this:

if self.configuration["fetch_permissions"] and self.configuration["fetch_granular_file_permissions"]:
    # do something

If this nesting becomes deeper (2 levels+) then the code becomes unmanagable.

Proposed Solution

Needs research:

Additional Context

See: https://elastic.slack.com/archives/C01795T48LQ/p1686734794049519?thread_ts=1686734378.694409&cid=C01795T48LQ

sphilipse commented 10 months ago

One option would be to check just fetch_granular_file_permissions, but have 'checking a value' also mean checking the specified dependencies (the depends_on) field. That is, instead of checking self.configuration["fetch_granular_file_permissions"] we'd have a method like, say check_config_value that takes a configuration object, a field to check, and the value to check against--and it would then recursively apply itself to any dependencies listed in depends_on.

So you'd get something like:

if self.check_config_field(self.configuration["fetch_granular_file_permissions"], True):
  do something

def check_config_field(self, config_field, value)
  if self.dependencies_satisfied(config_field) && config_field.value == value
    return True
  else
    return False
artem-shelkovnikov commented 10 months ago

@sphilipse does our UI allow for deep nesting (3-4-5 levels)?

sphilipse commented 10 months ago

I'd have to test that out, I don't think that's particularly relevant for this issue?

artem-shelkovnikov commented 10 months ago

It's just good to know the limitations/expectations for addressing it.

I'm not sure how "Depends on" work fully - I think it only allows for boolean logic, or not?

I don't particularly like self.dependencies_satisfied(config_field) cause it's same problem we have now - need to remember to actually check dependencies semi-manually. If our nesting is only 2 levels, adding this method does not reduce complexity much, for example.

IMO that's just a good topic to talk and set clear expecations on

sphilipse commented 10 months ago

We could modify dependencies_satisfied to just recursively check the config fields it's checking against, that would solve the problem generally, no?

sphilipse commented 10 months ago

depends_on is just a list of key/value pairs, with each key being the id of a config field, and the value being the necessary value. If all listed key/value pairs match the key/values in the config, the dependency is satisfied. Otherwise it's not. It's intentionally very simple logic, and "nesting" doesn't really enter into it.