#48820 lib389: Make consistent set/get properties api
Closed None Opened 4 years ago by firstyear.

Right now we have a very inconsistent api for lib389 get and set properties.

For example, in lib389/changelog.py, we have:

def setProperties(self, changelogdn=None, properties=None):
...
   for prop in properties:
      if str(prop).startswith('+'):
          op = ldap.MOD_ADD
 ....

In other places:

https://fedorahosted.org/389/attachment/ticket/48663/0001-Ticket-48663-Implement-get-and-set-properties-for-ba.2.patch

to:

lib389/backend.py

raise NotImplemented

They all take different arguments to identify the resource we are working on. Some of these are singleton, some of these take dns, some are entries, some take a variety of data points.

I think that we can make this better.

For singleton, we can have a base type that is subclassed. This would define the DN of the relevant configs, and the set/get prop methods done to take a dictionary of values that is consumed by ldap modlist. This way, we can much more easily drop in / out changes, and unify how we access some configs. This doesn't preclude wrapping some common ops in wrappers that just call get/set for convinence, and in fact, makes it easier to do.

For types with multiple instances (say backends, replication agreement), we can have a get backend / create backend parent that selects the correct backend. This then acts like a factory to hand back an instance of the backend with the singleton type, and the dns all set correctly. From there we can just use the same methods.

Essentially. I imagine something like:

standalone.changelog.setProperties({'nsslapd-changelog-triminterval': '5'})
standalone.encryption.enable()
# Under the hood this would call self.setProperties({nsslapd-secure: on}) etc.

be = standalone.backend.get(suffix="dc=example,dc=com")
be = standalone.backend.get(name="userRoot")
# This uses the factor pattern to select and return the right be.
be.setProperties({'dbcachememsize': '9001'})
# Now we can set properties
be.enable_readonly()
# We can still have specific functions for the type that just calls get/set underneath.

Question: Won't this break things?
Answer: Probably. Our tests are pretty inconsistent, we have some that call these helpers, some that call direct ldap mods. But in the long term, making this cleaner, and more consistent is going to make all our lives easier. It means we won't need to work so hard to add new resources. For example, we could add a new resource with as little code as:

class SomePluginConfig(LdapConfigObject):
    def __init__(self)
        self.dn = 'cn=someplugin,cn=plugins,cn=config'

And bang, we have a type that inherits get/set properties, and we can probably even start to think about some debug, listing and other nice base types to include.

Question: But things work, why do you want to change it?
Answer: with rest389 on the horizon, it's becoming more apparent that if we are to clean up and polish lib389, now is the time to do it before we are too far dependent on it's quirks.


Another benefit is that we can then implement an offline config reading mode for when the directory server state is offline. While online out baseClass knows to do the ldap search. But when offline, it knows to read and parse dse.ldif. This gives us more flexibility, at zero cost for all the new configs and types we create.

Fixes for python 3 to enable the consistent api
0001-Ticket-Fixes-for-python-3.patch

Hi William,

thank you! It is really - wow!

I've taken a look at your changes, but not much thoroughly, because there is a lot of stuff. I think we will find an issues (if there is any) while using at testing.

