Can't build an org chart using ActiveLdap 1.2.0

[GoogleCodeExporter]

I have a model that uses has_many to reference itself to build a tree. It's
my "Person" class, so this represents an org chart. Each record's "manager"
field contains the dn of that person's supervisor. The root of the chart is
a person who is their own manager.

I have updated from 1.0.9 to 1.2.0, and now the org chart doesn't work.

In the deprecation that was created in has_many to warn users about the
need to swap the primary and foreign key options, since my new primary key
is "dn" it always gets swapped. (This is because the "dn" attribute doesn't
exist for newly created objects?)

So I have to comment out the deprecation to get around it thinking I did it
wrong. I know I built my model correctly because it worked fine in 1.0.9
and I followed the directions to swap the options. Even if I comment the
deprecation out, I still cannot get back the proper list of subordinates.

With the sample code and data given below, this query

    Person.find('employee2').subordinates

should come back with employee4's record. Instead, I get employee2.

However

    Person.find('employee4').manager

does correctly return employee2's record.

Also,

    Person.find('employee1').subordinates

only returns employee1, when I should see employees 1 through 3.

Using: ActiveLdap 1.2.0 and OpenLDAP 2.4.11 on Linux 64 bit

Model:

class Person < ActiveLdap::Base
  ldap_mapping :dn_attribute => 'uid',
               :prefix => 'ou=People',
               :classes => ['top', 'inetOrgPerson'],
               :scope => :sub

#This is the association as it should be for ActiveLdap 1.2.0
  has_many :subordinates,
           :class => "Person",
           :foreign_key => "manager",
           :primary_key => "dn"

#This is the association that worked under ActiveLdap 1.0.9
#  has_many :subordinates,
#           :class => "Person",
#           :foreign_key => "dn",
#           :primary_key => "manager"

end

LDIF:

dn: dc=localhost
dc: localhost
o: Local Host
objectClass: top
objectClass: dcObject
objectClass: organization

dn: ou=People,dc=localhost
objectClass: organizationalUnit
ou: People

dn: uid=employee1,ou=People,dc=localhost
objectClass: inetOrgPerson
uid: employee1
sn: One
cn: Employee One
manager: uid=employee1,ou=People,dc=localhost

dn: uid=employee2,ou=People,dc=localhost
objectClass: inetOrgPerson
uid: employee2
sn: Two
cn: Employee Two
manager: uid=employee1,ou=People,dc=localhost

dn: uid=employee3,ou=People,dc=localhost
objectClass: inetOrgPerson
uid: employee3
sn: Three
cn: Employee Three
manager: uid=employee1,ou=People,dc=localhost

dn: uid=employee4,ou=People,dc=localhost
objectClass: inetOrgPerson
uid: employee4
sn: Four
cn: Employee Four
manager: uid=employee2,ou=People,dc=localhost

Original issue reported on code.google.com by culture...@gmail.com on 3 Nov 2009 at 12:25

[GoogleCodeExporter]

Tested against svn r1105, still broken.

Original comment by culture...@gmail.com on 3 Nov 2009 at 4:35

[GoogleCodeExporter]

I checked out the whole trunk history with git svn and did a bisect. This was
complicated by the first good revision and the latest revision being on opposite
sides of commits which introduced, removed, or updated the following:

1) gettext
2) rails version changes
3) ruby-ldap version changes
4) semantics of the options for has_many

So at each revision, I had to compensate for those factors. I was able to 
determine
where the problem was introduced. This is the "first bad commit":

http://ruby-activeldap.googlecode.com/svn/trunk@1021

This is not surprising, since it's the commit that changed the options for 
has_many.

Original comment by culture...@gmail.com on 3 Nov 2009 at 5:51

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I notice that has_many and has_many :wrap both use the same collect_targets 
method.
This appears to be fundamentally incompatible with the option switch for the 
non-wrap
case, unless you also impose the same switch on the :wrap case. The existing 
tests
never show any problem, because it appears that has_many without :wrap is never
tested with properties that have different names.

With :wrap, collect_targets looks up the wrapped property on the owning class, 
finds
a list of targets, and then looks up those targets on the foreign class. It 
makes the
assumption that "primary_key" names a property on the foreign class.

Without :wrap, collect_targets looks up the primary key property on the owning 
class.
Then it searches for foreign class objects that match, but the attribute it 
uses to
search is the same primary key name. The foreign key name is never used.

I recommend that either the option switch be reverted, or :wrap be made to use
:foreign_key so that its logic matches the non-wrap usage.

Original comment by culture...@gmail.com on 5 Nov 2009 at 4:33

[GoogleCodeExporter]

Ah, the tests I wrote were wrong. Deleted. Ignore the tests. Working on a 
better patch.

Original comment by culture...@gmail.com on 5 Nov 2009 at 4:48

[GoogleCodeExporter]

As promised, attached, please find a set of patches which:

1) correct the behavior of has_many without :wrap, in the case where the 
primary and
foreign key names are not the same

2) causes has_many :wrap to prefer :foreign_key, but allows :primary_key in a
backwards compatible manner

3) includes a test for item 1

Original comment by culture...@gmail.com on 5 Nov 2009 at 6:16

Attachments:

• has_many_of_self.patch

[GoogleCodeExporter]

Thanks for your patch! It includes a test!!!
I've applied your patch with some modifications.

Original comment by kou...@gmail.com on 7 Nov 2009 at 1:43

• Changed state: Fixed

[GoogleCodeExporter]

Wow golly... I spent half of yesterday and half of today trying to get a 
"manager"
relationship to work and was going nuts.  I finally chainsawed the code to get 
it
working.  Was going to post a patch or ask a question and bumped into this...

I kept assuming that something this basic can't be broken for long without an 
update.
 Maybe a new level should be pushed out.

Thanks!

Original comment by pedz...@gmail.com on 26 Nov 2009 at 5:41