Enhance structure to increase visibility of Account entity variants

[bocops]

Some of our account-related methods, namely verifyCredentials() and updateCredentials(), return a CredentialAccount instead of an Account since the last snapshot release.

In our code, these entities are completely separate out of necessity (inheritance and data classes don't mix well), although Mastodon does not make this distinction. While some parts of the documentation specifically mention a "CredentialAccount" entity, others refer to the returned data as "an Account entity, with the source parameter included" (example: https://docs.joinmastodon.org/client/authorized/).

There doesn't seem to be any other way to retrieve the user's own account other than using verifyCredentials(), so I'd argue that this function is really meant to be used that way, and we either shouldn't return a completely different entity, or at least offer some way of getting an Account from the returned CredentialAccount. Potential ways to do this include:

• add a nullable source to Account and get rid of the separate CredentialAccount
• add a separate function (e.g. getOwnAccount()) that internally still calls verifyCredentials() but returns data as an Account (or possibly Account? if credentials can not be verified)
• add a utility function that accepts a CredentialAccount and returns an Account

[PattaFeuFeu]

@bocops How do you feel about having CredentialAccount extend Account? You write “inheritance and data classes don't mix well”, but which part specifically do you mean?

[bocops]

Extending data classes is not possible in Kotlin, so we couldn't for example declare data class CredentialAccount(...) : Account(...) and then later just cast to get the user's own account as a simple Account.

[PattaFeuFeu]

Ah, wow, I had totally forgotten about that again. 😕

How do you feel about this approach? Not great, but could be an option?

[bocops]

If we do that, and considering that we (currently) use both Account and CredentialAccount as entities, wouldn't we need to do something like this?

abstract class AccountData {
    // ....
}

data class Account () : AccountData()

data class CredentialAccount(
    // additional data
) : AccountData()

And if we do that, it would still not be trivial to get an Account if the returned entity is a CredentialAccount, and vice versa.

For what it's worth, my own use case is this:

/**
 * Get account data for a currently signed in user.
 */
fun getUserData() : Account? {
    var account: Account? = null
    client?.let {
        try {
            account = it.accounts.verifyCredentials().execute() // no longer compiles
        } catch (b: BigBoneRequestException) {
            Log.d("Client", "Exception verifying credentials: ${b.message}; ${b.httpStatusCode}")
        }
    }
    return account
}

I want to use the user's account data just like I would use the account data of any other user the client might see, and verifyCredentials() despite its name just seems to be the natural way to get that information. I could use getAccount(id), but then I would need to retrieve the user's id first, so would need to use verifyCredentials() anyway. Maybe Mastodon offers some other way that I'm not seeing, but at the moment their "verification" endpoint just seems to be a misnamed "user account" endpoint. ;)

[bocops]

Similar to the above, I just found out that there's another Account detail we're not handling properly: currently, our method to retrieve accounts muted by the user, MuteMethods.getMutes(range) returns a Pageable<Account> in line with Mastodon's documentation of that endpoint.

However, Mastodon's documentation of the Account entity describes a sub-entity MutedAccount with an additional attribute mute_expires_at that is not present for the generic Account entity.

Just like in the CredentialAccount case, we need to decide whether we want an additional entity, or if we just want to add these additional attributes as nullable to the main Account.

There are pros and cons in either case. Having just one Account entity for everything would spare us and our users some amount of casting between different sub-types - but at the same time may lead to the same instance of an Account entity having different interpretations. For example, mute_expires_at=null might mean that the account is indefinitely muted if it was retrieved from the mutes endpoint - but it would also be null for all accounts retrieved from other endpoints.

At the moment, I'm leaning towards creating different entities, but adding functions like CredentialAccount.getAccount() and MutedAccount.getAccount() that return standard Account.

[PattaFeuFeu]

I agree with your reasoning.

One other option could be a wrapper type for mute_expires_at which clarifies that null means “there’s no data here” and a type value that clarifies that the current value means “indefinitely muted”. But that also doesn’t scale well as we’d need that for every such property.

[bocops]

Good idea. Turns out, we might already have that wrapper type in this specific instance. :)

mute_expires_at is supposed to be a datetime string, which we typically parse via our PrecisionDateTime interface with various implementing classes. So, what we could do here, instead of creating a separate MutedAccount, is to add that attribute to Account along the following lines:

• muteExpiresAt is Unavailable: the account object was retrieved from an endpoint other than getMutes, and we don't know whether the account is muted
• muteExpiresAt is ExactTime: the account is muted, and the mute will expire at the given datetime
• muteExpiresAt is Indefinite: the account is muted, and the mute will never expire

In this case, Indefinite would be a new sub-class of either ValidPrecisionDateTime or InvalidPrecisionDateTime - whatever makes most sense.

At the moment, I'm not quite sure how to parse a null value differently depending on the endpoint we're retrieving it from, but perhaps there is a way that is not too cumbersome.

[bocops]

After some more consideration, I take back the above. We should not complicate one class containing some straight-forward information (is a datetime string exact, inexact or not available?) for a completely different purpose, which is the information whether an account is muted or not.

At the same time, I am still not completely happy with the idea of having three completely unrelated data classes for basically the same entity (an account on a Mastodon instance) with or without some extra details.

One option that hasn't been brought up yet, although we discussed inheritance earlier, is to simply make Account a proper class instead of data class. We would have to take care of equals() and hashCode() ourselves, but that shouldn't be too difficult considering we have a unique-per-instance id value.

[bocops]

The PR I just added fixes the missing mute_expires_at attribute when getting muted accounts by introducing a new MutedAccount entity separate from Account, similar to the already existing CredentialAccount.

This means a good amount of code duplication, but anything I could come up with in terms of inheritance just shifted this around instead of outright removing it. Unless I was looking at it the wrong way, the only sensible options seem to be either this, or moving all additional CredentialAccount/MutedAccount attributes into Account as optional attributes, which might come with its own set of problems as discussed above.

I'd be content with this either way, closing this or discussing further.