Prevent disabled configured vote sites from being auto-enabled - #1546
Prevent disabled configured vote sites from being auto-enabled#1546BenCodez wants to merge 17 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ae6645c76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (configuredSite.equalsIgnoreCase(siteName)) { | ||
| return true; |
There was a problem hiding this comment.
When an offline vote was queued before its site was disabled, hasVoteSite() now returns true for that disabled configuration, but checkOfflineVotes() subsequently calls getVoteSite(voteSiteName, true), which returns null because the site is not enabled, and passes it to playerVote(), where it is dereferenced. This makes player login processing throw instead of safely skipping the disabled site's queued vote; either distinguish configured existence from enabled availability or add a null check at that caller.
Useful? React with 👍 / 👎.
|
@codex address the unresolved P2 review feedback. Fix the disabled-configured vote-site path so offline queued votes for a site disabled after queueing are skipped safely without passing null into playerVote(), while preserving the intended prevention of auto-creation/re-enabling. Add or update regression coverage for player-login/offline-vote processing and run the relevant checks. |
Summary
Testing
|
|
Implemented the Codex finding directly: offline queued votes now resolve the enabled site once, skip disabled sites safely, and never pass null into playerVote(). @codex review the new commit. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef79399cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if (!checkEnabled) { | ||
| ArrayList<String> configuredSites = plugin.getConfigVoteSites().getVoteSitesNames(false); |
There was a problem hiding this comment.
When a disabled section has an empty or missing ServiceSite, getVoteSitesNames(false) still removes it because ConfigVoteSites.getVoteSitesNames applies siteCheck regardless of checkEnabled. If Votifier reports an identifier matching that section's key while AutoCreateVoteSites is enabled, this lookup falls through, hasVoteSite returns false, and VotiferEvent calls tryGenerateVoteSite, which writes Enabled: true into the same section. Enumerate the raw configured section keys here rather than the validated/loadable site list.
Useful? React with 👍 / 👎.
| return false; | ||
| } | ||
|
|
||
| ArrayList<String> configuredSites = plugin.getConfigVoteSites().getVoteSitesNames(false); |
There was a problem hiding this comment.
When any configured site has a missing VoteURL or invalid ServiceSite, each hasVoteSite call now runs getVoteSitesNames(false) once through getVoteSiteName and again here; that method invokes siteCheck, which logs configuration warnings. Votifier processing calls these lookups for every incoming vote, so a static configuration warning is emitted repeatedly per vote and can flood production logs. Use a non-validating configured-key lookup or cache the configured aliases instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2865c2467
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String serviceSite = plugin.getConfigVoteSites().getServiceSite(siteName); | ||
| String displayName = plugin.getConfigVoteSites().getDisplayName(siteName); |
There was a problem hiding this comment.
When VoteSites contains a scalar entry such as Broken: true, getRawVoteSiteNames() includes that key, but getServiceSite() calls getData(siteName).getString(...) after getData() returns null for non-sections. Consequently, every Votifier lookup through getVoteSiteName(false, ...) throws before processing the vote, whereas getVoteSitesNames() previously detected and skipped this malformed entry. Filter raw keys to actual configuration sections before dereferencing their fields.
Useful? React with 👍 / 👎.
| when(voteSitesConfig.getVoteSitesNames(false)) | ||
| .thenReturn(new ArrayList<String>(Arrays.asList("DisabledSite"))); |
There was a problem hiding this comment.
This test stubs getVoteSitesNames(false), but the production lookup now reads getRawVoteSiteNames() instead. Because the mocked raw lookup returns its empty/default value, getVoteSiteName(false, "disabled.example.com") falls back to the input string, so the assertion on line 229 fails whenever this test suite runs; stub getRawVoteSiteNames() with DisabledSite as well or instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1637784fbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (String siteName : configuredSites) { | ||
| String serviceSite = plugin.getConfigVoteSites().getServiceSite(siteName); | ||
| String displayName = plugin.getConfigVoteSites().getDisplayName(siteName); | ||
| if (siteName.equalsIgnoreCase(url) |
There was a problem hiding this comment.
When a disabled section uses the generated key form (for example example_com) but has a missing ServiceSite, an incoming example.com does not match this raw-key comparison, so hasVoteSite reports it absent. VotiferEvent then calls tryGenerateVoteSite, which normalizes the identifier to example_com and writes Enabled: true into that existing section—the configuration corruption this change is intended to prevent. Compare the normalized identifier with configured keys before allowing generation.
Useful? React with 👍 / 👎.
| || (serviceSite != null && serviceSite.equalsIgnoreCase(url)) | ||
| || (displayName != null && displayName.equalsIgnoreCase(url))) { |
There was a problem hiding this comment.
With the default AdvancedServiceSiteHandling: false, VotiferEvent passes "" as its second identifier. If any raw configured section explicitly has ServiceSite: "" (or an empty display name), this lookup matches that section after the real service identifier fails, causing every otherwise-unknown vote to appear configured; auto-creation is suppressed and the vote is subsequently discarded because no enabled site resolves it. Apply the same non-empty check used by the in-memory lookup before matching raw fields.
Useful? React with 👍 / 👎.
|
@codex review the current head after the added disabled-site regression tests. Please verify the offline queued-vote null guard, raw configured-site lookup, malformed configuration handling, normalized-key matching, and whether this PR is ready. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Motivation
VoteSiteManagerresolve names only from the in-memoryvoteSiteslist, but that list contains only enabled sites, which caused disabled-but-configured sites to appear missing and be auto-created (and re-enabled) during vote ingestion.checkEnabled=false, preventing inadvertent persistent re-enabling of disabled sites.Description
VoteSiteManager.getVoteSiteName(boolean, String...)now, whencheckEnabledis false, checks the complete configured site list viaplugin.getConfigVoteSites().getVoteSitesNames(false)and matches by key, service site (getServiceSite) or display name (getDisplayName).VoteSiteManager.hasVoteSite(String)is now null-safe and also consults the configured site names so configured-but-disabled sites are reported as existing.testDisabledConfiguredVoteSiteIsNotAutoCreatedtoVoteSiteManagerTestto assert that a disabled configured site resolves by service name, counts as present, is not returned by enabled-only lookups, and does not invoke site generation (tryGenerateVoteSiteis never called).Testing
git diff --checkand repository diff validation, which reported no whitespace or patch issues.mvn -Dtest=VoteSiteManagerTest test, but the Maven run failed due to dependency resolution returning HTTP 403 formaven-resources-plugin:3.3.1, preventing execution of the unit suite in this environment.Codex Task