Description of problem: Analysis by andrey.ivanov > I'm pretty sure the referential integrity plug-in will not work for > modrdn operations with a new superior. Looking more thoroughly through > the code ( ldap / servers / plugins / referint / referint.c) confirms > my suspicion that new rdn superior is not taken into account. The > function referint_postop_modrdn extracts from the parameter block > only SLAPI_MODRDN_TARGET and SLAPI_MODRDN_NEWRDN, it does not extract > SLAPI_MODRDN_NEWSUPERIOR neither passes it further down the utility > functions - update_integrity(argv, dn, newrdn, logChanges) and > writeintegritylog(argv[1],dn, newrdn). The same applies to the delayed > referint operations (the plug-in writes to the special integrity log > file only the old DN and the new RDN, but never the new superior : > writeintegritylog(argv[1],dn, newrdn);) Another thought on the subject of referential integrity plug-in - in the previous mail i have only mentioned one-entry renames with a new superior. The things get even worse when we rename a whole non-empty sub-tree. It means that the referential integrity plug-in should change to the new DNs all the references to all the entries of the whole sub-tree, not only for one entry. And what if we rename a sub-tree containing both the referenced entry and the entry referencing the first one's DN in one of its "integrity" attributes? It actually means that we need at first make the rename and then all the searches and replacements. Though it seems it's already the case as it's a post-op(?) plug-in...
Created attachment 386896 [details] git patch for ldap/servers/plugins/referint/referint.c Fix Description: The referential integrity plugin has not supported the subtree rename (modrdn with newsuperior). This patch is adding the support.
Created attachment 386898 [details] test data How to verify: 1. import the attached data (example1k.ldif) and restart the server 2. rename uid=admin to uid=admin0 dn: uid=admin, dc=example,dc=com changetype: modrdn newrdn: uid=admin0 deleteoldrdn: -1 2-1. check the seeAlso, owner, and member attribute values to verify that uid=admin has been replaced with uid=admin0. 3. move uid=admin0 to ou=admin,dc=example,dc=com dn: uid=admin0, dc=example,dc=com changetype: modrdn newrdn: uid=admin0 deleteoldrdn: -1 newsuperior: ou=admin,dc=example,dc=com 3-1. check the seeAlso, owner, and member attribute values to verify that uid=admin0 has moved to ou=admin,dc=example,dc=com. 4. move the rdn and superior to the original dn: uid=admin0, ou=admin,dc=example,dc=com changetype: modrdn newrdn: uid=admin deleteoldrdn: -1 newsuperior: dc=example,dc=com 4-1. check the seeAlso, owner, and member attribute values are replaced with uid=admin,dc=example,dc=com.
Created attachment 387204 [details] git patch for ldap/servers/plugins/referint/referint.c Comment from andrey.ivanov: The patch you provided does take into account the modRDN with new superior but it does not seem to take care of subtree renames (smth like rename "ou=users" to "ou=utilisateurs" with child entries). For subtree renames all the entries in the renamed subtree change their DNs so for each of these renamed entries we have to make the search "slapi_search_internal_set_pb(search_result_pb, search_base, LDAP_SCOPE_SUBTREE, filter, attrs..." with a different filter. So for each modrdn we should verify if the renamed entry has any child entries and if it is the case then treat them either directly or recursively... Fix Description: The referential integrity plugin has not supported the subtree rename (modrdn with newsuperior). This patch is adding the support. There are 2 typical cases. (case 1) DN that modrdn modifies matches the value of attributes which is the target of the referential integrity. E.g., modrdn: uid=A,ou=B,o=C --> uid=AA,ou=BB,o=C then, member: uid=A,ou=B,ou=C --> uid=AA,ou=BB,ou=C seeAlso: uid=A,ou=B,ou=C --> uid=AA,ou=BB,ou=C (case 2) DN that modrdn modifies is the ancestor of the value of attributes which is the target of the referential integrity. E.g., modrdn: ou=B,o=C --> ou=BB,o=C then, member: uid=A,ou=B,ou=C --> uid=A,ou=BB,ou=C seeAlso: uid=A,ou=B,ou=C --> uid=A,ou=BB,ou=C
Reviewed by Rich (Thank you!!) Pushed to master. $ git merge subtree Updating d9fdaf2..3fdcbdd Fast forward ldap/servers/plugins/referint/referint.c | 702 +++++++++++++++++++++--------- 1 files changed, 493 insertions(+), 209 deletions(-) $ git push Counting objects: 13, done. Delta compression using 2 threads. Compressing objects: 100% (7/7), done. Writing objects: 100% (7/7), 4.18 KiB, done. Total 7 (delta 4), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git d9fdaf2..3fdcbdd master -> master
Noriko, the patch looks ok. I have only two comments concerning this patch. 1) Documentation impact : Since the new code makes use of the substring filter (%a=*%e), maybe the documentation should contain a recommendation to make not only presence and equality, but also substring index on referential plug-in attributes. Otherwise the internal search will be unindexed. 2) A question : in _update_one_per_mod() you do the following with the sval string : p = PL_strstr(sval, norm_origDN); bak = *p; *p = '\0'; newvalue = slapi_ch_smprintf("%s%s", sval, newDN); *p = bak; ... slapi_ch_free_string(&sval); Apparently you want to save the previous value of *p to make a correct slapi_ch_free_string (the string is freed up to '\0'?) Now, in _update_all_per_mod(): p = PL_strstr(sval, norm_origDN); *p = '\0'; ... slapi_ch_free_string(&sval); Here you don't save the previous value of *p. Will the slapi_ch_free_string function free the memory correctly? If the answer is yes, then we don't need the "bak" variable in _update_one_per_mod function...
Hi Andrey, thanks a lot for your review! My comments are inline. (In reply to comment #5) > Noriko, the patch looks ok. I have only two comments concerning this patch. > > 1) Documentation impact : > Since the new code makes use of the substring filter (%a=*%e), maybe the > documentation should contain a recommendation to make not only presence and > equality, but also substring index on referential plug-in attributes. Otherwise > the internal search will be unindexed. A very good point! I'm going to open a doc bug. > 2) A question : in _update_one_per_mod() you do the following with the sval > string : > > p = PL_strstr(sval, norm_origDN); > bak = *p; > *p = '\0'; > newvalue = slapi_ch_smprintf("%s%s", sval, newDN); > *p = bak; > > ... > slapi_ch_free_string(&sval); > > Apparently you want to save the previous value of *p to make a correct > slapi_ch_free_string (the string is freed up to '\0'?) I don't think so... "free" releases the memory allocated by "malloc" or its friends regardless of the contents. For instance, if you allocate some amount of memory using calloc, which fills the allocated memory with '\0', but the following "free" correctly frees the memory. The reason why I recovered the character in _update_one_per_mod is I wanted to print the original "sval" value in the error at line 465. On the other hand, it's not done in _update_all_per_mod. Thus, I did not get bothered to put the character back. > Now, in > _update_all_per_mod(): > > p = PL_strstr(sval, norm_origDN); > *p = '\0'; > ... > slapi_ch_free_string(&sval); > > Here you don't save the previous value of *p. Will the slapi_ch_free_string > function free the memory correctly? If the answer is yes, then we don't need > the "bak" variable in _update_one_per_mod function...
We have a test case for this bug in subtree renaming test suit : /home/amsharma/DS9.0/trunk/testcases/DS/6.0/subtreeRenames subtree_35 subtreeRenames startup 100% (1/1) subtreeRenames run 100% (32/32) subtreeRenames cleanup 100% (1/1) Hence marking as VERIFIED.