refactor(poracle): delete the dead direct-DB paths and partition the invasion clean toggle - #827
Merged
Merged
Conversation
…clean toggle
Three unrelated pieces of cleanup that carry no PoracleNG version risk, plus the
verification the v2 migration plan asked for.
Dead code. IProfileService.CreateAsync/UpdateAsync/DeleteAsync and
ProfileRepository.GetByUserAsync/GetByUserAndProfileNoAsync/CreateAsync/DeleteAsync
had zero callers anywhere -- the controllers use ProfileService's proxy-backed
methods of the same names. IProfileRepository is now RenameAsync + UpdateAsync,
the two writes PoracleNG's API genuinely cannot serve, and ProfileService no
longer holds a repository at all.
PoracleContext no longer maps the eight alarm tables. Nothing consumed the
DbSets; IUserAreaDualWriter reaches those tables by raw SQL over a validated
table name. Removing them makes reintroducing a direct alarm write structurally
impossible, which is what the 2.0 migration was for.
Live bug: CleaningService.ToggleCleanAsync("invasion", ...) read the whole
invasion table -- which includes kecleon/showcase/gold-stop rows, because v1 does
not partition it -- set the auto-delete bit on all of them and POSTed them back.
InvasionService and DashboardService both apply the PokestopEventTypes filter at
the same boundary; cleaning did not. Now conditional on the same feature gate, so
that with the Pokestop Events page switched off the invasion switch keeps owning
those rows. GetCleanStatusAsync partitions the same way, and reports not-clean
rather than all-clean for an array holding only somebody else's rows.
UserGeofenceService's owner-name read for the Discord review card moves from
IHumanRepository to the proxy-first IHumanService, with its own catch so it
degrades on its own like every other piece of that card.
Verification, no code change: UserOwnedOverrideAreaProxy's premise did NOT
reproduce. With a real user-drawn fence carrying userSelectable=false, from a
human proven non-admin by setAreas stripping the same name in the same session,
5.1.0 v1, 5.2.1 v1 and 5.2.1 v2 all accepted and stored it. Recorded in
docs/poracleng-v2-review.md with the method; the class stays. The setAreas
control re-confirms blocker 1 on 5.2.1 v2.
Docs: the nine v2 findings for an upstream report, each confirmed by calling
5.2.1 rather than reading source, and the reasoning for not raising the minimum
to 5.2.0.
…ntroller The controller sweep only reads a controller's own constructor, so a service that gains an unregistered dependency -- UserGeofenceService taking IHumanService in this branch -- resolves the controller fine and 500s underneath it. Walks every registered implementation's constructor instead, unwrapping IEnumerable<T> to its element type so an empty sequence does not read as a satisfied dependency. Verified red by commenting out the ITrackedUidRemapper registration. Also drops a changelog line's claim that two places still reach the Poracle database; two places still reach the alarm tables, which is what PoracleContext's own remark says.
The new registered-implementation check passed on Windows and failed on Linux CI: AddPoracleServices registers implementations whose own dependencies it does not register: KeyManagementOptionsSetup -> IRegistryPolicyResolver Both are .NET's. AddDataProtection registers KeyManagementOptionsSetup, which takes an IRegistryPolicyResolver that exists on Windows and not on Linux. Walking framework registrations asserts something about .NET rather than about this application, and it answers differently per platform -- which is exactly how it failed. Scoped to assemblies named Pgan.PoracleWebNet. Confirmed it still has teeth: removing the IPokestopEventService registration turns exactly one case red, naming the controller and the service.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge this first. It makes no API calls, carries no version risk, and removes code the other two v2 branches would otherwise have to reason about.
Dead direct-DB paths removed
IProfileService.CreateAsync/UpdateAsync/DeleteAsyncandProfileRepository.GetByUserAsync/GetByUserAndProfileNoAsync— every one zero-caller, every one delegating straight to EF while the live path went through the proxy.PoracleContextloses its eight alarmDbSets and theirOnModelCreatingconfiguration. That is the point of the change: reintroducing a direct alarm write is now a compile error rather than a code-review question.The owner-name read in
UserGeofenceService.BuildSubmissionPostAsyncmoves from the repository to the proxy-first service.A live bug fixed on the way past
CleaningService.ToggleCleanAsync("invasion", …)had noPokestopEventTypesfilter, so it set the auto-delete bit on Kecleon, Showcase and Gold Stop rows and POSTed them back. Those rows share theinvasiontable, and the toggle was never meant to reach them.Consequence worth stating plainly: with the partition in place there is now no bulk auto-delete control for Pokéstop events. Per-rule auto-delete still exists in their dialogs. The invasion switch used to cover them, incorrectly. Nothing covers them in bulk now — that is a deliberate gap, not an oversight.
What stays on the database, and why
Also in this branch: a nine-item report for jfberry/PoracleNG covering what upstream would have to change to unblock the rest.
setAreasruns the identicaluserSelectablefilter as v1 (v2_humans.go:if !admin && !f.UserSelectable { continue }, and its own doc says "Mirror v1 HandleSetAreas"). PoracleWeb serves user-drawn geofences withuserSelectable=false, so v2 strips them exactly as v1 did.IUserAreaDualWriterand its 17HACK: trusted-set-areassites stay.ProfileRepository.RenameAsyncstays.One negative result worth recording
The
UserOwnedOverrideAreaProxyre-test came back negative, but the probe was wrong: it used a nonexistent fence name rather than a real user-drawn geofence carryinguserSelectable=false, because the dev feed currently serves 793 fences all marked selectable. The code path was never exercised. Nothing was removed on the strength of it, which is the right call — but someone still has to create a genuinely non-selectable fence and re-test on both 5.1.0 and 5.2.1 before that class can go.Verification
Backend green. Includes a new test catching a missing DI registration one level below the controller — the gap that let Pokéstop Events ship dead this morning was at the controller level, and this closes the level under it.