fix(favorite): follow the three-valued addUrl contract - #21
Merged
Merged
Conversation
FavoriteLogService.addUrl used to return a boolean and now returns a FavoriteResult (ADDED / ALREADY_ADDED / NO_SUCH_USER), so `!addUrl(...)` no longer compiles. This module has not built since that change, independently of anything else. The old false meant exactly one thing -- the user code resolved to no user and nothing was written -- so NO_SUCH_USER keeps raising the same error. The new outcome is ALREADY_ADDED: re-adding a URL the user already marked is an idempotent success, and the favorite_count bump is skipped for it. Bumping on a repeat is what the upstream change set out to stop, because favorite_count is a documented sort key and a double click or a client retry would otherwise move a document up the ranking without anyone else favoriting it. This mirrors how Fess's own v2 FavoritePostHandler branches on the same enum. No test covers this path in this module; the behaviour is verified only by matching the core handler.
The switch from a boolean addUrl to the three-valued FavoriteResult changed behaviour as well as the signature, and nothing in this module held that behaviour in place: the choice was justified only by matching Fess core's v2 FavoritePostHandler, which is not something a build can check. JsonApiManagerFavoriteTest pins all three outcomes against what core does. ADDED bumps favorite_count and reports status 0. ALREADY_ADDED reports the same status 0 but must not bump, because favorite_count is a documented sort key and a repeat would otherwise let one user push a document up the ranking by clicking twice; a separate case runs the request twice and asserts the count is bumped once. NO_SUCH_USER keeps reporting status 4 with "Failed to add url", the error the old false produced, and bumps nothing. The bump is searchHelper.update(id, lambda), so it is asserted as whether update was called and for which document id. A recording SearchHelper captures the call without running the lambda, which would need a live search engine client. One further case applies the lambda addUrl is handed and checks the log entry it populates, so an empty lambda could not pass. Capture happens at writeJsonResponse(int, String, String) so the real error shaping in the Throwable overload still runs and the asserted message is the one a caller sees. Both branches were mutation-checked: widening the ADDED guard to "not NO_SUCH_USER" fails the two count cases, and removing the NO_SUCH_USER guard fails the error case. Registration happens before super.setUp because the container's own jsonApiManager resolves webApiManagerFactory from a @PostConstruct, and the stub FessConfig answers each index-field key directly because FessConfig.SimpleImpl throws NullPointerException from any properties-backed getter without a container this test does not start. Tests run: 75 before, 80 after.
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.
The fix from #19, built on the OpenSearch packages Fess uses today, so it can be merged ahead of the OpenSearch fork migration.
maindoes not compile against the current Fess 15.9.0-SNAPSHOT:FavoriteLogService.addUrlreturns the three-valuedFavoriteResultsince codelibs/fess#3391, soif (!addUrl(...))inJsonApiManager#processFavoriteRequestis no longer valid Java, and every PR on this repository fails CI until that is fixed.#19 fixes it, but its new test imports
org.codelibs.fesen.opensearch.action.update.UpdateRequestBuilder, which exists only once codelibs/fess#3439 relocates OpenSearch, so its test compile fails today. This branch carries #19's two commits unchanged except for that one import, which is noworg.opensearch.action.update.UpdateRequestBuilder, the typeSearchHelper#updatetakes in current Fess. The production change is identical to #19; switching the import to the relocated package belongs with the fork migration.What the fix does (from #19)
addUrlresultFavoritePostHandlerADDEDfavorite_count, report status 0ALREADY_ADDEDfavorite_countis not bumpedNO_SUCH_USERTesting
mvn -B -U packageagainst the published Fess 15.9.0-SNAPSHOT (build -71), the way CI runs it: 80 tests, 0 failures, including theJsonApiManagerFavoriteTestcases.test-compile.