On 11/19/2014 02:03 PM, Jan Cholasta wrote: > Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): >> >> On 11/19/2014 12:51 PM, Martin Kosek wrote: >>> On 11/19/2014 12:41 PM, Tomas Babej wrote: >>>> On 11/19/2014 12:24 PM, Martin Kosek wrote: >>>>> On 11/19/2014 12:03 PM, Tomas Babej wrote: >>>>>> Hi, >>>>>> >>>>>> When constructing a parent DN in LDAPSearch, we should always >>>>>> check that the parent object exists (hence use get_dn_if_exists), >>>>>> rather than search on unexistant containers (which can happen >>>>>> with get_dn). >>>>>> >>>>>> Replaces get_dn calls with get_dn_if_exists in *-find commands >>>>>> and makes sure proper error message is raised. >>>>>> >>>>>> https://fedorahosted.org/freeipa/ticket/4659 >>>>> Doesn't it produce extra LDAP search thus making all our search >>>>> commands >>>>> slower? Is that what we want? >>>> No it does not make all of our LDAP search slower. It only happens for >>>> the objects that have parent objects, such as idoverrides or >>>> dnsrecords. >>> ... and makes them slower. >> >> What I was pointing out here is that this is not a issue for ALL *-find >> commands (e.g user-find, group-find will not suffer from it), as you >> incorrectly stated. >> >>> >>>>> Wouldn't it be better to distinguish between LDAP >>>>> search with no results and LDAP search with missing parent DN? The >>>>> reply looks >>>>> different, at least in CLI: >>>> Up to discussion. We would probably need to introduce a new exception, >>>> like ParentObjectNotFound. >>>> >>>>> # search result >>>>> search: 4 >>>>> result: 0 Success >>>>> >>>>> # search result >>>>> search: 4 >>>>> result: 32 No such object >>>>> matchedDN: cn=accounts,dc=mkosek-f20,dc=test >>>>> >>>>> Also, I do not think you can just stop using get_dn(), some >>>>> commands override >>>>> this call to get more complex searches (like host-find searching >>>>> for shortname). >>>> Look into the get_dn_if_exists, it just wraps around get_dn, so no >>>> issue >>>> here. Any custom behaviour is preserved. >>> Ah, ok, thanks for info. >>> >>>> To sum up, I think this is worth changing this behaviour by default, >>>> ignoring a non-matching value of the parent object is not a correct >>>> general approach in my opinion. >>> Well, that's the question. Whether we would leave DS to validate the >>> search >>> itself or do all the pre-check ourselves. To me, doing just one LDAP >>> search and >>> processing the error correctly looks better. But I can live even >>> with your >>> version then, I will leave the framework guardians like Honza or >>> Petr3 to decide. > > +1 on single LDAP search and proper error processing. > >> >> I see now what you're trying to suggest. However, the reason boils >> down to ipaldap.find_entries method not differentiating between a >> LDAP search that returns error code 32 (No such object) and LDAP >> search returning error code 0 (Success), but returning no results. >> >> In both cases errors.NotFound is raised. >> >> The reason I did not go this way is that changing the find_entries >> method >> is quite more invasive as this is the method subsenqently called by >> almost >> any command. > > You can always derive the new error (ParentNotFound or whatever) on > NotFound, so old code won't break. >
Thanks for the suggestsions. Attached is a new patch which hooks into find_entries method and differentiates between the cases. -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org
>From 2de4f0020a2a09b6328018652a49d8e4bf6c2c20 Mon Sep 17 00:00:00 2001 From: Tomas Babej <tba...@redhat.com> Date: Wed, 19 Nov 2014 12:00:07 +0100 Subject: [PATCH] baseldap: Handle missing parent objects properly in *-find commands The find_entries function in ipaldap does not differentiate between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. In turn, LDAPSearch commands interpret NotFound exception as no results. To differentiate between the cases, a new error ContainerNotFound was added, which inherits from NotFound to preserve the compatibility with the new code. This error is raised by ipaldap.find_entries in case it is performing a search with onelevel or subtree scope, and the target dn does not exist. https://fedorahosted.org/freeipa/ticket/4659 --- ipalib/errors.py | 16 ++++++++++++++++ ipalib/plugins/baseldap.py | 2 ++ ipapython/ipaldap.py | 12 +++++++++--- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/ipalib/errors.py b/ipalib/errors.py index f0426583dae2982b661d8c6d1540fe4acea83cef..7436b5e4bce2c024ab7b686a94b6aa124ebb2840 100644 --- a/ipalib/errors.py +++ b/ipalib/errors.py @@ -1329,6 +1329,22 @@ class PosixGroupViolation(ExecutionError): errno = 4030 format = _('This is already a posix group and cannot be converted to external one') +class ContainerNotFound(NotFound): + """ + **4031** Raised when a container upon which a subtree or one-level search + is performed is not found. + + For example: + + >>> raise ContainerNotFound(reason='no such entry') + Traceback (most recent call last): + ... + ContainerNotFound: no such entry + + """ + + errno = 4031 + class BuiltinError(ExecutionError): """ **4100** Base class for builtin execution errors (*4100 - 4199*). diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 375441c0fd55efe70d5a6f5bed6700e618874082..67d0a08aed813f87798a657d6b3ab4a9497977ed 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -1995,6 +1995,8 @@ class LDAPSearch(BaseLDAPCommand, crud.Search): time_limit=options.get('timelimit', None), size_limit=options.get('sizelimit', None) ) + except errors.ContainerNotFound: + self.api.Object[self.obj.parent_object].handle_not_found(*args[:-1]) except errors.NotFound: (entries, truncated) = ([], False) diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py index 1702daa253d7eb568c27f66fda1810b4661656ad..4acd290e337b6544e85cca03226da8ae1c926c9c 100644 --- a/ipapython/ipaldap.py +++ b/ipapython/ipaldap.py @@ -1149,7 +1149,7 @@ class LDAPClient(object): self.conn = None @contextlib.contextmanager - def error_handler(self, arg_desc=None): + def error_handler(self, arg_desc=None, container_search=False): """Context manager that handles LDAPErrors """ try: @@ -1164,7 +1164,11 @@ class LDAPClient(object): info = "%s arguments: %s" % (info, arg_desc) raise except ldap.NO_SUCH_OBJECT: - raise errors.NotFound(reason=arg_desc or 'no such entry') + error_message = arg_desc or 'no such entry' + if container_search: + raise errors.ContainerNotFound(reason=error_message) + else: + raise errors.NotFound(reason=error_message) except ldap.ALREADY_EXISTS: raise errors.DuplicateEntry() except ldap.CONSTRAINT_VIOLATION: @@ -1472,8 +1476,10 @@ class LDAPClient(object): if page_size == 0: paged_search = False + search_in_container = scope in (ldap.SCOPE_SUBTREE, ldap.SCOPE_ONELEVEL) + # pass arguments to python-ldap - with self.error_handler(): + with self.error_handler(container_search=search_in_container): while True: if paged_search: sctrls = [SimplePagedResultsControl(0, page_size, cookie)] -- 1.9.3
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
