Looks good!
One small nitpick - the error output has the "pythonic leftovers":

$ sudo dsidm localhost -b dc=example,dc=com group modify new add:description:qwe
Error: {'desc': 'Type or value exists'}
$ sudo dsidm localhost -b dc=example,dc=com group modify new delete:description:qwe
Error: {'desc': 'No such attribute'}

It will be nice to get the 'desc' value and use it in the output.

I'm not actually sure I can fix this. This is part of how the error presentation code is dsctl/dsconf/dsidm works.

If you look in dsidm about line 126, you'll see a try/except block, and in the except we do:

log.debug(e, exc_info=True)
log.error("Error: %s" % str(e))

So we attempt to string-ify the exception. The issue is that most exceptions are fine for this:

dirsrv@ldapkdc:/home/william/development/389ds/ds/src/lib389> PYTHONPATH=./ ./cli/dsidm localhost group modify demo add:description:test
Error: No object exists given the filter criteria demo

So that prints a lovely message. If we look at -v it's from ldap.NO_SUCH_OBJECT: No object exists given the filter criteria demo

But ldap.TYPE_OR_VALUE_EXISTS has a dict instead, and that's why it formats weirdly.

So I think this is a fault of how str/unicode on ldap.TYPE_OR_VALUE_EXISTS works, not a fault of my patch :)

I'll let you comment on this first before I push

So I think this is a fault of how str/unicode on ldap.TYPE_OR_VALUE_EXISTS works, not a fault of my patch :)
I'll let you comment on this first before I push

Yeah, but if we leave it like this in the CLI, there will be complains for sure because it doesn't look "clean".
What I propose is to have exception processing at some point in the CLI.
We can get the description from e.args[0]['desc']. Yeah, it is not ideal but it will fix the issue.

So either at some point around log.error("Error: %s" % str(e)), either somewhere areound _generic_modify.
What do you think?

Also, it can be another PR. It's up to you. I agree that it is not this PR's fault.

No we can't. Because not everything has e.args[0][desc]. It's specific to this one ldap error, and we'll trigger an indexerror on everything else that uses e.message. It's a bug in pyldap's str/unicode for this error type. If we are going to fix it, we need to fix it in pyldap, not in our code to make it work correctly.

My point is that we can fix it at least for the cases where e.args[0][desc] exists.
As I said previously, there will be complains from users/customers.

So we can have some workaround which can be removed later (we can add the link to python-ldap issue in a comment for tracking purposes).
The option - check if e.args[0][desc] exists and print the content - str(e) otherwise - will cover most of the cases (I've never seen an ldap.ERROR which doesn't have it, actually). And for what's left - we can say our users that we've done everything and the rest of the solution is dependent on python-ldap.

There is only one case where e.args[0][desc] exists and it's that one ldap error. I'm really hesitant to write work arounds because then we'll never fix the original problem. We just mask over it.

I think we should accept this patch as is, because the error could happen in many other places, and you should report a bug with str/unicode python ldap to fix this.

There is only one case where e.args[0][desc] exists and it's that one ldap error. I'm really hesitant to write work arounds because then we'll never fix the original problem. We just mask over it.
I think we should accept this patch as is, because the error could happen in many other places, and you should report a bug with str/unicode python ldap to fix this.

IIRC, every ldap.LDAPError returns the e.args[0]['desc']...
For example, @vashirov fixed here - https://pagure.io/389-ds-base/c/af4631f

If we won't do the workaround I proposed, the users will eventually (and I think pretty soon) complain that WebUI and CLI have these weird formatted errors. And it will take a while because the writing, and merging, and waiting for the python-ldap build will take a few months.

I've done the same with another upstream issue - I've masked the problem and put the comment with the link to the upstream issue. When it was resolved - I changed the code back.

Maybe @mreynolds can have a fresh look on the issue and share his opinion too... :)

@spichugi Should this be a seperate issue then?

Yep, as I mentioned - https://pagure.io/389-ds-base/pull-request/50624#comment-97740

Also, it can be another PR. It's up to you. I agree that it is not this PR's fault.

You have my ack this one if so :)

Did you want to open the separate issue then?

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3679

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.