JSON-RPC: jamulusserver/getServerProfile: Add directory address

[hoffie]

Has this feature been discussed and generally agreed?

No.

Describe the solution you'd like

Raised by @softins in #1975 (review)

jamulusserver/getServerProfile reports whether the server is registered to a directory, but not to which directory.

Describe alternatives that have been considered

/

[ann0see]

I'm not that familiar with the code, but it should be an easy feature addition. @dtinth @Rob-NY @softins anyone wanting to work on it?

[Rob-NY]

I have it working. Will post a PR. I simply add a "directoryServer" key to the response and echo back whatever was passed on the command line as that is what is stored in the ServerList object.

[Rob-NY]

Also, what do you think about changing a few of the method names? For example jamulusserver/getServerProfile should just be getProfile. Same with setServerName (should just be jamulusserver/setName). There's some redundancy in the naming given that they are 'namespaced'. The jamulusclient/* method names seem fine.

[dtinth]

@Rob-NY Making it shorter makes sense to me. I did not spend any effort in naming methods properly, and since the RPC is still experimental, we can rename the method.

[ann0see]

Yes. I‘d split this in two PRs

[pljones]

I have it working. Will post a PR. I simply add a "directoryServer" key to the response and echo back whatever was passed on the command line as that is what is stored in the ServerList object.

What if the server isn't running headless and nothing was passed on the command line? What if it was changed over JSON-RPC?

All state should be retrieved from the (client or) server itself - don't trust anything that happens to by lying around outside that, as it could be stale.

[dtinth]

Might be a good idea to get it from Server.GetDirectoryAddress.

jamulus/src/server.h

Line 227 in 259545f

QString GetDirectoryAddress() { return ServerListManager.GetDirectoryAddress(); }

[Rob-NY]

@dtinth - Yes, that's exactly what I did. @pljones my command line comment was misleading, sorry. My (poorly made) point was that it will return the directory server that is/was the desired directory server -- not necessarily that the server is actually registered with it; for which you could examine the 'registrationStatus' property of the response.

This is a subtle point, but @hoffie 's original post here was looking for the directory server to which the server was registered. To deliver on that specifically, the DS name would only be populated if the Status was "registered". However, I thought it might be more useful to include the "target" DS alongside the status and let the RPC consumer figure out how they want to use the information.

[Rob-NY]

Sadly, GetDirectoryAddress() (edit: in server.cpp) does not get updated if a user chooses one of the official directories via the UI - this only returns the custom directory server address (which is always populated when headless).

Do you think RPC consumers would rather see the directory TYPE and let them convert that to one of the standard directories, OR should the RPC code convert the type to the directory address:port for them? The way the standard directories are implemented is somewhat interesting.

Choices:

"directoryServerType" : ENUM VALUE,
"directoryCustomServer" : String or blank and depends on Type

or

"directoryServer" : Always an address:port even if a standard DS or blank if none.

edit: This comment is moot. The confusing issue is that GetDirectoryAddress() exists as a method in both server.cpp and NetworkUtils -- and they do different things. See comment below.

[Rob-NY]

This approach seems Jamu-listic and accounts for the fact that choosing "None" actually defaults to anygenre1 behind the scenes so we need to adjust. Does this work?

        QString dsName = "";

        if ( AT_NONE != pServer->GetDirectoryType() )
            dsName = NetworkUtil::GetDirectoryAddress ( pServer->GetDirectoryType(), pServer->GetDirectoryAddress() );

        QJsonObject result{
            { "name", pServer->GetServerName() },
            { "city", pServer->GetServerCity() },
            { "countryId", pServer->GetServerCountry() },
            { "welcomeMessage", pServer->GetWelcomeMessage() },
            { "directoryServer", dsName },
            { "registrationStatus", SerializeRegistrationStatus ( pServer->GetSvrRegStatus() ) },
        };
        response["result"] = result;

[ann0see]

"None" actually defaults to anygenre1 behind the scenes so we need to adjust.

I thought @softins at least worked on changing that on GUI level?

[Rob-NY]

It probably doesn't matter for the rest of the code base. The issue for RPC is that in GetDirectoryAddress (utils.cpp), the switch 'default' catches types of NONE and AT_DEFAULT and returns the DEFAULT_SERVER (which is anygenre1). So the condition above is needed to differentiate between those choices which have different meanings in the RPC response.

There were other conditional options I suppose, but this seemed the most readable. The point is that I can't just call NetworkUtils::GetDirectoryAddress() blindly because I'll get back anygenre1 even if None is the chosen directory server.

From util.h:

// Directory type --------------------------------------------------------------
enum EDirectoryType
{
    // used for settings -> enum values should be fixed
    AT_NONE                 = -1, // means not registered, "invalid value"
    AT_DEFAULT              = 0,
    AT_ANY_GENRE2           = 1,
    AT_ANY_GENRE3           = 2,
    AT_GENRE_ROCK           = 3,
    AT_GENRE_JAZZ           = 4,
    AT_GENRE_CLASSICAL_FOLK = 5,
    AT_GENRE_CHORAL         = 6,
    AT_CUSTOM               = 7 // Must be the last entry!
};

From util.cpp:

QString NetworkUtil::GetDirectoryAddress ( const EDirectoryType eDirectoryType, const QString& strDirectoryAddress )
{
    switch ( eDirectoryType )
    {
    case AT_CUSTOM:
        return strDirectoryAddress;
    case AT_ANY_GENRE2:
        return CENTSERV_ANY_GENRE2;
    case AT_ANY_GENRE3:
        return CENTSERV_ANY_GENRE3;
    case AT_GENRE_ROCK:
        return CENTSERV_GENRE_ROCK;
    case AT_GENRE_JAZZ:
        return CENTSERV_GENRE_JAZZ;
    case AT_GENRE_CLASSICAL_FOLK:
        return CENTSERV_GENRE_CLASSICAL_FOLK;
    case AT_GENRE_CHORAL:
        return CENTSERV_GENRE_CHORAL;
    default:
        return DEFAULT_SERVER_ADDRESS; // AT_DEFAULT
    }
}

[pljones]

If directoryType is AT_NONE, then GetDirectoryAddress MUST NOT be used. The result of calling it is UNDEFINED (yes, the code simply returns default server address - but that could change, it's not defined in the case of AT_NONE).

Ideally you'd return directory type and ONLY if it's AT_CUSTOM return the address.

[Rob-NY]

That precisely what my suggested code does, above, with respect to adding directoryServer name to the RPC response requested here.

[softins]

If directoryType is AT_NONE, then GetDirectoryAddress MUST NOT be used. The result of calling it is UNDEFINED (yes, the code simply returns default server address - but that could change, it's not defined in the case of AT_NONE).

We should define it, so that AT_NONE returns something sensible to indicate it, such as nullptr, QString() or "". And of course make sure that everywhere that uses the return value still behaves sensibly in that case.

[pljones]

... returns something sensible to indicate it ...

Guess so - whilst the method shouldn't be called, calling it should therefore return an invalid result rather than the wrong value. Any value that won't get you AT_CUSTOM if you try to set it will do -- and as it's a string, you get AT_CUSTOM pretty much for any string, I think.