feat(discovery): apply policy filtering to home feed#27
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional countryCode and ageVerified query params to GET /v1/discovery/home, validates country codes, wires PolicyEvaluationClient into HomeFeedService to filter out policy-blocked items before blending, and returns 503 POLICY_SERVICE_UNAVAILABLE on policy evaluation failures. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as DiscoveryController
participant Service as HomeFeedService
participant Search as SearchIndexReader
participant Policy as PolicyEvaluationClient
Client->>Controller: GET /v1/discovery/home (userId,size,countryCode,ageVerified)
Controller->>Controller: validate `countryCode`
Controller->>Service: homeFeed(userId,size,countryCode,ageVerified)
Service->>Search: fetch candidate buckets
Search-->>Service: candidate items
Service->>Policy: evaluateBatch(contentIds,countryCode,ageVerified)
Policy-->>Service: Map(contentId→PolicyDecision)
loop per candidate item
alt decision.allowed == true
Service->>Service: include item
else decision.allowed == false
Service->>Service: exclude item
end
end
Service-->>Controller: HomeFeedResponse(filtered items)
Controller-->>Client: HTTP 200 (or 503 POLICY_SERVICE_UNAVAILABLE on policy error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
services/java/discovery-service/src/test/java/com/cloudmedia/discovery/discovery/HomeFeedServiceTest.java (1)
19-25: Assert the forwarded policy context too.
RecordingPolicyEvaluationClientignorescountryCodeandageVerified, so these tests would still pass ifHomeFeedServicestarted callingevaluate(contentId, null, null)or swapped the arguments. Please record those parameters and assert them in at least one happy-path test.Also applies to: 97-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/discovery-service/src/test/java/com/cloudmedia/discovery/discovery/HomeFeedServiceTest.java` around lines 19 - 25, The test is not verifying that HomeFeedService.forwarded the policy context (countryCode and ageVerified) to the policy evaluation; update the RecordingPolicyEvaluationClient used in the test to capture the evaluate(...) args (e.g., add fields like recordedCountryCodes and recordedAgeVerifieds or a list of tuples when evaluate(contentId, countryCode, ageVerified) is called) and then add assertions in the happy-path test that at least one recorded invocation contains the expected countryCode ("US") and ageVerified (true) alongside the recordedContentIds; ensure the HomeFeedService.homeFeed invocation in the test still uses the same parameters so the new assertions validate the correct argument forwarding.services/java/discovery-service/src/test/java/com/cloudmedia/discovery/api/discovery/DiscoveryControllerTest.java (1)
48-52: Add the controller-level 503 case as well.The PR introduces a new outward-facing
POLICY_SERVICE_UNAVAILABLEresponse forGET /v1/discovery/home, but this suite only covers success and validation. Please add a MockMvc test that hasHomeFeedService.homeFeed(...)fail and asserts503plus the serialized error code.Also applies to: 54-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/discovery-service/src/test/java/com/cloudmedia/discovery/api/discovery/DiscoveryControllerTest.java` around lines 48 - 52, Add a MockMvc test in DiscoveryControllerTest that simulates HomeFeedService.homeFeed(...) failing and asserts the controller returns HTTP 503 with the serialized error code POLICY_SERVICE_UNAVAILABLE: mock the HomeFeedService.homeFeed call (using the same mock used in the suite) to throw the service-level exception or otherwise produce the failure path the controller maps to POLICY_SERVICE_UNAVAILABLE, then perform get("/v1/discovery/home") via mockMvc and assert status().isServiceUnavailable() and jsonPath("$.error.code").value("POLICY_SERVICE_UNAVAILABLE"); place this alongside the existing validation and success tests so it covers the new outward-facing 503 response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contracts/rest-api-v1.md`:
- Around line 132-137: Update the docs for the GET /v1/discovery/home endpoint
to document the new 503 response: add that the service may return HTTP 503 with
error code POLICY_SERVICE_UNAVAILABLE when policy evaluation fails (in addition
to the existing blended-feed behavior and filtering note), and also add this
503/POLICY_SERVICE_UNAVAILABLE to the HTTP status usage section so clients know
to retry or surface an appropriate error; reference the endpoint name GET
/v1/discovery/home and the error code POLICY_SERVICE_UNAVAILABLE when making the
additions.
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/discovery/DiscoveryController.java`:
- Line 35: The current regexp in DiscoveryController for the countryCode request
param only enforces format and not real ISO 3166-1 alpha-2 membership; replace
it with a real allowlist or custom validator (e.g., create an annotation like
`@ValidCountryCode` with a CountryCodeValidator) or explicitly validate against
Locale.getISOCountries() (or a maintained Set of valid codes) inside the
controller method before passing countryCode to policy evaluation so values like
"ZZ" are rejected with a 400 and a clear message; reference the countryCode
parameter in DiscoveryController and add/annotate the new validator class
(CountryCodeValidator or similar) to centralize the check.
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.java`:
- Around line 35-36: The current flow calls blend(candidates, resolvedSize) then
filterByPolicy(...), which truncates to resolvedSize before removing blocked
items and can leave the final feed short; update the logic so policy filtering
happens before the final trim or make blend policy-aware — either (a) run
filterByPolicy on the candidates (or apply policy rules inside blend) and then
call blend(...) with the already-filtered list, or (b) over-fetch by increasing
the count passed to blend (e.g., fetch >resolvedSize), then apply
filterByPolicy(blended, countryCode, ageVerified) and finally truncate to
resolvedSize when constructing the HomeFeedResponse; reference blend,
filterByPolicy, HomeFeedItem, HomeFeedResponse, resolvedSize, and candidates
when making the change.
---
Nitpick comments:
In
`@services/java/discovery-service/src/test/java/com/cloudmedia/discovery/api/discovery/DiscoveryControllerTest.java`:
- Around line 48-52: Add a MockMvc test in DiscoveryControllerTest that
simulates HomeFeedService.homeFeed(...) failing and asserts the controller
returns HTTP 503 with the serialized error code POLICY_SERVICE_UNAVAILABLE: mock
the HomeFeedService.homeFeed call (using the same mock used in the suite) to
throw the service-level exception or otherwise produce the failure path the
controller maps to POLICY_SERVICE_UNAVAILABLE, then perform
get("/v1/discovery/home") via mockMvc and assert status().isServiceUnavailable()
and jsonPath("$.error.code").value("POLICY_SERVICE_UNAVAILABLE"); place this
alongside the existing validation and success tests so it covers the new
outward-facing 503 response.
In
`@services/java/discovery-service/src/test/java/com/cloudmedia/discovery/discovery/HomeFeedServiceTest.java`:
- Around line 19-25: The test is not verifying that HomeFeedService.forwarded
the policy context (countryCode and ageVerified) to the policy evaluation;
update the RecordingPolicyEvaluationClient used in the test to capture the
evaluate(...) args (e.g., add fields like recordedCountryCodes and
recordedAgeVerifieds or a list of tuples when evaluate(contentId, countryCode,
ageVerified) is called) and then add assertions in the happy-path test that at
least one recorded invocation contains the expected countryCode ("US") and
ageVerified (true) alongside the recordedContentIds; ensure the
HomeFeedService.homeFeed invocation in the test still uses the same parameters
so the new assertions validate the correct argument forwarding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd7e3d3c-0b10-4488-99fe-7e13da0d79ae
📒 Files selected for processing (5)
docs/contracts/rest-api-v1.mdservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/discovery/DiscoveryController.javaservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.javaservices/java/discovery-service/src/test/java/com/cloudmedia/discovery/api/discovery/DiscoveryControllerTest.javaservices/java/discovery-service/src/test/java/com/cloudmedia/discovery/discovery/HomeFeedServiceTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.java (1)
14-20: Pre-compile the regex pattern to avoid repeated compilation.
String.matches()compiles a newPatternon every call. For a validator invoked on each request, consider extracting a pre-compiled pattern.♻️ Suggested refactor
public class CountryCodeValidator implements ConstraintValidator<ValidCountryCode, String> { + private static final java.util.regex.Pattern FORMAT_PATTERN = java.util.regex.Pattern.compile("^[A-Z]{2}$"); + private static final Set<String> ISO_COUNTRY_CODES = Arrays.stream(java.util.Locale.getISOCountries()) .collect(Collectors.toUnmodifiableSet()); `@Override` public boolean isValid(String value, ConstraintValidatorContext context) { if (value == null) { return true; } - return value.matches("^[A-Z]{2}$") && ISO_COUNTRY_CODES.contains(value); + return FORMAT_PATTERN.matcher(value).matches() && ISO_COUNTRY_CODES.contains(value); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.java` around lines 14 - 20, The isValid method in CountryCodeValidator currently calls String.matches which recompiles the regex on every invocation; create a private static final java.util.regex.Pattern (e.g., COUNTRY_CODE_PATTERN) at class scope with the "^[A-Z]{2}$" regex and replace value.matches(...) with COUNTRY_CODE_PATTERN.matcher(value).matches(), keeping the existing null-check and ISO_COUNTRY_CODES.contains(value) logic intact (use the same ISO_COUNTRY_CODES reference).services/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.java (1)
90-99: Deduplicate policy evaluations across buckets to reduce policy service load.If the same
contentIdappears in multiple buckets (e.g., in bothtrendingandfresh), it will be evaluated multiple times. Sinceevaluate()makes HTTP calls to the policy service (2s connect + 3s read timeout), this introduces unnecessary latency and load.Suggested approach: collect distinct
contentIdsfrom all buckets, evaluate once, then filter each bucket against the cached results. This requires adding a batch evaluation method toPolicyEvaluationClientand updating the filtering logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.java` around lines 90 - 99, The current filterByPolicy method calls policyEvaluationClient.evaluate(item.contentId(), ...) for every HomeFeedItem, causing duplicate HTTP calls for repeated contentIds; change filterByPolicy to first collect distinct contentIds from the input List<HomeFeedItem> (using HomeFeedItem::contentId), call a new batch method on PolicyEvaluationClient (e.g., evaluateBatch(Collection<String> contentIds, String countryCode, Boolean ageVerified)) to get a Map<String,PolicyDecision> or Map<String,Boolean> of allowed results, then apply that cached map to filter each item (lookup by contentId) instead of calling evaluate per item; preserve the current PolicyEvaluationException handling (throw ApiException with SERVICE_UNAVAILABLE) when the batch call fails and ensure references to filterByPolicy, policyEvaluationClient.evaluate, PolicyEvaluationClient, HomeFeedItem.contentId(), and PolicyEvaluationException are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.java`:
- Around line 14-20: The isValid method in CountryCodeValidator currently calls
String.matches which recompiles the regex on every invocation; create a private
static final java.util.regex.Pattern (e.g., COUNTRY_CODE_PATTERN) at class scope
with the "^[A-Z]{2}$" regex and replace value.matches(...) with
COUNTRY_CODE_PATTERN.matcher(value).matches(), keeping the existing null-check
and ISO_COUNTRY_CODES.contains(value) logic intact (use the same
ISO_COUNTRY_CODES reference).
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.java`:
- Around line 90-99: The current filterByPolicy method calls
policyEvaluationClient.evaluate(item.contentId(), ...) for every HomeFeedItem,
causing duplicate HTTP calls for repeated contentIds; change filterByPolicy to
first collect distinct contentIds from the input List<HomeFeedItem> (using
HomeFeedItem::contentId), call a new batch method on PolicyEvaluationClient
(e.g., evaluateBatch(Collection<String> contentIds, String countryCode, Boolean
ageVerified)) to get a Map<String,PolicyDecision> or Map<String,Boolean> of
allowed results, then apply that cached map to filter each item (lookup by
contentId) instead of calling evaluate per item; preserve the current
PolicyEvaluationException handling (throw ApiException with SERVICE_UNAVAILABLE)
when the batch call fails and ensure references to filterByPolicy,
policyEvaluationClient.evaluate, PolicyEvaluationClient,
HomeFeedItem.contentId(), and PolicyEvaluationException are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7df63c6-9b86-447d-8860-3259bc725e19
📒 Files selected for processing (7)
docs/contracts/rest-api-v1.mdservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/discovery/DiscoveryController.javaservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.javaservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/ValidCountryCode.javaservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.javaservices/java/discovery-service/src/test/java/com/cloudmedia/discovery/api/discovery/DiscoveryControllerTest.javaservices/java/discovery-service/src/test/java/com/cloudmedia/discovery/discovery/HomeFeedServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/discovery/DiscoveryController.java
- docs/contracts/rest-api-v1.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.java (1)
5-8: Minor: Importjava.util.Localefor consistency.
Localeis referenced with its fully qualified name on line 12, while otherjava.utilclasses are imported. Consider adding the import for consistency.Suggested fix
import java.util.Arrays; +import java.util.Locale; import java.util.Set; import java.util.stream.Collectors; import java.util.regex.Pattern;Then on line 12:
- private static final Set<String> ISO_COUNTRY_CODES = Arrays.stream(java.util.Locale.getISOCountries()) + private static final Set<String> ISO_COUNTRY_CODES = Arrays.stream(Locale.getISOCountries())Also applies to: 12-12
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.java` around lines 5 - 8, CountryCodeValidator uses java.util.Locale fully-qualified on line 12; add an import for Locale and replace the fully-qualified reference with Locale to match other java.util imports for consistency — update the imports block to include Locale and adjust any usages within CountryCodeValidator (e.g., Locale.getISOCountries()) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/policy/PolicyEvaluationClient.java`:
- Around line 11-20: The default evaluateBatch implementation performs
sequential calls to evaluate for each contentId causing high cumulative latency;
update evaluateBatch (or override it in HttpPolicyEvaluationClient) to perform
calls in parallel (e.g., create uniqueIds from contentIds, launch
evaluate(contentId, countryCode, ageVerified) via CompletableFuture/supplyAsync
with a bounded Executor, collect .join() results into the LinkedHashMap) or,
preferably, implement a true batch call in HttpPolicyEvaluationClient that
invokes a policy service batch endpoint (e.g., POST
/v1/policy/content/evaluate-batch) and maps the single response to the returned
Map<String,PolicyDecision>.
---
Nitpick comments:
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.java`:
- Around line 5-8: CountryCodeValidator uses java.util.Locale fully-qualified on
line 12; add an import for Locale and replace the fully-qualified reference with
Locale to match other java.util imports for consistency — update the imports
block to include Locale and adjust any usages within CountryCodeValidator (e.g.,
Locale.getISOCountries()) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 980b30db-88d2-4184-8a6f-e3776398bc8d
📒 Files selected for processing (3)
services/java/discovery-service/src/main/java/com/cloudmedia/discovery/api/validation/CountryCodeValidator.javaservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.javaservices/java/discovery-service/src/main/java/com/cloudmedia/discovery/policy/PolicyEvaluationClient.java
🚧 Files skipped from review as they are similar to previous changes (1)
- services/java/discovery-service/src/main/java/com/cloudmedia/discovery/discovery/HomeFeedService.java
Comment on lines
+11
to
+20
| default Map<String, PolicyDecision> evaluateBatch(Collection<String> contentIds, String countryCode, | ||
| Boolean ageVerified) { | ||
| Map<String, PolicyDecision> decisions = new LinkedHashMap<>(); | ||
| for (String contentId : contentIds) { | ||
| if (contentId != null && !decisions.containsKey(contentId)) { | ||
| decisions.put(contentId, evaluate(contentId, countryCode, ageVerified)); | ||
| } | ||
| } | ||
| return decisions; | ||
| } |
There was a problem hiding this comment.
Sequential HTTP calls may cause high latency on home feed requests.
The default implementation makes N sequential HTTP calls to the policy service (one per unique content ID). For a home feed with 30-50 items, this could add 100-300ms+ of cumulative latency under normal conditions, and much worse under load or partial service degradation.
Consider parallelizing the calls or introducing a true batch endpoint on the policy service:
♻️ Option 1: Parallel execution with CompletableFuture
default Map<String, PolicyDecision> evaluateBatch(Collection<String> contentIds, String countryCode,
Boolean ageVerified) {
List<String> uniqueIds = contentIds.stream()
.filter(Objects::nonNull)
.distinct()
.toList();
Map<String, CompletableFuture<PolicyDecision>> futures = new LinkedHashMap<>();
for (String contentId : uniqueIds) {
futures.put(contentId, CompletableFuture.supplyAsync(
() -> evaluate(contentId, countryCode, ageVerified)));
}
Map<String, PolicyDecision> decisions = new LinkedHashMap<>();
for (var entry : futures.entrySet()) {
decisions.put(entry.getKey(), entry.getValue().join());
}
return decisions;
}This requires adding import java.util.concurrent.CompletableFuture;, import java.util.List;, and import java.util.Objects;. You may also want to use a bounded executor to limit concurrent requests.
♻️ Option 2: True batch API (preferred if policy service supports it)
Override evaluateBatch in HttpPolicyEvaluationClient to call a batch endpoint (e.g., POST /v1/policy/content/evaluate-batch) that accepts multiple content IDs in a single request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/java/discovery-service/src/main/java/com/cloudmedia/discovery/policy/PolicyEvaluationClient.java`
around lines 11 - 20, The default evaluateBatch implementation performs
sequential calls to evaluate for each contentId causing high cumulative latency;
update evaluateBatch (or override it in HttpPolicyEvaluationClient) to perform
calls in parallel (e.g., create uniqueIds from contentIds, launch
evaluate(contentId, countryCode, ageVerified) via CompletableFuture/supplyAsync
with a bounded Executor, collect .join() results into the LinkedHashMap) or,
preferably, implement a true batch call in HttpPolicyEvaluationClient that
invokes a policy service batch endpoint (e.g., POST
/v1/policy/content/evaluate-batch) and maps the single response to the returned
Map<String,PolicyDecision>.
poyrazK
added a commit
that referenced
this pull request
Apr 4, 2026
feat(discovery): apply policy filtering to home feed
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.
Summary
GET /v1/discovery/hometo accept optionalcountryCodeandageVerifiedpolicy context parameters with country code validation503 POLICY_SERVICE_UNAVAILABLEif policy checks are unavailableVerification
mvn -pl discovery-service spotless:apply checkstyle:checkmvn -pl discovery-service testSummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests