Design improvements for FileSupplier.java

[sina-golesorkhi]

Imagine that you have a class like following:

@Configuration
public class StupsOAuth2RestBean {

    private static final String UID_SCOPE = "uid";

    @Value("${tokens.accessTokenUri}")
    private String accessTokenURL;

    @Value("${tokens.configuration.tenant_service}")
    private String tokensConfigurationTemplateService;

    @Value("${tokens.configuration.tenant_service.scope}")
    private String tokensConfigurationTemplateServiceScope;

    @Bean
    public StupsOAuth2RestTemplate getInstance() {
        return new StupsOAuth2RestTemplate(new StupsTokensAccessTokenProvider(tokensConfigurationTemplateService,
                    getAccessTokens()));
    }

    @VisibleForTesting
    void setAccessTokenURL(final String accessTokenURL) {
        this.accessTokenURL = accessTokenURL;
    }

    @VisibleForTesting
    void setTokensConfigurationTemplateService(final String tokensConfigurationTemplateService) {
        this.tokensConfigurationTemplateService = tokensConfigurationTemplateService;
    }

    @VisibleForTesting
    void setTokensConfigurationTemplateServiceScope(final String tokensConfigurationTemplateServiceScope) {
        this.tokensConfigurationTemplateServiceScope = tokensConfigurationTemplateServiceScope;
    }

    private AccessTokens getAccessTokens() {
        try {
            //J-
            return Tokens.createAccessTokensWithUri(new URI(accessTokenURL))
                      .manageToken(tokensConfigurationTemplateService)
                      .addScope(tokensConfigurationTemplateServiceScope)
                      .addScope(UID_SCOPE).done().start();
            //J+
        } catch (final URISyntaxException e) {
            throw new RuntimeException(e);
        }
    }
}

And the following unit test:

@RunWith(MockitoJUnitRunner.class)
public class StupsOAuth2RestBeanTest {

    @Mock
    private AccessTokenProvider accessTokenProvider;

    @Test
    public void getInstance_returnsNewInstanceOfStupsOAuth() throws Exception {
        final StupsOAuth2RestBean stupsBean = new StupsOAuth2RestBean();
        stupsBean.setAccessTokenURL("accessTokenURL");
        stupsBean.setTokensConfigurationTemplateService("tokensConfigurationTemplateService");
        stupsBean.setTokensConfigurationTemplateServiceScope("tokensConfigurationTemplateServiceScope");
        assertNotNull(stupsBean.getInstance());
    }
}

The test will pass but still you will get an exception as following.

org.zalando.stups.tokens.CredentialsUnavailableException: environment variable CREDENTIALS_DIR not set
.... 

Caused by: java.lang.IllegalStateException: environment variable CREDENTIALS_DIR not set
    at org.zalando.stups.tokens.FileSupplier.getCredentialsDir(FileSupplier.java:57)
    at org.zalando.stups.tokens.FileSupplier.get(FileSupplier.java:45)
    at org.zalando.stups.tokens.AbstractJsonFileBackedCredentialsProvider.getFile(AbstractJsonFileBackedCredentialsProvider.java:36)
    at org.zalando.stups.tokens.AbstractJsonFileBackedCredentialsProvider.read(AbstractJsonFileBackedCredentialsProvider.java:41)
    ... 33 common frames omitted

For the client code StupsOAuth2RestBean class there is no way of knowing about this missing configuration from outside and without looking into the source code I could not figure out that I have to configure environment variable because on line 51 and 55 of the FileSupplier.java this is read as env variable.

So my suggestions would be to document it so that it becomes more clear what this class is needing or the Java solution instead would be to read this value from outside (IoC) either by constructor or setters instead of static call to System class so that the client code knows about this and is forced to provide this in the compile time instead of reading from system environment (even this option can be provided if you want to read and write stuff in a static way using system environments).

• the method org.zalando.stups.tokens.AccessTokenRefresher.run() is catching all throwables and swallowing them with only warn. That's why my test still passes but I get an stacktrace with WARN which doesn't seem to be reflect the expected behavior.

[jbellmann]

@sina-golesorkhi thanks for reporting the problem, we will discuss this problem in the team.

First thing we will do, is to improve documentation.

[midumitrescu]

Hello everybody,

I am working a small library that can be used for testing purposes.

Basically I will create a @rule that can be integrated in an JUnit Test for creating at test execution time all required dependencies.

Please have a little bit of more patience.

Best regards,
Mihai

[harti2006]

@sina-golesorkhi I like the idea of providing configurable properties via constructor or setter, but to be backwards-compatible and easy-to-use, we should populate these attributes from the environment.

