Github commands package crashes all bot features if github key is not configured

[ankitsmt211]

Describe the bug

Ideally when setting up bot, it should work with copy paste of config.template and bot token only. But atm, without entering githubAPI key, none of the other features work.

The root cause seems to be contructors of both GithubCommand.java and GithubReference.java, both calls two methods updateCache() and acquireRespositories(), but since we havent configured key yet this ends up crashing all other features.

Expected behavior

Other features except /github-search and GithubReference should work without githubAPI key.

To Reproduce

1. Clone Bot.
2. Copy config.json.template and create a new file inside application folder named config.json.
3. Add discord bot token.
4. When bot is live, use /ping or any other feature.

Additional context

constructor of GithubCommand

constructor of GithubReference

[ankitsmt211]

@marko-radosavljevic The issue i already mentioned if you wanna have a look.

[marko-radosavljevic]

That's indeed a problem, even if we disable this feature, it would still crash. Because object creation happens before we filter disabled features.

The most obvious solution seems moving these out of the constructor. So, for the first class, the only user of that method updateCache() is onAutoComplete(), and there it is already called. So maybe update cache at the start of the method (in a way that would run it the first time it is called as well).

In the similar fashion for the other class, you can do that invocation much latter, when findIssue() needs it. Since there are two different entries, check if it's already populated.

And then the feature can be disabled, for people that do not care about that feature or don't have a key. And doesn't crash automatically, even when dsiabled.

Also, if there is a bad key, or default "\<insert your key here>", it should throw a friendly exception that tells the user exactly where is the problem. "The github key ({}) used in the config is invalid, these features .. will not work."

---

Because another issue is that our config is getting more complicated, and requires a ton of different stuff. If you don't have the key, are you supposed to leave it empty or write null? You do that, and you get NPE, whole thing crashes, alright. They told me I can use dummy data, but when I use foo it crashes as invalid URL IllegalArgumentException: Failed to parse webhook URL. Alright so webhook needs dummy data, but not too dummy, it needs to be an actual url, that doesn't resolve... etc

So the purpose of our template was to make it easy to start the bot, it's already populated with everything you need to get started, so the bot doesn't crash.

Here is a funny help thread. With me that never used the config and these features, but somewhat followed the development, thinking it should be simple enough. And also zabu, that actually wrote that feature, but forgot the specific mechanism. We are project maintainers, and we ran in circles, trying to help a beginner that is setting up his project. ^^ Link of the discord help thread.

Moral of the story is that config should be simple, consistent, and documented.

[ankitsmt211]

@marko-radosavljevic i found a way to solve this without moving methods out of constructor since if you're not configuring those keys there's a solid chance you are not going to use it anyway.

  try{
            GitHubReference githubReference = new GitHubReference(config);
            GitHubCommand gitHubCommand = new GitHubCommand(githubReference);
            features.add(githubReference);
            features.add(gitHubCommand);
        }
        catch (Exception exception){
            LoggerFactory.getLogger(Features.class).error("github key is not configured, feature will not work",exception);
        }

 This ofc is a dirty snippet sitting in `Features.java` atm, Any suggestions?

Maybe optional features should go in seperate file instead of Features.java?

[Tais993]

This goes back to the fact that none of the features have a way to be disabled universally.

A global system should be added so optional features have to be enabled in the config manually before actually being enabled / or they disable automatically when lacking the valid information or when empty.

We talked before about this, and I don't remember what was decided in the end.

[ankitsmt211]

@Tais993 i think you're referring to this PR which is already merged. https://github.com/Together-Java/TJ-Bot/pull/886

[marko-radosavljevic]

Yeah, the issue is that the feature object creation happens before adding them to the blacklist. And calls to APIs that are not specified in config happen during construction. So blacklisted or not, this crashes.

https://github.com/Together-Java/TJ-Bot/blob/214882178d598408a496358a8452122d0a8aa296/application/src/main/java/org/togetherjava/tjbot/features/Features.java#L164-L170

Probably the simplest and most graceful solution for now is just not to call any APIs specified in config during construction, but when it's actually used/needed, like every other feature. If that is not possible for some technical reason, you can do your try catch, but in classes where the API is actually called. With some nice, and user-friendly explanation, that the API key is not provided in the config, and thus the feature is disabled.

And if this problem happens again, we can think of redesigning disabling features and config.