[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Freeipa-devel] [PATCHES] 213-224 Use old entry state in LDAP mods

On 12/18/2013 12:50 PM, Jan Cholasta wrote:
On 11.12.2013 18:28, Petr Viktorin wrote:
On 12/11/2013 05:05 PM, Petr Viktorin wrote:
On 12/10/2013 02:10 PM, Jan Cholasta wrote:

the attached patches fix


These look great, thanks! Just a couple of questions/nicpicks.

213: ACK
214: ACK
215: ACK
216: ACK
217: ACK
218: ACK

Does the new method guarantee 'attributetypes' are updated before
I can move the logic to schemaupdate if needed.

That won't be necessary, fixed.

Why is OnlyOneValueAllowed not raised any more? It should be, since the
method assumes get_single_value() gives correct information.


Why is MOD_REPLACE no longer used when no old values are retained? Or
(MOD_DELETE, a, None) when the new set is empty?


However, I am not convinced if this guessing is the right approach. I
think we can do better, by either not guessing:

     del entry[attr] -> delete attribute
     entry[attr] = value -> replace attribute
     entry[attr][index] = value -> delete old value, add new value

or by guessing better, to minimize the size of the modify request.

220: ACK

An ancient comment in ldapupdate still has a reference to orig_data in,
can you remove the comment as well?


222: ACK
223: ACK
224: ACK

I spoke too soon, NACK. This line:

 > -from ipaserver.plugins import ldap2

is important, please put it back. Without it api.Backend.ldap2 will not
be available and upgrades will fail with AttributeError.

(Yes, I know.)

Fixed. (But it's retarded.)

I noticed that IPASimpleLdapObject.convert_result's docstring is
obsolete, could you update/shorten it?


Also fixed some more issues I noticed.

It would be nice to write unit tests for issues as you notice them.

Updated patches attached.

I now have a failing test in test_permission_rollback. Let's think about this case for a moment:

The permission system has "rollback": if an ACI update fails, the entry is rolled back. Currently it works (for ipapermlocation changes) like this:

- The old entry is retreived
- A new entry is populated (NB, the framework's mod operation does not retrieve the entry it modifies; rather it builds an entirely new entry with *only* the data that's changed, and relies on generate_modlist doing MOD_REPLACE when orig data is missing).
- update is called on the new entry
- The ACI is updated, and this fails (e.g. with SyntaxError)
- update is called on the *old* entry retreived in the first step. Up to now this had restored the entry (since existing state was looked up before each mod), but with these patches it raises EmptyModlist since the object has not changed relative to its orig data.

Obviously this approach is wrong given how entry is supposed to work now, and I'll be happy to change it it. But it's not clear what's the right way to do such rollback.

I'd like to discuss in person after the holidays, so we sync our expectations of how ipaldap should work.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]