Feat(ingest/ldap)fix list index out of range error #8525
Conversation
github-actions
bot
added
the
ingestion
…x_list_index_out_of_range_error
metadata-ingestion/tests/integration/ldap/ldap_mces_golden.json
Outdated
Show resolved
Hide resolved
| description="Use pagination while search for managers (enabled by default).", | ||
| description="[deprecated] Use pagination_enabled ", | ||
| ) | ||
| _deprecate_manager_pagination_enabled = pydantic_field_deprecated( |
There was a problem hiding this comment.
Why is this required ? There is also manager_filter_enabled. Does it also need renaming ?
Suggestion : Use pydantic_renamed_field instead. No need to write your own validator in that case.
There was a problem hiding this comment.
Why is this required ?
LDAP service like database, we can get error "SIZELIMIT_EXCEEDED: {'desc': 'Size limit exceeded'}"
This error tells us that we reached the limit of search result entries (1000 by default). There are 2 ways of resolving this error:
- Go to your LDAP DATABASE and increase the maximum page size (MaxPageSize) option.
- Use result paginating in your Python code.
Also, LDAP DATABASE sometimes doesn't support pagination at all. For example, google workspace LDAP doesn't support it -> so we need the option to turn off pagination completely, or we get an error.
Sometimes finding a person manager provides unexpected data and can be reduced with pagination.
There is also manager_filter_enabled. Does it also need renaming?
No, this option is responsible for additional manager discovery. We want that person's head will be founded or not.
Suggestion : Use pydantic_renamed_field instead. No need to write your own validator in that case.
Thx, I change it.
There was a problem hiding this comment.
Right. My question was around - why this rename is required - manager_pagination_enabled->pagination_enabled. Does this rename also warrant a rename in manager_filter_enabled->filter_enabled ?
There was a problem hiding this comment.
In the old code/way, we always setup page control with line self.lc = create_controls(self.config.page_size).
Sometimes LDAP provider doesn't support pagination. That's why i need an option – enable pagination or not in new code.
Now I have a decision:
- create a new variable, for example
pagination_enabledandsave manager_pagination_enabled - rename
manager_pagination_enabledtopagination_enabled
I choose option 2) because I don't see a situation where we need pagination for manager discovery, and at the same time, we need a global user/group pagination. If we have a problem and pagination is needed for the user/group or head, we can enable or disable it globally. The user will need clarification, as I am, with the name manager_pagination_enabled and global enable/disable pagination.
| self.report.report_failure( | ||
| "ldap-control", "Server ignores RFC 2696 control." | ||
| ) | ||
| if serverctrls: |
There was a problem hiding this comment.
Should the source report failure if serverctrls is not truthy ? When is serverctls not truthy ?
There was a problem hiding this comment.
Should the source report failure if
serverctrlsis not truth ?
No, only if enabled pagecontrol. i fix it.
When is serverctls not truth?
In some scenarios, clients might want to check if the server supports specific controls without actually using them. To do this, they can send an empty server control in the request and observe the server's response. If the server supports the control, it will be included in the response.
Changed on self.lc
| # - attribute exists | ||
| # - attribute is not empty list | ||
| if attrs.get(self.config.user_attrs_map["email"]): | ||
| email = (attrs[self.config.user_attrs_map["email"]][0]).decode() |
There was a problem hiding this comment.
can this logic be extracted into a method and use the method - say - get_attr_or_none everywhere. looks like a lot of repetition here..
def get_attr_or_none(attrs, key, default=None):
return attrs[self.config.user_attrs_map[key]][0].decode() if attrs.get(self.config.user_attrs_map[key]) else default
also is it assumed that attrs[self.config.user_attrs_map[key]] is instance of list ? Is is always true ? May help to add a check here to avoid errors.
| else None | ||
| ) | ||
|
|
||
| if attrs.get(self.config.group_attrs_map["email"]): |
There was a problem hiding this comment.
same comment as above
…x_list_index_out_of_range_error
Co-authored-by: Mayuri Nehate <33225191+mayurinehate@users.noreply.github.com>
…out_of_range_error' into feat(ingest/ldap)fix_list_index_out_of_range_error
mayurinehate
added
the
community-contribution
Co-authored-by: Mayuri Nehate <33225191+mayurinehate@users.noreply.github.com>
issue #8523
Checklist
Provided changes:
– Some fields may not be filled in, now there will be a check via
dict.get("key")– In some cases, the LDAP provider does not have pagination functionality, and the ingest fails, although all data was loaded successfully. Now possible to use the
pagination_enabledoption to work with LDAP providers.– In LDAP ingestor, the
manager_pagination_enabledchanged to generalpagination_enabled. The old optionmanager_pagination_enablednow provides a warning massage.