#2346 automember should use separate attribute for error message
Closed: wontfix 5 years ago Opened 12 years ago by edewata.

Currently the automember-default-group-show command uses the same automemberdefaultgroup attribute to return two different results.

When there's a default group the attribute contains the DN of the group:

{
    ...
    "result": {
        "result": {
            "automemberdefaultgroup": [
                "cn=ipausers,cn=groups,cn=accounts,dc=example,dc=com"
            ], 
            ...
        }, 
        ...
    }, 
    ...

When there's no default group the attribute contains an error message:

{
    ...
    "result": {
        "result": {
            "automemberdefaultgroup": "No default group set", 
            ...
        }, 
        ...
    }, 
    ...
}

Sharing the same attribute makes it more difficult for client applications (including the UI) to distinguish the two. The client could check whether the value is in DN format, but this function is not available in standard JavaScript. Another possibility is to compare the value with a string constant, which is not reliable because the message could change or be translated.

It would be better not to return automemberdefaultgroup when there is no default group and to use a separate attribute to return the error message.

For example:

{
    "error": {
        "code": ..., 
        "message": "No default group set",
        ...
    },
    "result": {
        "result": {
            ...
        }, 
        ...
    }, 
    ...
}

The reason for this is this is essentially a NotFound error but we're suppressing that and returning it as a non-error.

If we go ahead and raise an error then it isn't so nice on the command-line:

$ ipa automember-default-group-show --type=group
ipa: ERROR: No default group set

I can set a fake attribute, nodefault=True/False, as an alternative.

Petr, I'm interested on your take on this. Having no default is not really an error, it just is. Would setting a new virtual attribute in the response be adequate? I guess the issue is you don't know whether to try to link this to a group?

In short: I'm not for virtual attribute, but it can work with it.

Longer version:

How it should be (IMHO)

The problem isn't that we need to change to output in order to UI to work. It works now, but it had to adapt (we are testing if group looks like dn or not). Problem is that current behaviour is inconsistent with the rest of the API - as Endi wrote. It is OK to have no default group. Standard way is to not include the attribute in response - this says no group set.

If I understand correctly, the message is there for the CLI and you want it to stay that way (thin client...). IMO server should not differ from its standards because of client app (CLI or Web UI). Also it is kinda inconsistent to send dn instead of just plain group name. It is necessary to distinguish from the error message though.

If we want to show some message in CLI and the message should not be error, we may consider something like:

{
    "info": {
        "code": ..., 
        "message": "No default group set",
        ...
    },
    "result": {
        "result": {
            ...
        }, 
        ...
    }, 
    ...
}

And the following CLI output will be:

# INFO: No default group set.

the 'INFO' label may be some more appropriate string. Or the info part in result may be error with code indication that it is info. This code would change CLI output from 'ERROR:...' to 'INFO:...'

If default group is set the correct output will be (notice: no dn):

{
    ...
    "result": {
        "result": {
            "automemberdefaultgroup": [
                "foogroup"
            ], 
            ...
        }, 
        ...
    }, 
    ...
}

New virtual attribute

It would make UI logic more simple but it would be still inconsistent.

In either case, UI would have to be modified.

How about raising a new exception? That would work on the CLI, but what about the UI, would you pop up a dialog?

UI would pop up a dialog. I like the summary thing which you mentioned on IRC. I think that my info.message proposal is basically what the summary does.

Replying to [comment:5 pvoborni]:

...

== How it should be (IMHO) ==

The problem isn't that we need to change to output in order to UI to work. It works now, but it had to adapt (we are testing if group looks like dn or not). Problem is that current behaviour is inconsistent with the rest of the API - as Endi wrote. It is OK to have no default group. Standard way is to not include the attribute in response - this says no group set.

If I understand correctly, the message is there for the CLI and you want it to stay that way (thin client...). IMO server should not differ from its standards because of client app (CLI or Web UI). Also it is kinda inconsistent to send dn instead of just plain group name. It is necessary to distinguish from the error message though.

If we want to show some message in CLI and the message should not be error, we may consider something like:

{{{

{
"info": {
"code": ...,
"message": "No default group set",
...
},
"result": {
"result": {
...
},
...
},
...
}
}}}

And the following CLI output will be:

{{{

INFO: No default group set.

}}}
the 'INFO' label may be some more appropriate string. Or the info part in result may be error with code indication that it is info. This code would change CLI output from 'ERROR:...' to 'INFO:...'

...

I like this solution, but I think it could be improved by allowing multiple messages of different severities (DEBUG, INFO, WARNING, etc.) in the reply:

{
    "messages": [
        {
            "severity": ...,
            "code": ..., 
            "message": "No default group set",
            ...
        },
        ...
    ],
    "result": {
        "result": {
            ...
        }, 
        ...
    }, 
    ...
}

I can't use a dynamic msg_summary because it is read-only once the server starts.

Returning a whole new class of data would require a major change in the output of the function which is beyond the scope of 2.2.

On the cli the output would look strange, unfortunately. We have the same problem with dnsconfig where dnsconfig-show returns nothing when there is no global config.

If I return nothing it looks like:
$ ipa automember-default-group-show --type=group
$

Or I can raise an exception, but as discussed you might need special handling to catch and handle this w/o a dialog:
$ ipa automember-default-group-show --type=group
ipa: ERROR: No default group set

Or we can leave it as-is since you are already working around it.

It works in CLI and Web UI so I think it is not a priority. There is no point to change it to some other workaround. So I would leave it as is for 2.2. It can be done right sometime later when there aren't more important issues.

May a backward compatibility be a problem if we decide to do it later?

This is already out there so I think I don't think it will introduce any new problems.

Moving to 3.1 for now.

Metadata Update from @edewata:
- Issue assigned to rcritten
- Issue set to the milestone: Ticket Backlog

7 years ago

Thank you taking time to submit this request for FreeIPA. Unfortunately this bug was not given priority and the team lacks the capacity to work on it at this time.

Given that we are unable to fulfil this request I am closing the issue as wontfix. To request re-consideration of this decision please reopen this issue and provide additional technical details about its importance to you.

Metadata Update from @rcritten:
- Issue close_status updated to: wontfix
- Issue status updated to: Closed (was: Open)

5 years ago

Login to comment on this ticket.

Metadata