Codecov Report

Merging #272 (084b622) into master (39ea8e5) will increase coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
+ Coverage   71.30%   71.72%   +0.41%     
==========================================
  Files          50       49       -1     
  Lines        4796     5033     +237     
  Branches      802      894      +92     
==========================================
+ Hits         3420     3610     +190     
- Misses       1045     1067      +22     
- Partials      331      356      +25     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2647f59...42b87b0. Read the comment docs.

The tests fail only due to an upstream error in spinx when building the documentation:

Traceback (most recent call last):
  File "/home/travis/build/python-ldap/python-ldap/.tox/doc/lib/python3.6/site-packages/sphinx/cmd/build.py", line 283, in build_main
    args.tags, args.verbosity, args.jobs, args.keep_going)
  File "/home/travis/build/python-ldap/python-ldap/.tox/doc/lib/python3.6/site-packages/sphinx/application.py", line 228, in __init__
    self.setup_extension(extension)
  File "/home/travis/build/python-ldap/python-ldap/.tox/doc/lib/python3.6/site-packages/sphinx/application.py", line 380, in setup_extension
    self.registry.load_extension(self, extname)
  File "/home/travis/build/python-ldap/python-ldap/.tox/doc/lib/python3.6/site-packages/sphinx/registry.py", line 484, in load_extension
    metadata = mod.setup(app)
  File "/home/travis/build/python-ldap/python-ldap/.tox/doc/lib/python3.6/site-packages/sphinxcontrib/spelling/__init__.py", line 11, in setup
    app.info('Initializing Spelling Checker')
AttributeError: 'Sphinx' object has no attribute 'info'

@tiran
On my notebook, no LDAP connection is done with this and not required. It works even without any network connection. And it is very fast.

Would it create a ldap connection if I had a /etc/openldap/ldap.conf? Then I will see if I can find a way to prevent this.

If there is no connection involved, why catch SERVER_DOWN?
This logic looks fragile: if the server is down, all filters are considered valid.

If the ldap object is uninitialized and unconnected it raises ldap.SERVER_DOWN without any connection attempt.
Using strace I cannot see any connection attempt:
strace python -c 'import ldap; lo = ldap.initialize(""); lo.search_ext_s("", ldap.SCOPE_BASE, "foo=bar")' 2>&1 | less

What I can see is that it reads my "/etc/ldap/ldap.conf" and tries to read ~/.ldaprc.
My ldap.conf contains URI and BASE. Do I need HOST so that it would actually try to connect even if I pass "" as URI?

The main problem is that, however you connect, on SERVER_DOWN, all filters would be considered valid – the function would return a wrong result with no indication of failure.

I don't want to merge code that can silently hide a failure, so I'll reject this approach. It might be suitable for a library that builds on top of python-ldap, rather than for python-ldap itself.
Please re-open if you want to discuss further.

There is no REOPEN button.
As far as I inspected this, your assumptions that there will be a ldap connection established, are not correct. Can you proove me wrong, please, and explain what I need to do so that this behavior would occurr?

If you set LDAPNOINIT the client libraries will bypass reading ldap.conf/.ldaprc. This is what the OpenLDAP test suite does to ensure that local client configurations don't interfere.

In my opinion the patch is not a clean solution but a hack. I don't like to add such hacks to python-ldap. You can easily ship the short function in your application.

I wouldn't mind to add this feature iff OpenLDAP adds a public API to perform the check.

There is no REOPEN button.

Apologies, I had wrong assumptions of the GitHub UI.

My main objection is that is_filter returns different results based on whether the server is reachable or not.
If that is not the case, why does the code handle ldap.SERVER_DOWN?

I wouldn't mind to add this feature iff OpenLDAP adds a public API to perform the check.

Yes, that would be the best. You already mentioned the not exposed function ldap_pvt_put_filter.

My main objection is that is_filter returns different results based on whether the server is reachable or not.
If that is not the case, why does the code handle ldap.SERVER_DOWN?

The ldap connection there is not connected and will never be. At least I could not get any connection attempt with that code. ldap.SERVER_DOWN therefore does only indicate that the filter is valid, no matter if there is a server which can be connected to or not. The filter validity is checked before any connection attempts, it there are any at all.

If that code really does a connection, how would I set this up to reproduce this? We are using the above code since years and it works. We have a /etc/openldap/ldap.conf and a /etc/ldap/ldap.conf with valid entries. I don't see where it's evaluated. and we did not set LDAPNOINIT.

I took the name is_filter() for consistency, because there is also a is_dn() already.

I see. I misunderstood the code.

Still, this does rely on undocumented behavior, so @tiran's comment holds:

In my opinion the patch is not a clean solution but a hack. I don't like to add such hacks to python-ldap. You can easily ship the short function in your application.

I wouldn't mind to add this feature iff OpenLDAP adds a public API to perform the check.

IOW, if this is the easiest way to check filter validity using OpenLDAP, it's an issue in OpenLDAP. python-ldap is the wrong place for a workaround.

Did you raise the issue with OpenLDAP upstream to request a filter validation function? I'm hesitant to use an internal, private function ldap_pvt_put_filter to validate filters.

Sorry, I did not see your latest answer. I created a feature request at openldap: https://bugs.openldap.org/show_bug.cgi?id=9393
Let's see what they are saying.

I also updated the pull request, made it more clear that ldap.SERVER_DOWN is expected to be raised and raise a RuntimeError (which can't happen) in all other cases. Maybe you are fine with this implementation and could provide it as fallback until there is something available in Open-LDAP? The function has detailed tests.

They aren't replying at all.

They aren't replying at all.

What you filed is a feature request. The earliest it would be done is OpenLDAP 2.7.