SAK-52258 Samigo Refactor question search functionality to use QuestionSearchService#14354
SAK-52258 Samigo Refactor question search functionality to use QuestionSearchService#14354kunaljaykam wants to merge 9 commits intosakaiproject:masterfrom
Conversation
…hService and remove OpenSearch dependencies
The removal of searchResponse() from SearchService API will be done in a separate PR (stmp-1) after this PR is merged. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
These modules need OpenSearch dependency to mock SearchService until PR2 (stmp-1) removes searchResponse() from the SearchService interface. After PR2 is merged, these dependencies can be removed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThis pull request introduces a new QuestionSearchService abstraction layer backed by Elasticsearch/OpenSearch, replacing the previous search implementation. The SearchQuestionBean is refactored to delegate to this service and use a new QuestionSearchResult data model. Dependencies are reorganized across modules, and the JSP search results view is updated. Changes
Suggested reviewers
🚥 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)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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: 1
🤖 Fix all issues with AI agents
In
`@samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/services/question/QuestionSearchServiceImpl.java`:
- Around line 158-189: The method processSearchResponse may NPE when
hit.field("tags"), hit.field("typeId"), or hit.field("qText") return null;
before calling getValues() or getValue() check the Field/DocumentField is
non-null and only then extract values (e.g., verify hit.field("tags") != null
before calling getValues(), and similarly guard hit.field("typeId") and
hit.field("qText")), falling back to empty set or null/empty string as
appropriate to match how questionPoolId/assessmentId/site are handled later in
processSearchResponse; update the code paths that build tags, typeId, and qText
to follow the same null-check pattern used for questionPoolId/assessmentId/site
to avoid dropping valid hits.
🧹 Nitpick comments (3)
samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/services/question/QuestionSearchServiceImpl.java (1)
58-84: Optional: extract shared search-parameter setup.
searchByTagsandsearchByTextduplicate the same base params (group,scope,subtype,logic). A small helper could reduce repetition.samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/bean/author/SearchQuestionBean.java (2)
104-118: Avoid doubleOptionallookup for tag retrieval.
tagService.getTags().getForId(tagId)is called twice per iteration—once forisPresent()and again forget(). This is inefficient and risks a race condition if the tag is removed between calls.♻️ Proposed fix using ifPresent or direct Optional handling
if (tagList != null) { boolean first = true; for (String tagId : tagList) { - if (tagService.getTags().getForId(tagId).isPresent()) { - Tag tag = tagService.getTags().getForId(tagId).get(); + Optional<Tag> optionalTag = tagService.getTags().getForId(tagId); + if (optionalTag.isPresent()) { + Tag tag = optionalTag.get(); String fullLabel = tag.getTagLabel() + "(" + tag.getCollectionName() + ")"; tagLabelsForSearch.add(fullLabel); if (!first) {You'll need to add
import java.util.Optional;at the top of the file.
215-225: Consider cachingItemServiceinstead of instantiating on each call.Both
getItemandgetDatacreate a newItemService()instance per invocation. If these methods are called repeatedly (e.g., in JSP iteration over search results), this could be inefficient.♻️ Proposed refactor to cache ItemService
Add as an instance field alongside the other services:
private QuestionPoolService questionPoolService = new QuestionPoolService(); private AssessmentService assessmentService = new AssessmentService(); + private ItemService itemService = new ItemService();Then update the methods:
public ItemFacade getItem(String itemId) { - return new ItemService().getItem(itemId); + return itemService.getItem(itemId); } public ItemDataIfc getData(String itemId) { - ItemFacade item = new ItemService().getItem(itemId); + ItemFacade item = itemService.getItem(itemId); if (item == null) { return null; } return item.getData(); }
| private List<QuestionSearchResult> processSearchResponse(SearchResponse sr) { | ||
| List<QuestionSearchResult> results = new ArrayList<>(); | ||
|
|
||
| Terms dedup = sr.getAggregations().get("dedup"); | ||
| if (dedup == null) { | ||
| return results; | ||
| } | ||
|
|
||
| for (Terms.Bucket entry : dedup.getBuckets()) { | ||
| TopHits topHits = entry.getAggregations().get("dedup_docs"); | ||
| if (topHits == null) { | ||
| continue; | ||
| } | ||
|
|
||
| for (SearchHit hit : topHits.getHits().getHits()) { | ||
| try { | ||
| String id = hit.getId(); | ||
| if (!id.startsWith("/sam_item/")) { | ||
| continue; | ||
| } | ||
|
|
||
| String itemId = id.substring(10); | ||
| String typeId = hit.field("typeId").getValue(); | ||
| String qText = hit.field("qText").getValue(); | ||
|
|
||
| Set<String> tags = new HashSet<>(); | ||
| List<Object> tagValues = hit.field("tags").getValues(); | ||
| if (tagValues != null) { | ||
| for (Object tagValue : tagValues) { | ||
| tags.add(tagValue.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the actual file
fd -t f "QuestionSearchServiceImpl.java" --exec cat -n {} \;Repository: sakaiproject/sakai
Length of output: 13187
🏁 Script executed:
# Check the pom.xml or dependencies to understand Elasticsearch version
fd -t f "pom.xml" | head -5 | xargs -I {} grep -A 5 -B 5 "elasticsearch" {} 2>/dev/null | head -50Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
# Search for usage patterns of hit.field() in the codebase
rg "hit\.field\(" -A 2 -B 2 samiagoRepository: sakaiproject/sakai
Length of output: 109
🏁 Script executed:
# Search for other usages of hit.field() in samiago directory
rg "hit\.field\(" samigo -A 1 -B 1 -cRepository: sakaiproject/sakai
Length of output: 178
🌐 Web query:
OpenSearch SearchHit field() method null behavior documentation
💡 Result:
In OpenSearch server (High Level REST client / server-side classes), org.opensearch.search.SearchHit#field(String fieldName) returns the DocumentField for that name if it was actually returned on the hit (i.e., you requested stored fields / fields / script fields, etc.). The Javadoc describes it only as “The hit field matching the given field name” and does not explicitly document nullability. In practice, if the field isn’t present in the hit’s fields map, the result is null (same behavior you’d expect from a map lookup). [1]
Related: SearchHit#getFields() is “a map of hit fields … if additional fields were required to be loaded” (so if you didn’t request any, don’t assume the map contains your field). [1]
In the opensearch-java (new Java client), the per-hit fields() accessor returns a Map<String, JsonData>; if you didn’t request fields (or a specific field wasn’t returned), it will simply be absent from the map (so looking it up yields null). [2]
Sources
[1] OpenSearch server Javadoc: org.opensearch.search.SearchHit (includes field(String) and getFields() docs) https://javadoc.io/static/org.opensearch/opensearch/1.2.4/org/opensearch/search/SearchHit.html
[2] OpenSearch Java client Javadoc: org.opensearch.client.opensearch.core.search.Hit#fields() https://www.javadoc.io/static/org.opensearch.client/opensearch-java/2.17.0/org/opensearch/client/opensearch/core/search/Hit.html
Add null check before calling getValues() on the tags field.
The hit.field("tags") may return null if the field is missing from the hit, and calling .getValues() on null throws a NullPointerException. This causes valid search results to be dropped silently. The same pattern should also be applied to typeId and qText (lines 180–181) for consistency.
Fix for tags field
- List<Object> tagValues = hit.field("tags").getValues();
- if (tagValues != null) {
+ List<Object> tagValues = null;
+ if (hit.field("tags") != null) {
+ tagValues = hit.field("tags").getValues();
+ }
+ if (tagValues != null) {
for (Object tagValue : tagValues) {
tags.add(tagValue.toString());
}
}This pattern already exists in the method for questionPoolId, assessmentId, and site (lines 191–204) and should be applied consistently to all optional fields.
🤖 Prompt for AI Agents
In
`@samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/services/question/QuestionSearchServiceImpl.java`
around lines 158 - 189, The method processSearchResponse may NPE when
hit.field("tags"), hit.field("typeId"), or hit.field("qText") return null;
before calling getValues() or getValue() check the Field/DocumentField is
non-null and only then extract values (e.g., verify hit.field("tags") != null
before calling getValues(), and similarly guard hit.field("typeId") and
hit.field("qText")), falling back to empty set or null/empty string as
appropriate to match how questionPoolId/assessmentId/site are handled later in
processSearchResponse; update the code paths that build tags, typeId, and qText
to follow the same null-check pattern used for questionPoolId/assessmentId/site
to avoid dropping valid hits.
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.