3
\$\begingroup\$

I have a class in my application which manages storage of OAuth2 tokens. There are unauthenticated tokens (in my code, called "application access tokens") which can be used for API calls that do not require a user account, but when a user signs into the app, I receive an authentication code from which I retrieve an authenticated access token (in my code, called "user access token"), then use that to make all API requests, instead.

Initially I was using an event bus which was fed API requests, and presented them to an authentication proxy which checked the following flow:

  1. If there is an user access token, check if it has expired. If not expired, return it.
  2. If it expired, submit a request to refresh it.
  3. If the refresh is successful, return the token.
  4. If there is no refresh token, or no user access token, check if there is an application access token.
  5. If there is an application access token, check if it has expired. If not expired, return it.
  6. If it's expired, retrieve another one and return it.

This seemed like a good candidate for RxJava. I've been attempting to convert the class over to a more functional style over the last few weeks, and I think I have been learning a lot and making progress, but I can't help but think I could use a review from someone more experienced with Rx and coding in a functional style.

Interface:

public interface AccessTokenManager {
    boolean isUserAuthorized();
    boolean hasValidAccessToken();
    String getValidAccessToken();
    Observable<AccessToken> getUserAccessToken();
    Observable<AccessToken> getAccessToken();
    Action1<AccessToken> saveUserAccessToken();
    Action1<AccessToken> saveApplicationAccessToken();
    void clearSavedUserAccessToken();
    void clearSavedApplicationAccessToken();
}

Implementation:

I only included the getUserAccessToken and getAccessToken methods as the methods to save and clear are irrelevant to the flow I am currently refactoring. Here is the full class if desired.

@Override
public Observable<AccessToken> getAccessToken() {
    return mGetUserAccessToken.onErrorResumeNext(mGetApplicationAccessToken);
}

private Func1<AccessToken, Observable<AccessToken>> refreshUserAccessToken =
        accessToken -> {
            if (accessToken.secondsUntilExpiration() > EXPIRATION_THRESHOLD) {
                return Observable.just(accessToken);
            } else return refreshUserAccessToken(accessToken);
        };

private Func1<AccessToken, Observable<AccessToken>> refreshApplicationAccessToken =
        accessToken -> {
            if (accessToken != null
                    && accessToken.secondsUntilExpiration() > EXPIRATION_THRESHOLD) {
                return Observable.just(accessToken);
            } else {
                return mServiceAuth.authorizeApplication()
                        .map(responseToAccessToken())
                        .doOnNext(saveApplicationAccessToken());
            }
        };

private Observable<AccessToken> mGetUserAccessToken = Observable.create(subscriber -> {
    AccessToken token = getSavedUserAccessToken();
    if (token == null) {
        subscriber.onError(new RuntimeException("No user access token available"));
    } else {
        refreshUserAccessToken.call(token)
                .subscribe(subscriber::onNext, subscriber::onError, subscriber::onCompleted);
    }
});

private Observable<AccessToken> mGetApplicationAccessToken = Observable.create(subscriber -> {
    AccessToken token = getSavedApplicationAccessToken();
    if (token == null) {
        subscriber.onError(new RuntimeException("No access token available"));
    } else {
        refreshApplicationAccessToken.call(token)
                .subscribe(subscriber::onNext, subscriber::onError, subscriber::onCompleted);
    }
});

private Observable<AccessToken> refreshUserAccessToken(AccessToken accessToken) {
    String refreshToken = accessToken.getRefreshToken();
    if (refreshToken == null) {
        return Observable.error(new RuntimeException("No refresh token available"));
    }
    return mServiceAuth.refreshUserAccessToken(refreshToken)
            .map(responseToAccessToken())
            .doOnNext(saveUserAccessToken())
            .doOnError(error -> {
                clearSavedUserAccessToken();
                mIdentityManager.clearSavedUserIdentity();
            });
}

Firstly, I am not really a fan of defining Observable and Func as fields in the class, as fields are typically private, and therefore additionally require a getter to expose them to the public API. Also, fields cannot reference other fields declared later in the class, forcing me to declare them in reverse of the actual flow. An alternative I considered was to wrap them in a function which returns an instance of the Observable or Func each time it is called, but this is more expensive in both time and memory- certainly not worth it to simply make the code a bit more concise.

Also, I am using Observable.create for getUserAccessToken and getApplicationAccessToken. It feels a bit fragile using this technique, as it requires me to notify subscribers to the Observable myself. Originally, I had these implemented as regular functions which returned Observable (or Func0<Observable>s), but in doing this, getApplicationAccessToken() is evaluated during getUserAccessToken().onErrorResumeNext(getApplicationAccessToken())) even when the getUserAccessToken() does not encounter an error.

I hope I explained everything adequately. If anyone has any tips they can provide, they would be greatly appreciated.

\$\endgroup\$
2
\$\begingroup\$

A few small notes from skimming the code here:

  • Why does getValidAccessToken() return a String when you don't seem to have any qualms about exposing AccessTokens through the other methods?

  • You're repeating accessToken.secondsUntilExpiration() > EXPIRATION_THRESHOLD in quite some places. I'd consider extracting this into a separate method. isExpired(accessToken) is cleaner and more succinct. Also it allows you to change the mechanism of expiry without having to touch everywhere in your code.

  • Returning null-tokens from getters takes it's toll on your code. You could try to remedy this by enforcing there to be a token, defining a "NULL-Object" or (to get more functional) using an Optional (or Maybe) instead.

  • Sidenote about the initialization order of fields you mention: You should consider instead using the constructor to initialize your class. This should allow you to follow the conventional flow.
    Aside from that you may want to reduce coupling between fields as much as you can. Unfortunately it's a little hard to see how you would do that with the code you've given us.

  • mIdentityManager, mGetApplicationAccessToken and mGetUserAccessToken seem to be systems hungarian. Get rid of that, it provides no benefit whatsoever.

  • Functional code does not come from declaring methods as Func instances. If you're trying to write functional code in an Object-Oriented language such as Java, you'll have to make some concessions about first-class functions to stay idiomatic.

Overall the code looks nice, but has some small rough edges here and there. This is the result of you trying too hard to follow functional paradigms when it's not fully supported to do so.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.