Raw search usage.

no one liners

What is the point of this restart?

This is ... not right at all. You should NEVER use useraccount on a NOT user account type! There is a inst.config.set_() series of functions.
You don't need this helper at all.

This list is too complex and hard to follow.

Hey there,

I have given lots of feedback on other test cases you have provided, but it seems you may have missed it. I provide feedback not just on a patch, but to help educate you, to be a better programmer so that every time you submit a patch it improves and lifts the quality of your work and what we accept. I'm seeing some patterns in here that are things I have already commented on in past patches. I'd really like you to do a self review of your work, keeping in mind the many comments I have provided you already. Additionally, I'd really like to see you ask questions to the 389-devel list when you are stuck, rather than inventing solutions. An example is you could have asked what is the right way to set a value in cn=config, becauuse it is very little effort for simon or I to answer, and would save you the time of patch + review etc.

So please review your work with all my comments in mind thanks.

I am not sure to understand the need of this setting+restart
If the search is done by DM, sizelimit is not enforced and any value should be fine.
If it is not DM, then the number of returned entry is generally low (e.g. 6) so why this setting to 52000 ? Do you need to set lookthroughlimit rather that size ?

Is this testcase really pass ?
IMHO some should succeed (e.g. 4000, 'limit=0 type=sub', 'type=eq') but most of the others setting should trigger something like unwilling to perform

Raw search usage.

What is the recommended way to search with a specific filter on arbitrary LDAP entry?

Is this testcase really pass ?
IMHO some should succeed (e.g. 4000, 'limit=0 type=sub', 'type=eq') but most of the others setting should trigger something like unwilling to perform

This test case passes, but it shouldn't. All these values are accepted, only errors are logged. It should really reject incorrect values.

This list is too complex and hard to follow.

This remark is not helpful, please suggest a better way.

Surprisingly I was able to apply successfully all those settings. But some of them should fail (they are invalid).
Also I applied those setting on an index entry, but the code seems to apply it on a userAccount that make no real sense

Surprisingly I was able to apply successfully all those settings. But some of them should fail (they are invalid).
Also I applied those setting on an index entry, but the code seems to apply it on a userAccount that make no real sense

