stefangeyer / challonge-java

Java bindings for the Challonge API
MIT License
10 stars 7 forks source link

Is there any way to update a match as a tie? #20

Open InklingOnFire opened 2 years ago

InklingOnFire commented 2 years ago

I'm currently working on a bot for a league system where, on some occasions, a tie is possible (in a round robin). However, i can only provide a Long for MatchQueryBuilder#winnerId, but the Challonge API insists on having a String of "tie" as winnerId (if a tie occurs.) Leaving the winnersId null or 0L will leave the match open and inputting something else than the participant's IDs will throw a DataAccessException.

I was looking in every Java file involved for some clues, but to no avail. Is there any way to update a match as a tie?

gpluscb commented 2 years ago

Hmm yeah as far as I can tell there is currently no way to do this. I'd be willing to pr a change later today or tomorrow.

I have tested a bit, and the api doesn't seem to recognise the id in String form (e.g. "1234") as a player id in this context. So I think it's probably easiest to special case null in MatchQuery.winnerId for that, but I'm not entirely sure how nicely the Lombok @Builder thing would play with that. Also because we'd have to add a new method to some interfaces probably, I think this would technically be considered a breaking change. Nevermind, that doesn't work so easily because not providing a winnerId is also valid. @stefangeyer what do you think about that idea?

Also a note on forward compatibility - challonge-java uses the v1 api currently which will be deprecated someday (which I didn't know until today). I don't know if there are plans to migrate this project to v2, so it might break in the future.

stefangeyer commented 2 years ago

Yeah, I seem to have overlooked this when writing the first implementation. The easiest fix I see would be to change winnerId in MatchQuery to Object and provide two separate builder methods for setting a Long as winnerId and setting the match as tied. This is easy to implement since Lombok builders can be extended as needed.

Something like this:

@Data
@Builder
public class MatchQuery {
    /**
     * winnerId can be either a long identifying the winner of the match or the string "tie" to declare the match as tied.
     **/
    private Object winnerId;
    private Integer votesForPlayer1;
    private Integer votesForPlayer2;
    private String scoresCsv;

    public static class MatchQueryBuilder {
        public MatchQueryBuilder winnerId(final Long winnerId) {
            this.winnerId = winnerId;
            return this;
        }
        public MatchQueryBuilder winnerTied() {
            this.winnerId = "tie";
            return this;
        }
    }
}

Any opinions on this solution? I also tested the API a little bit as I was unsure if tied matches would maybe return a string as winnerId as well, but it seems like this field just stays null in case of a tie.

stefangeyer commented 2 years ago

Also, the little red banner on the Challonge API page that states that certain features will be available in v2 of the API has been around for years. I don't see v2 of the API being released or the current version being deprecated any time soon. Any updates on that matter will probably be announced in the Google group, but as you can see the last update is from 2017 - so there is probably no hope.

gpluscb commented 2 years ago

Also, the little red banner on the Challonge API page that states that certain features will be available in v2 of the API has been around for years. I don't see v2 of the API being released or the current version being deprecated any time soon. Any updates on that matter will probably be announced in the Google group, but as you can see the last update is from 2017 - so there is probably no hope.

I'll look at the other comment in a bit, but the v2 I'm referring to is at https://challonge.com/de/connect, and the docs are at https://www.notion.so/Challonge-Connect-Documentation-ab016fd6b875474b9b67b9d8f8590497. They don't seem to have announced it in their changelogs or api changelogs, I only saw it because of a banner on the v1 documentation page.

gpluscb commented 2 years ago

Any opinions on this solution? I also tested the API a little bit as I was unsure if tied matches would maybe return a string as winnerId as well, but it seems like this field just stays null in case of a tie.

This would probably work well. A disadvantage would be that in MatchQueryAdapter, the String/Long check would probably have to occur anyway, and the String value wouldn't carry any extra information. But it could potentially be nicer for some custom adapters, and it could be more easily adaptable if Challonge changes something about this system in the future.

An alternative would be to add a boolean isTie field which flags the query as a tie if set to true. This approach also has the problem of redundant information, but it makes it impossible for invalid MatchQuery states to exist.

I don't know if either approach is strictly better here. But I think those are probably the best approaches for this problem.

stefangeyer commented 2 years ago

As an extra field would most definitely make more sense from a design perspective, having the winnerId containing both possible values represents the API payload more accurately which should avoid potential confusion when using it. Which is why I will go with that option I think.

I also didn't know about Challonge Connect. This will require a major rework and is probably a good opportunity to finally port this project to Kotlin. I probably won't have time to do this before fall, though.