Skip to content

794: Refactor Leaderboard#806

Open
alfardil wants to merge 9 commits intomainfrom
794
Open

794: Refactor Leaderboard#806
alfardil wants to merge 9 commits intomainfrom
794

Conversation

@alfardil
Copy link
Collaborator

@alfardil alfardil commented Feb 23, 2026

794

Description of changes

Refactored to use Optional for Leaderboard types

Checklist before review

  • I have done a thorough self-review of the PR
  • Copilot has reviewed my latest changes, and all comments have been fixed and/or closed.
  • If I have made database changes, I have made sure I followed all the db repo rules listed in the wiki here. (check if no db changes)
  • All tests have passed
  • I have successfully deployed this PR to staging
  • I have done manual QA in both dev (and staging if possible) and attached screenshots below.

Screenshots

Dev

All tests passed in dev
Screenshot 2026-02-23 at 3 06 39 PM
image
Screenshot 2026-03-01 at 11 46 18 PM
Screenshot 2026-03-01 at 11 46 46 PM

Staging

@github-actions
Copy link
Contributor

Available PR Commands

  • /ai - Triggers all AI review commands at once
  • /review - AI review of the PR changes
  • /describe - AI-powered description of the PR
  • /improve - AI-powered suggestions
  • /deploy - Deploy to staging

See: https://github.com/tahminator/codebloom/wiki/CI-Commands

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Unsafe Optional.get()

Several endpoints call .get() on Optionals from repository/manager without presence checks, which can throw and return 500 instead of a 404/empty response. Please guard with isPresent()/orElseThrow(ResponseStatusException) and update the null-checks accordingly.

Optional<Leaderboard> leaderboardData = leaderboardManager.getLeaderboardMetadata(
        leaderboardRepository.getRecentLeaderboardMetadata().get().getId());

return ResponseEntity.ok()
        .body(ApiResponder.success(
                "All leaderboards found!", LeaderboardDto.fromLeaderboard(leaderboardData.get())));
Unsafe Optional.get()

Discord notifications build paths/titles using currentLeaderboard.get() with no fallback if no active leaderboard exists. Add presence checks (and a graceful no-leaderboard behavior) to avoid NoSuchElementException in production jobs.

topUsersSection,
club.getName(),
serverUrlUtils.getUrl(),
currentLeaderboard.get().getId(),
club.getTag().name().toLowerCase(),
Optional Handling

Recent leaderboard is now Optional but code still assumes non-null and calls .get() directly. Replace the null check with isEmpty() and handle absence explicitly before dereferencing.

boolean isTooLate = recentLeaderboard.get().getCreatedAt().isAfter(leetcodeSubmission.getTimestamp());

@tahminator
Copy link
Owner

/ai

@tahminator
Copy link
Owner

/review

@tahminator
Copy link
Owner

/describe

@tahminator
Copy link
Owner

/improve

@github-actions
Copy link
Contributor

Preparing PR description...

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

Creating a new leaderboard now throws when there is no current leaderboard (using Optional.isEmpty → RuntimeException), which changes prior behavior and conflicts with tests expecting success when none exists. Treat absence as “no previous leaderboard” and proceed instead of throwing.

// BE VERY CAREFUL WITH THIS ROUTE. IT WILL DEACTIVATE THE PREVIOUS LEADERBOARD
// (however, it should be in a recoverable state, as it just gets toggled to be
// deactivated, not deleted).
Optional<Leaderboard> currentLeaderboard = leaderboardRepository.getRecentLeaderboardMetadata();

if (currentLeaderboard.isEmpty()) {
    throw new RuntimeException("No recent leaderboard found.");
}
discordClubManager.sendLeaderboardCompletedDiscordMessageToAllClubs();
leaderboardManager.generateAchievementsForAllWinners();
leaderboardRepository.disableLeaderboardById(currentLeaderboard.get().getId());
Optional Misuse

Unsafe Optional.get() usage on the current leaderboard can cause 500s when none exists (e.g., fetching current metadata and users). Check presence and return 404 or a typed error instead of calling get() directly; similarly avoid Optional.get() on ranked-user lookups and handle empty results.

        final HttpServletRequest request) {
    FakeLag.sleep(650);

    Optional<Leaderboard> leaderboardData = leaderboardManager.getLeaderboardMetadata(
            leaderboardRepository.getRecentLeaderboardMetadata().get().getId());

    if (leaderboardData.isEmpty()) {
        throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Leaderboard cannot be found or does not exist.");
    }

    return ResponseEntity.ok()
            .body(ApiResponder.success(
                    "All leaderboards found!", LeaderboardDto.fromLeaderboard(leaderboardData.get())));
}
Null Optional Risk

Optional fields lack default values, so builder-created instances may have null Optionals, causing NPEs where .orElse(...) is called (e.g., DTO mapping). Consider @Builder.Default with Optional.empty() or enforce non-null Optionals at construction.

    private LocalDateTime createdAt;

    @NullColumn
    private Optional<LocalDateTime> deletedAt;

    @NullColumn
    private Optional<LocalDateTime> shouldExpireBy;

    @NullColumn
    private Optional<String> syntaxHighlightingLanguage;
}

@alfardil
Copy link
Collaborator Author

/ai

@alfardil
Copy link
Collaborator Author

/review

@alfardil
Copy link
Collaborator Author

/describe

@alfardil
Copy link
Collaborator Author

/improve

@tahminator
Copy link
Owner

/review

@tahminator
Copy link
Owner

/describe

@tahminator
Copy link
Owner

/improve

@github-actions
Copy link
Contributor

Preparing PR description...

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Optional Defaults

Optional fields on the model can be null if not explicitly set, which will cause NPEs when calling methods like .orElse(...) or .isPresent(). Consider initializing Optional fields to Optional.empty() (e.g., with @Builder.Default) and remove null checks elsewhere for consistency.

    private Optional<LocalDateTime> deletedAt;

    @NullColumn
    private Optional<LocalDateTime> shouldExpireBy;

    @NullColumn
    private Optional<String> syntaxHighlightingLanguage;
}
Test Assertion Bug

Tests use assertNotNull(optional) on Optional results, which always passes even when empty. Replace with assertTrue(optional.isPresent()) (and adjust subsequent .get() calls) to avoid false positives and potential NoSuchElementException.

    assertNotNull(patinaResult, "User with Patina tag should appear in Patina filter");
    assertNotNull(baruchResult, "User with Baruch tag should appear in Baruch filter");
    assertEquals(
            patinaResult.get().getItem(),
            baruchResult.get().getItem(),
            "Same user should be returned in both filters");
}
Error Handling

Throwing RuntimeException when no active leaderboard is found (without logging) may crash background flows. Prefer logging with context and returning early, or using a controlled error path to avoid noisy failures in scheduled/notification workflows.

Optional<Leaderboard> currentLeaderboard = leaderboardRepository.getRecentLeaderboardMetadata();

if (currentLeaderboard.isEmpty()) {
    throw new RuntimeException("No recent leaderboard found.");
}

@alfardil alfardil force-pushed the 794 branch 3 times, most recently from e3854bd to cf80e11 Compare March 2, 2026 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants