Skip to content

Commit 9c85868

Browse files
srikanthpadakantisrikanthpadakanti
committed
Fix: Ensure TermsLookup supports both id and query; added validation checks and updated unit tests
1 parent 997ddbe commit 9c85868

File tree

2 files changed

+142
-194
lines changed

2 files changed

+142
-194
lines changed

‎server/src/main/java/org/opensearch/indices/TermsLookup.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public TermsLookup(String index, String id, String path, QueryBuilder query) {
8484
}
8585
if (id != null && query != null) {
8686
throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element cannot specify both id and query.");
87+
8788
}
8889

8990
this.index = index;
@@ -170,10 +171,16 @@ public TermsLookup query(QueryBuilder query) {
170171
}
171172

172173
public void setQuery(QueryBuilder query) {
174+
if (this.id != null && query != null) {
175+
throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element cannot specify both id and query.");
176+
}
173177
this.query = query;
174178
}
175179

176180
public TermsLookup id(String id) {
181+
if (this.query != null && id != null) {
182+
throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element cannot specify both id and query.");
183+
}
177184
this.id = id;
178185
return this;
179186
}
@@ -184,6 +191,15 @@ public TermsLookup id(String id) {
184191
String path = (String) args[2];
185192
QueryBuilder query = (QueryBuilder) args[3]; // Optional query
186193

194+
// Validation: Either id or query must be present, but not both
195+
if (id == null && query == null) {
196+
throw new IllegalArgumentException(
197+
"[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying either the id or the query."
198+
);
199+
}
200+
if (id != null && query != null) {
201+
throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element cannot specify both id and query.");
202+
}
187203
return new TermsLookup(index, id, path, query);
188204
});
189205
static {

‎server/src/test/java/org/opensearch/index/query/TermQueryWithDocIdAndQueryTests.java

Lines changed: 126 additions & 194 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,7 @@ public void testTermsQueryWithQueryOnlyAndVerifyResults() throws Exception {
103103
SearchHits mockHits = new SearchHits(
104104
new SearchHit[] {
105105
new SearchHit(1).sourceRef(new BytesArray("{\"name\":\"Jane Doe\",\"student_id\":\"111\"}")),
106-
new SearchHit(2).sourceRef(new BytesArray("{\"name\":\"Mary Major\",\"student_id\":\"222\"}"))
107-
},
106+
new SearchHit(2).sourceRef(new BytesArray("{\"name\":\"Mary Major\",\"student_id\":\"222\"}")) },
108107
new TotalHits(2, TotalHits.Relation.EQUAL_TO),
109108
1.0f
110109
);
@@ -123,196 +122,129 @@ public void testTermsQueryWithQueryOnlyAndVerifyResults() throws Exception {
123122
assertEquals("Mary Major", hits[1].getSourceAsMap().get("name"));
124123
assertEquals("222", hits[1].getSourceAsMap().get("student_id"));
125124
}
125+
126+
public void testEnhancedTermsLookupWithQueryClause() throws Exception {
127+
// Setup TermsLookup with a query clause
128+
QueryBuilder queryBuilder = QueryBuilders.matchQuery("course", "Math101");
129+
TermsLookup termsLookup = new TermsLookup("courses", null, "students", queryBuilder);
130+
131+
TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
132+
133+
// Mock QueryShardContext
134+
QueryShardContext context = mock(QueryShardContext.class);
135+
when(context.getIndexSettings()).thenReturn(null);
136+
137+
// Rewrite the query before execution
138+
QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
139+
140+
// Validate the rewritten query
141+
assertNotNull(rewrittenQueryBuilder);
142+
assertThat(rewrittenQueryBuilder, instanceOf(QueryBuilder.class));
143+
}
144+
145+
public void testQueryClauseReturnsNoResults() throws Exception {
146+
// Setup TermsLookup with a query clause that returns no results
147+
QueryBuilder queryBuilder = QueryBuilders.matchQuery("course", "NonExistentCourse");
148+
TermsLookup termsLookup = new TermsLookup("courses", null, "students", queryBuilder);
149+
150+
TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
151+
152+
// Mock QueryShardContext
153+
QueryShardContext context = mock(QueryShardContext.class);
154+
when(context.getIndexSettings()).thenReturn(null);
155+
156+
// Rewrite the query before execution
157+
QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
158+
159+
// Mock the search response with no hits
160+
SearchResponse mockResponse = mock(SearchResponse.class);
161+
SearchHits mockHits = new SearchHits(new SearchHit[0], new TotalHits(0, TotalHits.Relation.EQUAL_TO), 0.0f);
162+
when(mockResponse.getHits()).thenReturn(mockHits);
163+
164+
// Validate the results
165+
assertEquals(0, mockResponse.getHits().getHits().length);
166+
}
167+
168+
public void testTermsQueryWithInsertedData() throws Exception {
169+
// Setup TermsLookup with valid data
170+
TermsLookup termsLookup = new TermsLookup("classes", "102", "enrolled");
171+
TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
172+
173+
// Mock QueryShardContext
174+
QueryShardContext context = mock(QueryShardContext.class);
175+
when(context.getIndexSettings()).thenReturn(null);
176+
177+
// Rewrite the query before execution
178+
QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
179+
180+
// Mock the search response
181+
SearchResponse mockResponse = mock(SearchResponse.class);
182+
SearchHits mockHits = new SearchHits(
183+
new SearchHit[] {
184+
new SearchHit(1).sourceRef(new BytesArray("{\"name\":\"John Smith\",\"student_id\":\"333\"}")),
185+
new SearchHit(2).sourceRef(new BytesArray("{\"name\":\"Alice Brown\",\"student_id\":\"444\"}")) },
186+
new TotalHits(2, TotalHits.Relation.EQUAL_TO),
187+
1.0f
188+
);
189+
when(mockResponse.getHits()).thenReturn(mockHits);
190+
191+
// Validate the results
192+
SearchHit[] hits = mockResponse.getHits().getHits();
193+
assertEquals(2, hits.length);
194+
assertEquals("John Smith", hits[0].getSourceAsMap().get("name"));
195+
assertEquals("333", hits[0].getSourceAsMap().get("student_id"));
196+
assertEquals("Alice Brown", hits[1].getSourceAsMap().get("name"));
197+
assertEquals("444", hits[1].getSourceAsMap().get("student_id"));
198+
}
199+
200+
public void testTermsQueryWithNoIdAndNoQuery() {
201+
// Attempt to create a TermsLookup with no id and no query
202+
Exception exception = expectThrows(IllegalArgumentException.class, () -> {
203+
new TermsLookup("classes", null, "enrolled");
204+
});
205+
206+
// Verify the exception message
207+
assertEquals("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying either the id or the query.", exception.getMessage());
208+
}
209+
210+
public void testTermsQueryWithIdAndQuery() throws Exception {
211+
// Setup TermsLookup with both id and query
212+
QueryBuilder queryBuilder = QueryBuilders.matchQuery("course", "CS102");
213+
TermsLookup termsLookup = new TermsLookup("classes", "103", "enrolled");
214+
215+
// Expect an exception due to both id and query being set
216+
Exception exception = expectThrows(IllegalArgumentException.class, () -> termsLookup.setQuery(queryBuilder));
217+
assertEquals("[" + TermsQueryBuilder.NAME + "] query lookup element cannot specify both id and query.", exception.getMessage());
218+
219+
}
220+
221+
public void testTermsQueryWithComplexQuery() throws Exception {
222+
// Setup TermsLookup with a complex query
223+
QueryBuilder queryBuilder = QueryBuilders.boolQuery()
224+
.must(QueryBuilders.matchQuery("course", "CS103"))
225+
.filter(QueryBuilders.rangeQuery("year").gte(2020));
226+
TermsLookup termsLookup = new TermsLookup("classes", null, "enrolled", queryBuilder);
227+
228+
TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
229+
230+
// Mock QueryShardContext
231+
QueryShardContext context = mock(QueryShardContext.class);
232+
when(context.getIndexSettings()).thenReturn(null);
233+
234+
// Rewrite the query before execution
235+
QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
236+
237+
// Validate the rewritten query
238+
assertNotNull(rewrittenQueryBuilder);
239+
assertThat(rewrittenQueryBuilder, instanceOf(QueryBuilder.class));
240+
}
126241
}
127-
// {
128-
//
129-
// public void testEnhancedTermsLookupWithQueryClause() throws Exception {
130-
// // Setup TermsLookup with a valid id and query clause
131-
// QueryBuilder queryBuilder = QueryBuilders.matchQuery("name", "CS101");
132-
// TermsLookup termsLookup = new TermsLookup("classes", "101", "enrolled").query(queryBuilder);
133-
// TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
134-
//
135-
// // Create real IndexMetadata with valid settings
136-
// Settings indexSettings = Settings.builder()
137-
// .put("index.query.bool.max_clause_count", 1024) // Example setting
138-
// .put("index.version.created", 8000099) // Add the required index.version.created setting
139-
// .build();
140-
// IndexMetadata indexMetadata = IndexMetadata.builder("test_index")
141-
// .settings(indexSettings)
142-
// .numberOfShards(1)
143-
// .numberOfReplicas(1)
144-
// .build();
145-
//
146-
// // Create IndexSettings using the IndexMetadata
147-
// IndexSettings settings = new IndexSettings(indexMetadata, indexSettings);
148-
//
149-
// // Mock QueryShardContext
150-
// QueryShardContext context = mock(QueryShardContext.class);
151-
// when(context.getIndexSettings()).thenReturn(settings); // Return the real IndexSettings
152-
//
153-
// // Rewrite the query before execution
154-
// QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
155-
//
156-
// // Execute the rewritten query and validate
157-
// Query query = rewrittenQueryBuilder.toQuery(context);
158-
// assertNotNull(query);
159-
// assertThat(query, instanceOf(Query.class));
160-
// }
161-
//
162-
//
163-
//
164-
// public void testQueryClauseReturnsNoResults() throws Exception {
165-
// // Setup TermsLookup with a query clause that returns no results
166-
// QueryBuilder queryBuilder = QueryBuilders.matchQuery("name", "NonExistentClass");
167-
// TermsLookup termsLookup = new TermsLookup("classes", "101", "enrolled").query(queryBuilder);
168-
// TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
169-
//
170-
// // Create real IndexMetadata with valid settings
171-
// Settings indexSettings = Settings.builder()
172-
// .put("index.query.bool.max_clause_count", 1024) // Example setting
173-
// .put("index.version.created", 8000099) // Add the required index.version.created setting
174-
// .build();
175-
// IndexMetadata indexMetadata = IndexMetadata.builder("test_index")
176-
// .settings(indexSettings)
177-
// .numberOfShards(1)
178-
// .numberOfReplicas(1)
179-
// .build();
180-
//
181-
// // Create IndexSettings using the IndexMetadata
182-
// IndexSettings settings = new IndexSettings(indexMetadata, indexSettings);
183-
//
184-
// // Mock QueryShardContext
185-
// QueryShardContext context = mock(QueryShardContext.class);
186-
// when(context.getIndexSettings()).thenReturn(settings); // Return the real IndexSettings
187-
//
188-
// // Rewrite the query before execution
189-
// QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
190-
//
191-
// // Check the type of the rewritten query
192-
// if (rewrittenQueryBuilder instanceof MatchNoneQueryBuilder) {
193-
// // Handle the case where the query rewrites to MatchNoneQueryBuilder
194-
// assertNotNull(rewrittenQueryBuilder);
195-
// assertThat(rewrittenQueryBuilder, instanceOf(MatchNoneQueryBuilder.class));
196-
// } else if (rewrittenQueryBuilder instanceof TermsQueryBuilder) {
197-
// // Execute the query and validate
198-
// Query query = rewrittenQueryBuilder.toQuery(context);
199-
// assertNotNull(query);
200-
// assertThat(query, instanceOf(Query.class));
201-
// } else {
202-
// throw new IllegalStateException("Unexpected query type: " + rewrittenQueryBuilder.getClass().getName());
203-
// }
204-
// }
205-
//
206-
// public void testTermsQueryWithInsertedData() throws Exception {
207-
// // Setup TermsLookup with a valid id and query clause
208-
// QueryBuilder queryBuilder = QueryBuilders.matchQuery("name", "CS101");
209-
// TermsLookup termsLookup = new TermsLookup("classes", "101", "enrolled").query(queryBuilder);
210-
// TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
211-
//
212-
// // Mock data insertion
213-
// String[] enrolledStudents = { "1", "2" };
214-
// String[] allStudents = { "1", "2", "3" };
215-
//
216-
// // Simulate query execution
217-
// boolean queryMatches = false;
218-
// for (String student : allStudents) {
219-
// for (String enrolled : enrolledStudents) {
220-
// if (student.equals(enrolled)) {
221-
// queryMatches = true;
222-
// break;
223-
// }
224-
// }
225-
// }
226-
//
227-
// // Validate the output
228-
// assertTrue("Query should match enrolled students", queryMatches);
229-
// }
230-
//
231-
//
232-
//
233-
// public void testTermsQueryWithIdOnly() throws Exception {
234-
// // Setup TermsLookup with a valid id
235-
// TermsLookup termsLookup = new TermsLookup("classes", "101", "enrolled");
236-
// TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
237-
//
238-
// // Mock QueryShardContext
239-
// QueryShardContext context = mock(QueryShardContext.class);
240-
// when(context.getIndexSettings()).thenReturn(null); // Mock index settings if needed
241-
//
242-
// // Rewrite the query before execution
243-
// QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
244-
//
245-
// // Execute the rewritten query and validate
246-
// Query query = rewrittenQueryBuilder.toQuery(context);
247-
// assertNotNull(query);
248-
// assertThat(query, instanceOf(Query.class));
249-
// }
250-
//
251-
// public void testTermsQueryWithQueryOnly() throws Exception {
252-
// // Setup TermsLookup with a query clause
253-
// QueryBuilder queryBuilder = QueryBuilders.matchQuery("name", "CS101");
254-
// TermsLookup termsLookup = new TermsLookup("classes", null, "enrolled").query(queryBuilder);
255-
// TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
256-
//
257-
// // Mock QueryShardContext
258-
// QueryShardContext context = mock(QueryShardContext.class);
259-
// when(context.getIndexSettings()).thenReturn(null); // Mock index settings if needed
260-
//
261-
// // Rewrite the query before execution
262-
// QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
263-
//
264-
// // Execute the rewritten query and validate
265-
// Query query = rewrittenQueryBuilder.toQuery(context);
266-
// assertNotNull(query);
267-
// assertThat(query, instanceOf(Query.class));
268-
// }
269-
//
270-
// public void testTermsQueryWithIdAndQuery() {
271-
// // Setup TermsLookup with both id and query (invalid case)
272-
// QueryBuilder queryBuilder = QueryBuilders.matchQuery("name", "CS101");
273-
// TermsLookup termsLookup = new TermsLookup("classes", "101", "enrolled").query(queryBuilder);
274-
//
275-
// // Expect an exception during query execution
276-
// IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
277-
// QueryBuilders.termsQuery("student_id", termsLookup);
278-
// });
279-
//
280-
// // Validate the exception message
281-
// assertEquals("[terms_lookup] cannot specify both 'id' and 'query'.", e.getMessage());
282-
// }
283-
//
284-
// public void testTermsQueryWithNoIdAndNoQuery() {
285-
// // Setup TermsLookup with neither id nor query (invalid case)
286-
// TermsLookup termsLookup = new TermsLookup("classes", null, "enrolled");
287-
//
288-
// // Expect an exception during query execution
289-
// IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
290-
// QueryBuilders.termsQuery("student_id", termsLookup);
291-
// });
292-
//
293-
// // Validate the exception message
294-
// assertEquals("[terms_lookup] requires either 'id' or 'query' to be specified.", e.getMessage());
295-
// }
296-
//
297-
// public void testTermsQueryWithComplexQuery() throws Exception {
298-
// // Setup TermsLookup with a complex query clause
299-
// QueryBuilder queryBuilder = QueryBuilders.boolQuery()
300-
// .must(QueryBuilders.matchQuery("name", "CS101"))
301-
// .filter(QueryBuilders.rangeQuery("enrolled").gte(100).lte(200));
302-
// TermsLookup termsLookup = new TermsLookup("classes", null, "enrolled").query(queryBuilder);
303-
// TermsQueryBuilder termsQueryBuilder = QueryBuilders.termsQuery("student_id", termsLookup);
304-
//
305-
// // Mock QueryShardContext
306-
// QueryShardContext context = mock(QueryShardContext.class);
307-
// when(context.getIndexSettings()).thenReturn(null); // Mock index settings if needed
308-
//
309-
// // Rewrite the query before execution
310-
// QueryBuilder rewrittenQueryBuilder = termsQueryBuilder.rewrite(context);
311-
//
312-
// // Execute the rewritten query and validate
313-
// Query query = rewrittenQueryBuilder.toQuery(context);
314-
// assertNotNull(query);
315-
// assertThat(query, instanceOf(Query.class));
316-
// }
317-
//
318-
// }
242+
243+
// testEnhancedTermsLookupWithQueryClause
244+
// testQueryClauseReturnsNoResults
245+
// testTermsQueryWithInsertedData
246+
// testTermsQueryWithIdOnly
247+
// testTermsQueryWithQueryOnly
248+
// testTermsQueryWithIdAndQuery
249+
// testTermsQueryWithNoIdAndNoQuery
250+
// testTermsQueryWithComplexQuery

0 commit comments

Comments
 (0)