I would also suggest a fail-fast approach: A bad configuration should immediately throw a runtime exception.

[sina-golesorkhi]

@harti2006 I'm not quite getting why providing a configuration from outside and doing inversion of control would add complexity to the library. Actually by providing those options you are telling your client in compile time what you do need (specially with the constructor). If this was an optional configuration I could follow your argument but right now the library is throwing an exception which is logged but swallowed. To me if a code needs a configuration from me it should tell me in the compile that what is needed and reading stuff statically from a central place (e.g. System) is generally a bad idea because you are hiding it from the client thought that you want him provide you with that.

Regarding the fail-fast: As the client I would be happy to have that but such that this exception is bubbled up through the whole stack so that the client also gets the exception, though as I said in the previous paragraph by that time (runtime) the client wouldn't also be that much happy because the API didn't tell him (in compile time) that this exception is going to be thrown if one specific property is not configured/provided.

[jbellmann]

@sina-golesorkhi

because of the code you used in your comment. If you are using Spring-Boot, are you aware of Spring-Boot support for 'tokens'?

It does not help to solve this concrete issue, but you don't have to write such custom-configurations as above and 'AccessTokens' will be available as injectable bean.

[sina-golesorkhi]

@jbellmann I was not aware of this other project. Thank you for naming it I will have a look at it but as you also mentioned it is out of the scope of this issue because this issue is addressing a design improvement regarding this code base and one client might not want yet another dependency to another library;)

[lasomethingsomething]

@sina-golesorkhi @jbellmann @duergner @harti2006 @midumitrescu Hey, can we close this issue? It dates back to April 2016.

Or: We could add labels to suggest "enhancement" or "Help wanted" or both. Your call.

[duergner]

Wouldn't close it - but Help wanted sounds definitely like a good idea.

[harti2006]

@jbellmann and me were thinking about fixing the issue with the mandatory environment variable, to prepare the 1.0.0 version of this library.
@whiskeysierra what do you think?

[whiskeysierra]

What's the plan exactly? Nullable constructor parameter that fallsback to environment variable?

[harti2006]

I'm not sure. There are some cases (fixed test access tokens, or the test case above), where people don't want to worry about files and a credentials_dir at all. We need to provide an easy (and documented way) to turn the entire feature off.

[whiskeysierra]

I assume we are using semantic versioning, right? Since we are still at 0.x.y we can break the behaviour if we decide that it's worth the benefit. The next possibility to clean it up would be the next major version bump to 2.x which I'd guess is not on the immediate roadmap 😜.

[duergner]

What about a constructor which does exactly this, i.e. turn off the feature? Should be the most obvious approach, right?

[vadeg]

I've created PR with one of the solutions.
In short: check CREDENTIALS_DIR existence before checking files on it.

[duergner]

But that does not solve the underlying "problem" in cases we don't want to rely on CREDENTIALS_DIR right?

[vadeg]

@duergner Yes. It solves the problem when you want to use AccessTokenRefresher but still see an error environment variable CREDENTIALS_DIR not set what is not correct.

If we don't want to rely on CREDENTIALS_DIR we can for example add a new method AccessTokensBuilder.withCredentialsDir(...). That will force to use FilesystemSecretRefresher instead of AccessTokenRefresher. There can be many cases.

[sina-golesorkhi]

I just wanted to rephrase the issue that I saw a in my code as the client of this library that lead to create this ticket. There is a magic happening in this code which is the access to CREDENTIALS_DIR statically which is needed for this library to work but is hidden from the client point of view. This can be done by providing a method like @vadeg said (IoC) but I will even go further and make it mandatory and part of the constructor because without this value the code won't work so being part of the builder still makes it optional and doesn't signal that it has to be there.

[duergner]

@sina-golesorkhi do you think the proposal regarding having two constructors where one has the explicit mentioning and the other falling back to the current behaviour would make sense? We would not break backward compatibility (we could mark the one without arguments as deprecated) but it should become more obvious.

[sina-golesorkhi]

@duergner sure. That would be the best option :)

[vadeg]

@duergner I was thinking that CREDENTIALS_DIR is an optional feature. But now I see that library would not work without CREDENTIALS_DIR at all.
For example if there is no CREDENTIALS_DIR set that means that AccessTokenRefresher (remote tokens fetch) will be used instead of FilesystemSecretRefresher (local tokens refresh). But JsonFileBackedClientCredentialsProvider and JsonFileBackedUserCredentialsProvider will be used by default for parsing client.json and user.json. Obviously, they will fail to parse these files because there is no CREDENTIALS_DIR.