def test_error_handling_for_invalid_configuration(topo):
""""
Error handling for invalid configuration
Starting...test cases for bug1011539
Index config error handling does not exist - you can add any old thing
:id:377950f6-9f06-11e8-831b-8c16451d917b
:setup: Standalone instance
:steps:
1. Create Instance
2. Try change nsIndexIDListScanLimit
:expectedresults:
1. This should pass
2. This should pass
"""
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'4000')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 flags=bogus')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 limit=1')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=eq type=eq')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=sub type=eq')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 flags=AND flags=AND')])
assert topo.standalone.modify_s(GIVEN_NAME, [
(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=eq values=foo values=foo')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=eq values=foo,foo')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=sub')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=eq values=notvalid')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit')])
assert topo.standalone.modify_s(GIVEN_NAME, [
(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=eq values=notavaliddn')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=pres values=bogus')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=eq,sub values=bogus')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=1 type=')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=1 flags=')])
assert topo.standalone.modify_s(GIVEN_NAME,
[(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=1 type=eq values=')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=-2')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'type=eq')])
assert topo.standalone.modify_s(GIVEN_NAME, [(ldap.MOD_REPLACE, 'nsIndexIDListScanLimit', b'limit=0 type=bogus')])

This was the actual test case , (This one is also passed ) as raw modify_s is not allowed , i have used userAccount . With topo.standalone.modify_s it takes all the invalid values

https://pagure.io/389-ds-base/issue/50201 to track the successful update of invalid values

Changes are made , Please check

Please, check PEP8 for the function names
https://www.python.org/dev/peps/pep-0008/

We should use the existing plugin classes. If there is no class for Bitwise Plugin in plugins.py, we should create one.

You use the same type of object in three places. It will be more efficient to create the class instance one time in the beginning and then reuse it.

More meaningful constant names will increase readability.

We should use the existing plugin classes. If there is no class for Bitwise Plugin in plugins.py, we should create one.

'cn=Bitwise Plugin,cn=plugins,cn=config' is already there in plugins.py

(Pdb) for i in Plugins(topo.standalone, DEFAULT_SUFFIX).list(): i.dn
'cn=7-bit check,cn=plugins,cn=config'
'cn=Account Policy Plugin,cn=plugins,cn=config'
'cn=Account Usability Plugin,cn=plugins,cn=config'
'cn=ACL Plugin,cn=plugins,cn=config'
'cn=ACL preoperation,cn=plugins,cn=config'
'cn=attribute uniqueness,cn=plugins,cn=config'
'cn=Auto Membership Plugin,cn=plugins,cn=config'
'cn=Binary Syntax,cn=plugins,cn=config'
'cn=Bit String Syntax,cn=plugins,cn=config'
'cn=Bitwise Plugin,cn=plugins,cn=config'
'cn=Boolean Syntax,cn=plugins,cn=config'
'cn=Case Exact String Syntax,cn=plugins,cn=config'
'cn=Case Ignore String Syntax,cn=plugins,cn=config'
'cn=chaining database,cn=plugins,cn=config'
'cn=Class of Service,cn=plugins,cn=config'
'cn=Content Synchronization,cn=plugins,cn=config'

We should use the existing plugin classes. If there is no class for Bitwise Plugin in plugins.py, we should create one.

'cn=Bitwise Plugin,cn=plugins,cn=config' is already there in plugins.py
(Pdb) for i in Plugins(topo.standalone, DEFAULT_SUFFIX).list(): i.dn

I mean Python classes which are defined like class PluginName(Plugin):

Please, run --pylint.
Also, it will mention that eval is a bad practice. It is slow, hard to debug, unsafe and hard to read and follow.

Please, rework it... I see that the only things that repeat in params are the filters and the number for the len().
So you can use them for the parametrization.

We should use the existing plugin classes. If there is no class for Bitwise Plugin in plugins.py, we should create one.
'cn=Bitwise Plugin,cn=plugins,cn=config' is already there in plugins.py
(Pdb) for i in Plugins(topo.standalone, DEFAULT_SUFFIX).list(): i.dn

I mean Python classes which are defined like class PluginName(Plugin):

PR raised: https://pagure.io/389-ds-base/pull-request/50359

Also, we have 'disable' and 'enable' for plugins

And please, make sure that the tests may run separately. Do not assume that the plugin is disabled.

@spichugi all changes are done as per your suggestion

I mean that it will be nicer to have the constant names that reflect the actual purpose.
You have only three of them here and each of them is used for the different situations.
So, please, try always name things meaningfully.

Also, now I am thinking that it will be more failproof if we will create a separate backend instead of using the default one. It will help us to make sure that the number of returned users are exact and won't be changed in the future (like DEFAULT_SUFFIX can have a different number of sample entries in different versions)

@spichugi all changes are done as per your suggestion , ready with new back end

Please, rebase PR with the latest master.
Also, check pylint output for your test case, it has a few issues.

@spichugi rebased .

pylint is giving error like :
Anomalous backslash in string: '*'. String constant might be missing an r prefix. (anomalous-backslash-in-string)

Which is part of filter . Cant be corrected

pylint is giving error like :
Anomalous backslash in string: '*'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
Which is part of filter . Cant be corrected

@aborah

>>> "\*"
'\\*'
>>> "\\*"
'\\*'
>>> r"\*"
'\\*'
>>> r"\\*"
'\\\\*'
>>> b"\*"
b'\\*'
>>> b"\\*"
b'\\*'
>>> "\n"
'\n'
>>> "\\n"
'\\n'

Notice how in the first case Python converts one backslash to two of them? It's Python being nice to you and not failing just because it does not know the escape sequence. :) For comparison, the \n is not changed into \\n because the \n actually is understood by Python as a request for a newline character.

That said, you should properly rewrite the Python strings so that they evaluate to the actually expected sequences. Just check out by hand with a Python interpreter on the fly.

@spichugi @mhonek all changes are done as per your suggestion

Please, don't ignore problems if you don't understand something. It is okay to ask if you haven't found the answer.
Pylint states W:122, 4: Global variable 'SUFFIX' undefined at the module level (global-variable-undefined)
So you should define the default value on the module level.

@spichugi all changes are done as per your suggestion

You can use SUFFIX here and everywhere instead of 'dc=anuj,dc=com'

@spichugi all changes are done as per your suggestion

LGTM! Ack

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3251

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.