But I've tested your patches with our current test suites and there is a few failures. All of them are at '''lib389/backend.py'''.

If we won't put any args to '''topology.standalone.backend.create()''', it will fail like this:
{{{
# suffix is mandatory

  if properties.get(BACKEND_SUFFIX) is not None:

E AttributeError: 'NoneType' object has no attribute 'get'
}}}

I think, it would be better if it will give some sane exception. It was ldap.UNWILLING_TO_PERFORM before.

Also, we will get the same error, if we put '''topology.standalone.backend.create(suffix=NEW_SUFFIX_1)''' - so without properties specified. But it should work like this anyway.

The rest looks good.

Thanks,
Simon

Hi Simon,

Thanks for the review! I appreciate it: Still a long way to go with this code, but I'm glad you like the style and idea of it.

I am attaching a patch that fixes the backend tests. I realised what the mistake was.

Thanks, and I hope that this looks better to you this time!

Thanks! All tests have passed.

commit 683d6bdfbe450b35fb219a8a1c1d13b989ce661e
commit e45750651e462962ab5744d7faef24eddf0fafa8
commit c285f090623abb7a8fac04c91ddcbdb83c8d274f
Writing objects: 100% (22/22), 11.31 KiB | 0 bytes/s, done.
Total 22 (delta 14), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/lib389.git
ab53dca..c285f09 master -> master

Leaving ticket open to track the re-implementations of other types.

Hi William,
you are really working hard! Thanks!

So about patches.
First one looks good.
Second... I have a few really small questions about it:

'''lib389/config.py:35:'''
{{{

self._instance = conn

}}}

So it is commented, but used afterwards. Or have I missed something?
Also, I think, this enable_ssl shouldn't be used at all and we plan to move it, yes?

'''lib389/tests/dsadmin_basic_test.py:18 and 110 and many other files'''
{{{
18 #from nose.tools import *
...
110 #@raises(NoSuchEntryError)
}}}

Why is it commented? Shouldn't we just remove it for good? I guess, we've decioded to go along with '''pytest'''.

Beside this small stuff - ack.

commit e45750651e462962ab5744d7faef24eddf0fafa8
{{{
+ install_requires=['python-ldap', 'dateutil'],
}}}
It's python-dateutil, not dateutil. Because of this lib389 is not installable from git anymore.
Also, if you add new dependencies, please update requirements.txt.
Thanks!

Replying to [comment:11 spichugi]:

Hi William,
you are really working hard! Thanks!

Always! Thanks for the review.

So about patches.
First one looks good.
Second... I have a few really small questions about it:

'''lib389/config.py:35:'''
{{{

self._instance = conn

}}}

self._instance is set in the super class. See _mapped_object.py :70

So it is commented, but used afterwards. Or have I missed something?
Also, I think, this enable_ssl shouldn't be used at all and we plan to move it, yes?

We still have some code that relies on it. The idea is to leave it inplace for now, update and create the "new types", then we can remove enable_ssl and fix the tests to rely on the new functions.

It's a rolling replacement.

'''lib389/tests/dsadmin_basic_test.py:18 and 110 and many other files'''
{{{
18 #from nose.tools import *
...
110 #@raises(NoSuchEntryError)
}}}

Why is it commented? Shouldn't we just remove it for good? I guess, we've decioded to go along with '''pytest'''.

That's right. Really, we just need to delete those two files I think, since we aren't actually running the tests in them. What do you think?

Beside this small stuff - ack.

commit e2669069f636cbda699320a312f46e02a41fbde6
commit 96728ba3cbe9003b828aaa102f8761df833746d7
commit 1f88f1d4b0649b0fd391af96975968b7911b4b90
Writing objects: 100% (27/27), 4.98 KiB | 0 bytes/s, done.
Total 27 (delta 23), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/lib389.git
c285f09..1f88f1d master -> master

Hi William,
what was the purpose for moving changes from #48853 to this ticket?

I think, changes like adding new class SetupDs(object) and etc would look more appropriate at #48853 '''[RFE: lib389, experimental. ds-setup(.py)]''', because they relates to its topic more than to topic of the current ticket.

Also, I would like to leave a review of '''ds-setup(.py)''' to the developer part of our team, because their setup-ds knowledge is much more deeper than mine. Even though I can do this, theirs opinion is matter here.

Thanks,
Simon

Replying to [comment:15 spichugi]:

what was the purpose for moving changes from #48853 to this ticket?

It was an accident because git hates me :)

Will fix these patches up now.

Part 3: Py test fixes, and python 3. Move backend test to new style api.
0001-Ticket-48820-Begin-to-test-compatability-with-py.tes.patch

After this patch, I'm pretty happy with where it's at. We have a working backend test, and lots of improvements. Next step after this is acked I think is to open tickets related to each type (index, mapping tree, replica, agreement) to re-write those.

What do you think?

Current test score card:

{{{
python 2:

platform linux2 -- Python 2.7.11, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/wibrown/development/389ds/lib389, inifile:
collected 66 items

lib389/lib389/tests/aci_test.py ..
lib389/lib389/tests/agreement_test.py .......F
lib389/lib389/tests/backend_test.py .....
lib389/lib389/tests/dereference_test.py .
lib389/lib389/tests/dirsrv_log_test.py ...
lib389/lib389/tests/dirsrv_test.py .....
lib389/lib389/tests/effective_rights_test.py .
lib389/lib389/tests/entry_test.py ......
lib389/lib389/tests/krb5_create_test.py .
lib389/lib389/tests/ldclt_test.py .
lib389/lib389/tests/mappingTree_test.py .....
lib389/lib389/tests/nss_ssl_test.py .
lib389/lib389/tests/replica_test.py .......
lib389/lib389/tests/schema_test.py .
lib389/lib389/tests/suffix_test.py ...
lib389/lib389/tests/test_module_proxy.py ........
lib389/lib389/tests/utils_test.py ........

python 3:

platform linux -- Python 3.5.1, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/wibrown/development/389ds/lib389, inifile:
collected 66 items

lib389/lib389/tests/aci_test.py ..
lib389/lib389/tests/agreement_test.py EEEEEEEE
lib389/lib389/tests/backend_test.py .FFFF
lib389/lib389/tests/dereference_test.py F
lib389/lib389/tests/dirsrv_log_test.py ...
lib389/lib389/tests/dirsrv_test.py EEEEE
lib389/lib389/tests/effective_rights_test.py F
lib389/lib389/tests/entry_test.py .....F
lib389/lib389/tests/krb5_create_test.py E
lib389/lib389/tests/ldclt_test.py F
lib389/lib389/tests/mappingTree_test.py .FFF.
lib389/lib389/tests/nss_ssl_test.py F
lib389/lib389/tests/replica_test.py FFFFFF.
lib389/lib389/tests/schema_test.py .
lib389/lib389/tests/suffix_test.py FFF
lib389/lib389/tests/test_module_proxy.py ........
lib389/lib389/tests/utils_test.py ........

}}}

Hi William,
looks good to me.
One thing only, please, disable Verbose at '''lib389/tests/backend_test.py'''.

Talking about future work... Sounds good, but lets move the discussion to mail-list. :)

Thanks,
Simon

Looks good, looks like there are indentation issue in lib389/tests/backend_test.py.

Please make sure to run pep8 on all of your python code. Since we getting ready to do a respin I'd like everything to be clean.

Thanks!

Do you want me to run pep8 on this and re-submit? OR make a new patch with the pep8 fixes?

Replying to [comment:21 firstyear]:

Do you want me to run pep8 on this and re-submit? OR make a new patch with the pep8 fixes?

Either one is fine, your call.

commit 665889e1f66da9708c05c6a5a683fcdc41e0b10a
Total 26 (delta 22), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/lib389.git
924de2f..1c09a99 master -> master

I will do the pep8 fixes as a new patch today for you.

See #48878, I'll do the pep8 / prerelease clean up there.

Metadata Update from @vashirov:
- Issue assigned to firstyear
- Issue set to the milestone: lib389 1.0.3

3 years ago

Metadata Update from @mreynolds:
- Issue close_status updated to: None (was: Fixed)
- Issue set to the milestone: None (was: lib389 1.0.3)

4 months ago

Login to comment on this ticket.

Metadata