Thanks for the explanation. I now know what these two lines are supposed to do,
but apparently they don't work.
I removed the (redundant) check from modify_pmcchairs for testing and:
[MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --add clr
Password for clr:
[MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --add clr
Password for clr:
_WARN [#<LDAP::Mod:0x7f966a1238a8 LDAP_MOD_ADD
{"member"=>[]}>]
IIUC this means that the
_WARN cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
/Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify': Protocol
error (LDAP::ResultError)
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify'
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:1377:in `add'
from ./modify_pmcchairs.rb:36:in `block in <main>'
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
from ./modify_pmcchairs.rb:36:in `<main>'
[MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --rm clr
Password for clr:
[MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --rm clr
Password for clr:
_WARN [#<LDAP::Mod:0x7fc03380b6f0 LDAP_MOD_DELETE
{"member"=>[]}>]
_WARN cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
/Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify': Object
class violation (LDAP::ResultError)
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify'
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:1368:in
`remove'
from ./modify_pmcchairs.rb:38:in `block in <main>'
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
from ./modify_pmcchairs.rb:38:in `<main>'
Here's the fix:
diff --git a/lib/whimsy/asf/ldap.rb b/lib/whimsy/asf/ldap.rb
index 6f2065b..9d1c9f7 100644
--- a/lib/whimsy/asf/ldap.rb
+++ b/lib/whimsy/asf/ldap.rb
@@ -1365,6 +1365,7 @@ module ASF
def remove(people)
@members = nil
people = (Array(people) & members).map(&:dn)
+ return if people.empty?
ASF::LDAP.modify(self.dn, [ASF::Base.mod_delete('member', people)])
ensure
@members = nil
@@ -1374,6 +1375,7 @@ module ASF
def add(people)
@members = nil
people = (Array(people) - members).map(&:dn)
+ if people.empty?
ASF::LDAP.modify(self.dn, [ASF::Base.mod_add('member', people)])
ensure
@members = nil
I think for human experience it's bad to return without doing anything and
without giving the user information that it's expected that nothing was done.
Now that I know how this is supposed to work, the issue is where the checking
should be done and how to report the issue to the user.
The code in ldap.rb is used everywhere so it is probably not good to puts
messages but perhaps better to raise a meaningful error instead of Object class
exception.
Craig
> On Jan 9, 2018, at 8:13 PM, Sam Ruby <ru...@intertwingly.net> wrote:
>
> On Tue, Jan 9, 2018 at 6:11 PM, Craig Russell <apache....@gmail.com> wrote:
>>
>>> On Jan 8, 2018, at 11:55 PM, Sam Ruby <ru...@intertwingly.net> wrote:
>>>
>>> On Mon, Jan 8, 2018 at 11:34 PM, Craig Russell <apache....@gmail.com> wrote:
>>>>
>>>>> On Jan 8, 2018, at 7:32 PM, Sam Ruby <ru...@intertwingly.net> wrote:
>>>>>
>>>>> On Mon, Jan 8, 2018 at 7:07 PM, Craig Russell <apache....@gmail.com>
>>>>> wrote:
>>>>>> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify':
>>>>>> Object class violation (LDAP::ResultError)
>>>>>>
>>>>>> And error reporting is not great. I guess more checking is needed but
>>>>>> Object class violation is not very informative.
>>>>>
>>>>> Oh, and AGREED!
>>>>>
>>>>> LDAP sucks. You want to add zero members: Object class violation.
>>>>> You want to add somebody who is already a member: Object class
>>>>> violation. You want to remove somebody who is not a member: Object
>>>>> class violation.
>>>>>
>>>>> That's why the action that caused the error is logged. In this case:
>>>>>
>>>>> LDAP_MOD_DELETE
>>>>> {"member"=>[]}>
>>>>> cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
>>>>>
>>>>> Here you are deleting nobody. That apparently is not allowed.
>>>>
>>>> Seems like there is some error checking that could be done in the ldap.rb
>>>> code.
>>>>
>>>> Here is the remove code from lib/whimsy/asf/ldap.rb :
>>>>
>>>> # remove people from this service in LDAP
>>>> def remove(people)
>>>> @members = nil
>>>> people = (Array(people) & members).map(&:dn)
>>>> ASF::LDAP.modify(self.dn, [ASF::Base.mod_delete('member', people)])
>>>> ensure
>>>> @members = nil
>>>> end
>>>>
>>>> Who wrote this code?
>>>
>>> That would be me.
>>>
>>>> Would it be possible for this code to check before calling
>>>> ASF::LDAP.modify that the member actually exists already?
>>>
>>> The line above it takes the intersection between the list of people
>>> passed to remove and the current set of members. So if a person who
>>> is not a member of the service is passed in, they will be silently
>>> removed from the set.
>>
>> Evidence suggests that the line and the corresponding line in Service.add
>> are not working as intended.
>>
>> Where does the variable members come from? Is it different from @members
>> that is set to nil just before trying to remove existing members?
>
> members is a method call. Essentially, self.members().
>
> https://whimsy.apache.org/docs/api/ASF/Service.html
>
> @members is the cached value for this function. Setting it to nil
> will force the members method to fetch new data from LDAP.
>
> - Sam Ruby
>
>>> I'm not sure how to refactor it, but there are add and remove methods
>>> for a number of LDAP objects: Committee, Project, etc.
>>
>> I'm not sure that a refactoring is needed, just testing to make it work as
>> intended.
>>
>> Craig
>>>
>>> - Sam Ruby
>>>
>>>> Craig
>>>>
>>>>> - Sam Ruby
>>>>>
>>>>> - Sam Ruby
>>>>
>>>> Craig L Russell
>>>> Secretary, Apache Software Foundation
>>>> c...@apache.org http://db.apache.org/jdo
>>
>> Craig L Russell
>> Secretary, Apache Software Foundation
>> c...@apache.org http://db.apache.org/jdo
>>
Craig L Russell
Secretary, Apache Software Foundation
c...@apache.org http://db.apache.org/jdo
