Bug 32781 - modules/univention/admin/modules.py: pos undefined, unstable sort
modules/univention/admin/modules.py: pos undefined, unstable sort
Product: UCS
Classification: Unclassified
Component: UDM - Extended Attributes
UCS 4.2
Other Linux
: P5 normal (vote)
: UCS 4.2-1-errata
Assigned To: Johannes Keiser
Florian Best
: 28171 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2013-10-01 16:04 CEST by Philipp Hahn
Modified: 2017-07-26 14:39 CEST (History)
3 users (show)

See Also:
What kind of report is it?: Development Internal
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Ticket number:
Bug group (optional):
Max CVSS v3 score:
best: Patch_Available+

Fix syntax error (2.38 KB, patch)
2013-10-01 16:04 CEST, Philipp Hahn
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philipp Hahn univentionstaff 2013-10-01 16:04:56 CEST
Created attachment 5488 [details]
Fix syntax error

>	if tabPosition == -1:
>		for ea_layout in properties4tabs[ tabname ]:
>			try:
>				if ea_layout.position <= tabPosition:
>					# CLEANUP, FIXME: pos is undefined!
>					# does not break because of except:
>					tabPosition = pos-1
>			except:
>				ud.debug(ud.ADMIN, ud.WARN, 'modules update_extended_attributes: custom field for tab %s: failed to set tabPosition' % tabname)

This code was copied from UCS-2.4-4 and wrongly adapted for UCS-3.0-0.
At a minimum "pos" needs to be renamed to "ea_layout.position".

FYI: What this gem of code is trying to achieve is to calculate a unique *priority* in case that no tapPosition is explicitly set. Since normally positive integers are used to specify the order,  negative integers guarantee that.

This "tabPosition" is then used by EA_Layout() to sort the EAs first by "groupName" and than by that "tabPosition". Then the EAs are added in that order to the tab:
>	for tabname, priofields in properties4tabs.items():
>		priofields.sort()

This sort algorithm is not stable, as the LDAP search result is not sorted by default and thus the EAs can be returned in any order:
>	for dn, attrs in lo.search( base = position.getDomainConfigBase(),
>		filter='(&(objectClass=univentionUDMProperty)(univentionUDMPropertyModule=%s)(univentionUDMPropertyVersion=2))' % name(module) ):

1. Apply the patch to fix the Syntax eror
2. "tabPosition" should be renamed to "priority".
Comment 1 Florian Best univentionstaff 2016-09-23 12:28:02 CEST
This happens to me every time I want to commit the UDM package because flake8 complains.
Comment 2 Florian Best univentionstaff 2017-07-05 14:28:42 CEST
Please apply the patch and additionally add "and properties4tabs[tabname]" to the "if tabPosition == -1:" condition.
Comment 3 Johannes Keiser univentionstaff 2017-07-05 16:01:33 CEST
(In reply to Philipp Hahn from comment #0)
> Created attachment 5488 [details]
> Fix syntax error

(In reply to Florian Best from comment #2)
> Please apply the patch and additionally add "and properties4tabs[tabname]"
> to the "if tabPosition == -1:" condition.

Applied rebased patch:

r 80876
univention-directory-manager-modules (12.0.17-60) 
* Bug #32781: Applied patch from Philipp Hahn - Fix syntax error for
updating the position of extended attributes

YAML: r 80881
Comment 4 Johannes Keiser univentionstaff 2017-07-05 16:51:16 CEST
Applied patch:

r 80888
univention-directory-manager-modules (12.0.17-63) 
* Bug #32781: Applied additional patch from Florian Best
Comment 5 Florian Best univentionstaff 2017-07-11 18:22:04 CEST
I could trigger an exception with the following extended attributes:

# udm settings/extended_attribute list | grep -e ^DN: -e tabPosition -e tabName
DN: cn=free1,cn=custom attributes,cn=univention,dc=school,dc=local
  tabName: test
  tabPosition: 2
DN: cn=free2,cn=custom attributes,cn=univention,dc=school,dc=local
  tabName: test
  tabPosition: None
DN: cn=free3,cn=custom attributes,cn=univention,dc=school,dc=local
  tabName: test
  tabPosition: 2
DN: cn=free4,cn=custom attributes,cn=univention,dc=school,dc=local
  tabName: test
  tabPosition: None

Die Ausführung des Kommandos udm/layout users/user ist fehlgeschlagen:

Traceback (most recent call last):
  File "%PY2.7%/univention/management/console/base.py", line 249, in execute
    function.__func__(self, request, *args, **kwargs)
  File "%PY2.7%/univention/management/console/modules/udm/__init__.py", line 117, in _decoarated
    ret = [func(self, request) for request.options in options]
  File "%PY2.7%/univention/management/console/modules/decorators.py", line 192, in _response
    return function(self, request)
  File "%PY2.7%/univention/management/console/modules/udm/__init__.py", line 786, in layout
    module.load(force_reload=True)  # reload for instant extended attributes
  File "%PY2.7%/univention/management/console/modules/udm/udm_ldap.py", line 265, in load
    self.module = _module_cache.get(module, force_reload=force_reload)
  File "%PY2.7%/univention/management/console/modules/udm/udm_ldap.py", line 82, in _decorated
    return method(*args, **kwargs)
  File "%PY2.7%/univention/management/console/ldap.py", line 143, in _decorated
    result = func(*args, **kwargs)
  File "%PY2.7%/univention/management/console/modules/udm/udm_ldap.py", line 234, in get
    udm_modules.init(ldap_connection, ldap_position, self[name], template_object, force_reload=force_reload)
  File "%PY2.7%/univention/admin/modules.py", line 135, in init
    update_extended_attributes(lo, module, position)
  File "%PY2.7%/univention/admin/modules.py", line 509, in update_extended_attributes
    currentTab.insert(ea_layout.position, ea_layout.name)
  File "%PY2.7%/univention/admin/layout.py", line 143, in insert
    if isinstance(self.layout[currentLine], basestring):
IndexError: list index out of range
Comment 6 Florian Best univentionstaff 2017-07-11 18:44:55 CEST
for ((i=1; i<=15; i++)); do udm settings/extended_attribute create --set name=free$i --set shortDescription=free$i --set module=users/user --set objectClass=univentionFreeAttributes --set ldapMapping=univentionFreeAttribute$i --set tabName=test --set tabPosition=$i --position "cn=custom attributes,cn=univention,$(ucr get ldap/base)"; done
Comment 7 Florian Best univentionstaff 2017-07-11 18:49:17 CEST
I can trigger the same exception with the old code when setting any tabPosition to 0.
Comment 8 Florian Best univentionstaff 2017-07-11 18:57:38 CEST
This is effectively Bug #28171.
Comment 9 Florian Best univentionstaff 2017-07-11 19:00:00 CEST
*** Bug 28171 has been marked as a duplicate of this bug. ***
Comment 10 Johannes Keiser univentionstaff 2017-07-12 12:09:33 CEST
Applied patch:

r 81072
univention-directory-manager-modules (12.0.17-81) 
* Bug #32781: Applied patch from Florian Best - Fix error with tabPosition=0

YAML: r 81073
Comment 11 Florian Best univentionstaff 2017-07-12 12:23:40 CEST
OK: layout looks as before
OK: layoutPosition=0 doesn't raise exception anymore
Comment 12 Erik Damrose univentionstaff 2017-07-26 14:39:10 CEST