feat: improve opensearch - #2889
Open
Junjiequan wants to merge 22 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
TransportTypeimport inapp.module.tsuses a hard-codednode_modulespath, which is brittle and may break with dependency changes; prefer importing directly from@nestjs-modules/mailer’s public API instead. - In
OpensearchService.search/runSearch, theSearchParams.limitdefault andsize: limit - skipcalculation can yield confusing behavior whenskipis non-zero; consider cappingsizeexplicitly to the remaining window and documenting the semantics oflimitvsskip.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `TransportType` import in `app.module.ts` uses a hard-coded `node_modules` path, which is brittle and may break with dependency changes; prefer importing directly from `@nestjs-modules/mailer`’s public API instead.
- In `OpensearchService.search` / `runSearch`, the `SearchParams.limit` default and `size: limit - skip` calculation can yield confusing behavior when `skip` is non-zero; consider capping `size` explicitly to the remaining window and documenting the semantics of `limit` vs `skip`.
## Individual Comments
### Comment 1
<location path="src/opensearch/opensearch.service.ts" line_range="293-302" />
<code_context>
+ const { body } = (await this.osClient.search({
+ index,
+ body: {
+ from: skip,
+ size: limit - skip,
+ track_total_hits: this.maxResultWindow,
+ _source: false,
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `size = limit - skip` can produce negative sizes and changes the meaning of `limit`.
Previously, `limit` was the page size and independent of `skip`. Computing `size = limit - skip` means `size` can become negative when `skip > limit` (likely causing runtime errors) and also changes `limit` to behave like an end index instead of a page size. Please either keep `size = limit` and enforce `from + size <= maxResultWindow`, or add a guard for `limit <= skip` and clearly document the new semantics of `limit`.
</issue_to_address>
### Comment 2
<location path="src/opensearch/opensearch.service.ts" line_range="31-36" />
<code_context>
+import { BulkStats } from "@opensearch-project/opensearch/lib/Helpers.js";
+import { Sort } from "@opensearch-project/opensearch/api/_types/_common.js";
+
+export interface SearchParams {
+ filter: ISearchFilter;
+ index?: string;
+ limit?: number;
+ skip?: number;
+ sort?: Record<string, "asc" | "desc">[];
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `SearchParams.sort` type does not match the `Sort` type actually used.
`SearchParams.sort` is typed as `Record<string, "asc" | "desc">[]`, but `runSearch` uses OpenSearch’s `Sort` type and defaults to `[{ _score: "desc" }, { _id: "asc" }]`. This mismatch can let callers pass shapes OpenSearch will reject. Please change `SearchParams.sort` to use `Sort` so the parameter and implementation share the same type.
Suggested implementation:
```typescript
import type { IndexSettings } from "@opensearch-project/opensearch/api/_types/indices._common.js";
import { ISearchFilter } from "./interfaces/os-common.type";
import { CreateIndexDto } from "./dto/create-index.dto";
import { UpdateIndexDto } from "./dto/update-index.dto";
import type { TypeMapping } from "@opensearch-project/opensearch/api/_types/_common.mapping.js";
import { Readable } from "stream";
import { BulkStats } from "@opensearch-project/opensearch/lib/Helpers.js";
import type { Sort } from "@opensearch-project/opensearch/api/_types/_common.js";
import {
SearchMode,
SearchQueryService,
} from "./providers/query-builder.service";
import {
DatasetClass,
import { ConfigService } from "@nestjs/config";
import { sleep } from "src/common/utils";
```
```typescript
+export interface SearchParams {
+ filter: ISearchFilter;
+ index?: string;
+ limit?: number;
+ skip?: number;
+ sort?: Sort;
+}
```
If `SearchParams` is used elsewhere (e.g. in `runSearch` or controller methods), no further changes should be needed as long as those call sites already pass values compatible with OpenSearch's `Sort` type (such as `[{ _score: "desc" }, { _id: "asc" }]`). If any call sites were relying on the old `Record<string, "asc" | "desc">[]` typing with shapes that are not valid `Sort`, they will now surface type errors that should be corrected to match OpenSearch’s expected sort format.
</issue_to_address>
### Comment 3
<location path="src/opensearch/opensearch.service.ts" line_range="296" />
<code_context>
- limit = 1000,
- skip = 0,
- ): Promise<{ totalCount: number; data: (string | undefined)[] }> {
+ async search(params: SearchParams): Promise<SearchResult> {
try {
- const searchQuery = this.searchService.buildSearchQuery(filter);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the search and bulk helper logic to separate request construction, execution, and data source handling so each method has a narrower responsibility and is easier to reason about.
### `search` / `runSearch`
You can keep the fast/wildcard behavior and the `SearchParams`/`SearchResult` types while reducing coupling and removing the `unknown` cast by separating *request building* from *execution* and simplifying paging.
Key ideas:
- `buildSearchRequest(params, mode)` returns a typed `{ index, body }`.
- `runSearch` only calls `osClient.search` and normalizes the response.
- Use `from = skip`, `size = limit` instead of `size = limit - skip`.
Example refactor (core pieces only):
```ts
type OsSearchResponseBody = {
hits: {
total?: number | { value: number };
hits: { _id: string }[];
};
};
interface BuiltSearchRequest {
index: string;
body: {
from: number;
size: number;
track_total_hits: number;
_source: false;
query: unknown;
sort: Sort;
};
}
private buildSearchRequest(
params: SearchParams,
mode: SearchMode,
): BuiltSearchRequest {
const defaultSort: Sort = [{ _score: "desc" }, { _id: "asc" }];
const {
filter,
index = this.defaultIndex,
limit = this.maxResultWindow,
skip = 0,
sort = defaultSort,
} = params;
return {
index,
body: {
from: skip,
size: limit, // simpler paging: size = limit
track_total_hits: this.maxResultWindow,
_source: false,
query: this.searchService.buildQuery(filter, mode),
sort,
},
};
}
private async runSearch(
params: SearchParams,
mode: SearchMode,
): Promise<SearchResult> {
const { index, body } = this.buildSearchRequest(params, mode);
const { body: res } = await this.osClient.search<OsSearchResponseBody>({
index,
body,
});
const total = res.hits.total;
return {
totalCount: typeof total === "number" ? total : (total?.value ?? 0),
hits: res.hits.hits.map((h) => h._id),
};
}
async search(params: SearchParams): Promise<SearchResult> {
try {
const fast = await this.runSearch(params, "fast");
if (fast.totalCount > 0) return fast;
return await this.runSearch(params, "wildcard");
} catch (error) {
throw new HttpException(
`search failed -> OpensearchService ${error}`,
HttpStatus.BAD_REQUEST,
);
}
}
```
This keeps all current behavior (fast mode, wildcard fallback, sorting, max result window) while making `runSearch` narrower and eliminating `as unknown` casting.
### `performBulkOperation`
You’re combining: source type handling, transformation, progress, failure aggregation, and logging into one helper. You can keep all features but make the core bulk helper simpler by:
1. Normalizing the `datasource` at the call site (or in a tiny wrapper) to a single `AsyncIterable<T>`.
2. Moving logging / reason aggregation to a dedicated helper, so `performBulkOperation` just wires the bulk helper and returns stats.
3. Keeping `transform` / `onProgress` available as thin wrappers around the core.
Example split:
```ts
private toAsyncIterable<T>(
datasource: T[] | Readable | AsyncIterator<T>,
): AsyncIterable<T> {
if (Array.isArray(datasource)) {
return (async function* () {
for (const item of datasource) yield item;
})();
}
if (Symbol.asyncIterator in datasource) {
return datasource as AsyncIterable<T>;
}
// Readable -> AsyncIterable
return (async function* () {
for await (const chunk of datasource as Readable) yield chunk as T;
})();
}
private async performBulkCore<T extends { _id: unknown }>(
datasource: AsyncIterable<T>,
index: string,
): Promise<BulkStats> {
return this.osClient.helpers.bulk({
datasource,
flushBytes: 5_000_000,
concurrency: 5,
retries: 5,
wait: 10000,
onDocument(doc: T) {
const { _id: mongoId, ...body } = doc;
return [
{ index: { _index: index, _id: String(mongoId) } },
body,
];
},
});
}
```
Then your current richer behavior can be layered on top in a thin wrapper:
```ts
async performBulkOperation<T extends { _id: unknown }>(
datasource: T[] | Readable | AsyncIterator<T>,
index: string,
transform: (doc: Omit<T, "_id">) => Record<string, unknown> = (d) => d,
onProgress?: (count: number) => void,
): Promise<BulkStats> {
const iterable = this.toAsyncIterable(datasource);
const dropped: string[] = [];
const reasons = new Map<string, number>();
let processed = 0;
const stats = await this.osClient.helpers.bulk({
datasource: iterable,
flushBytes: 5_000_000,
concurrency: 5,
retries: 5,
wait: 10000,
onDocument(doc: T) {
const { _id: mongoId, ...body } = doc;
processed++;
if (onProgress && processed % 10_000 === 0) onProgress(processed);
return [
{ index: { _index: index, _id: String(mongoId) } },
transform(body as Omit<T, "_id">),
];
},
onDrop(doc) {
const id =
(doc.operation as { index?: { _id?: string } } | undefined)?.index?._id ??
"unknown";
const reason = (doc.error?.reason ?? doc.error?.type ?? "unknown")
.replace(/ in document with id '[^']*'/, "")
.replace(/\. Preview of field's value: '.*'$/, "");
reasons.set(reason, (reasons.get(reason) ?? 0) + 1);
dropped.push(id);
},
});
this.logBulkFailures(reasons, dropped);
return stats;
}
private logBulkFailures(reasons: Map<string, number>, dropped: string[]): void {
for (const [reason, count] of [...reasons].sort((a, b) => b[1] - a[1])) {
Logger.error(
`${count} × ${reason}, failed ids: ${dropped.slice(0, 10)}...`,
"OpensearchService",
);
}
}
```
This keeps streaming support, transformation, progress reporting, and diagnostic logging, but the core bulk helper is now easier to reason about, and the more complex concerns are clearly separated.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Junjiequan
force-pushed
the
improve-open-search
branch
2 times, most recently
from
August 18, 2026 08:26
a04578c to
9b77dd6
Compare
Junjiequan
force-pushed
the
improve-open-search
branch
from
August 19, 2026 14:47
e1ced50 to
604b3ae
Compare
nitrosx
requested changes
Aug 20, 2026
Comment on lines
+1188
to
+1285
| export const opensearchIndexMappingsExample = { | ||
| dynamic: false, | ||
| properties: { | ||
| all_text: { | ||
| type: "text", | ||
| analyzer: "autocomplete", | ||
| search_analyzer: "autocomplete_search", | ||
| fields: { | ||
| wild: { type: "wildcard" }, | ||
| }, | ||
| }, | ||
| isPublished: { type: "boolean" }, | ||
| ownerGroup: { type: "keyword" }, | ||
| accessGroups: { type: "keyword" }, | ||
|
|
||
| pid: { type: "keyword", copy_to: "all_text" }, | ||
| owner: { type: "keyword", copy_to: "all_text" }, | ||
| ownerEmail: { type: "keyword", copy_to: "all_text" }, | ||
| contactEmail: { type: "keyword", copy_to: "all_text" }, | ||
| sourceFolder: { type: "keyword", copy_to: "all_text" }, | ||
| type: { type: "keyword", copy_to: "all_text" }, | ||
| keywords: { type: "keyword", copy_to: "all_text" }, | ||
| description: { type: "keyword", copy_to: "all_text" }, | ||
| datasetName: { type: "keyword", copy_to: "all_text" }, | ||
| classification: { type: "keyword", copy_to: "all_text" }, | ||
| version: { type: "keyword", copy_to: "all_text" }, | ||
| createdBy: { type: "keyword", copy_to: "all_text" }, | ||
| updatedBy: { type: "keyword", copy_to: "all_text" }, | ||
| creationLocation: { type: "keyword", copy_to: "all_text" }, | ||
| proposalIds: { type: "keyword", copy_to: "all_text" }, | ||
| instrumentIds: { type: "keyword", copy_to: "all_text" }, | ||
| sampleIds: { type: "keyword", copy_to: "all_text" }, | ||
| techniques: { | ||
| properties: { | ||
| pid: { type: "keyword", copy_to: "all_text" }, | ||
| name: { type: "keyword", copy_to: "all_text" }, | ||
| }, | ||
| }, | ||
| principalInvestigators: { type: "keyword", copy_to: "all_text" }, | ||
| creationTime: { type: "date", copy_to: "all_text" }, | ||
| createdAt: { type: "date", copy_to: "all_text" }, | ||
| updatedAt: { type: "date", copy_to: "all_text" }, | ||
| numberOfFiles: { type: "long", copy_to: "all_text" }, | ||
| runNumber: { type: "long", copy_to: "all_text" }, | ||
| size: { type: "long", copy_to: "all_text" }, | ||
| datasetlifecycle: { | ||
| properties: { | ||
| archiveStatusMessage: { type: "keyword", copy_to: "all_text" }, | ||
| retrieveStatusMessage: { type: "keyword", copy_to: "all_text" }, | ||
| }, | ||
| }, | ||
|
|
||
| scientificMetadata: { type: "object", enabled: false }, | ||
| scientificMetadataText: { | ||
| type: "text", | ||
| index: false, | ||
| copy_to: "all_text", | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| export const opensearchIndexSettingsExample = { | ||
| index: { | ||
| max_result_window: 10000, | ||
| number_of_replicas: 0, | ||
| }, | ||
| analysis: { | ||
| analyzer: { | ||
| autocomplete: { | ||
| type: "custom", | ||
| tokenizer: "autocomplete", | ||
| filter: ["word_delimiter", "lowercase"], | ||
| }, | ||
| autocomplete_search: { | ||
| type: "custom", | ||
| tokenizer: "keyword", | ||
| filter: ["lowercase"], | ||
| }, | ||
| }, | ||
| tokenizer: { | ||
| autocomplete: { | ||
| type: "edge_ngram", | ||
| min_gram: 2, | ||
| max_gram: 64, | ||
| token_chars: ["letter", "digit", "symbol", "punctuation"], | ||
| }, | ||
| }, | ||
| filter: { | ||
| word_delimiter: { | ||
| type: "word_delimiter_graph", | ||
| split_on_case_change: false, | ||
| split_on_numerics: false, | ||
| preserve_original: true, | ||
| type_table: [". => ALPHA"], | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
Member
There was a problem hiding this comment.
Are site admins expected to configure this two object to their needs in order to optimize the search to their data?
If so, I would move them to an external json file that is loaded at start time. If the json file does not exists, than you default back to the configuration above.
| "authenticatedUser": "PROPOSALS" | ||
| }, | ||
| "statusBannerMessage": "", | ||
| "addScientificMetadataKeysAsColumn": true, |
Comment on lines
-477
to
-490
| "name": "startTime", | ||
| "name": "creationTime", | ||
| "type": "date", | ||
| "width": 200, | ||
| "enabled": false, | ||
| "format": "yyyy-mm-dd HH:MM", | ||
| "sort": "desc" | ||
| }, | ||
| { | ||
| "name": "endTime", | ||
| "type": "date", | ||
| "width": 200, | ||
| "enabled": false, | ||
| "format": "yyyy-mm-dd HH:MM" | ||
| }, |
Member
There was a problem hiding this comment.
Why are start and end time removed?
| const osResult = await this.opensearchService.search({ | ||
| filter: { text, userGroups, isPublished }, | ||
| index: this.osDefaultIndex, | ||
| skip: modifiers.skip, |
Member
There was a problem hiding this comment.
We should apply the skip only as last step of the query in mongo and not here
Comment on lines
+321
to
+325
| const osResult = await this.opensearchService.search({ | ||
| filter: { text, userGroups, isPublished }, | ||
| index: this.osDefaultIndex, | ||
| skip: modifiers.skip, | ||
| }); |
Member
There was a problem hiding this comment.
Where do we apply the limit on the number of items returned by opensearch?
| import { MetadataKeysModule } from "./metadata-keys/metadatakeys.module"; | ||
| import { OidcClientModule } from "./common/openid-client/openid-client.module"; | ||
| import { ThrottlerModule } from "@nestjs/throttler"; | ||
| import { TransportType } from "node_modules/@nestjs-modules/mailer/dist/interfaces/mailer-options.interface"; |
Member
There was a problem hiding this comment.
Is this change part of this PR?
Member
Code Review Report:
|
| File | Change Type | Description |
|---|---|---|
opensearchConfig.example.json |
Modified | Updated mappings to use all_text catch-all field with copy_to, added wildcard sub-field, changed max_gram from 150 to 64, reduced max_result_window from 2000000 to 10000 |
src/common/utils.ts |
Modified | Added opensearchIndexMappingsExample and opensearchIndexSettingsExample constants for Swagger documentation |
src/app.module.ts |
Modified | Fixed import path for TransportType |
src/config/frontend.config.json |
Modified | Added addScientificMetadataKeysAsColumn: true, reordered date fields |
src/datasets/datasets.service.ts |
Modified | Integrated OpenSearch query with fallback, removed direct DatasetOpenSearchDto field exclusion, streamlined sync |
src/opensearch/opensearch.controller.ts |
Modified | Removed public /opensearch/search endpoint (now internal) |
src/opensearch/opensearch.service.ts |
Modified | Added SearchParams, SearchResult interfaces, maxResultWindow tracking, dual-mode search, streaming bulk operations, improved error handling |
src/opensearch/providers/query-builder.service.ts |
Modified | Complete rewrite: dual-mode query building, proper access filtering, removed hardcoded field dependencies |
src/opensearch/providers/query-builder.service.spec.ts |
Modified | Comprehensive tests for new query builder logic |
src/opensearch/dto/*.ts |
Modified | Updated examples to use shared constants, removed redundant @Expose() decorators |
src/opensearch/utils/opensearch.util.ts |
New | flattenScientificMetadata() and toOpensearchDocument() functions |
src/opensearch/utils/dataset-opensearch.utils.ts |
Modified | Added DATASET_OPENSEARCH_FIELDS constant as single source of truth |
docs/developer-guide/opensearch-guidelines.md |
Modified | Complete documentation overhaul reflecting new architecture |
test/OpenSearch.js |
Modified | Added tests for nested scientific metadata search |
Assessment: Are the changes necessary?
Yes, absolutely. The previous implementation was severely limited:
- Only
datasetNameanddescriptionwere searchable - No support for nested scientific metadata
- No proper handling of large text fields
- Hardcoded field dependencies made customization fragile
The new implementation provides comprehensive text search across all relevant dataset fields with proper handling of edge cases.
Improvement Needed
-
Configuration validation: While the documentation warns about required fields, there's no runtime validation that
opensearchConfig.jsoncontains the required mappings. A startup check could verifyall_text,all_text.wild,isPublished,ownerGroup,accessGroupsexist in the mapping.- Location:
OpensearchService.onModuleInit() - Suggestion: Add validation after loading config:
if (!mappings.properties.all_text) throw new Error("Missing required mapping: all_text")
- Location:
-
Error message clarity: In
OpensearchService.search(), the error messagesearch failed -> OpensearchService ${error}could be more specific. Consider differentiating between connection errors, query errors, and timeout errors. -
Bulk operation logging: The
logBulkFailures()method logs only the first 10 failed IDs. For large failures, consider logging to a file or providing a way to retrieve the full list. -
Memory management: The
flattenScientificMetadata()function collects all values in aSetbefore joining. For extremely large metadata objects, this could consume significant memory. Consider streaming the chunking process. -
Documentation: The
syncDatasetsToOpensearchdescription in the docs mentions it's automatic on startup, but the implementation requires explicit sync. Clarify this. -
Type safety: The
SearchParamsinterface allowslimitandskipto be undefined, which could lead to unintended defaults. Consider making them required or providing explicit default values in the interface documentation.
Verdict
Excellent implementation. The changes are well-architected, thoroughly tested, and address real limitations. The dual-mode search approach is particularly elegant, providing both performance (fast mode) and completeness (wildcard fallback). The scientific metadata flattening is a robust solution to a difficult problem.
Security Review
1. Potential Injection Vulnerabilities
No injection vulnerabilities identified.
- OpenSearch query construction: All user input (
text) is properly escaped. In wildcard mode, special characters (*,?,\) are escaped withreplace(/([*?\\])/g, "\\$1"). In fast mode,simple_query_stringwithflags: "WHITESPACE"anddefault_operator: "and"prevents query injection by treating the input as a literal string to be analyzed, not as query syntax. - MongoDB queries: User input is passed through established query builders (
createFullqueryFilter,createFullfacetPipeline) that use parameterized queries, not string concatenation.
2. Sensitive User Data Exposure
No sensitive data exposure identified.
- OpenSearch returns only document IDs (
_id), not the full documents (_source: false). The actual dataset data is fetched from MongoDB using these IDs, maintaining the existing access control mechanisms. - The
DATASET_OPENSEARCH_FIELDSexplicitly excludes sensitive fields. No fields containing credentials, tokens, or personally identifiable information beyond what's already in the dataset metadata are indexed. - Access filtering (
isPublished,ownerGroup,accessGroups) is applied at the OpenSearch query level, ensuring users only see datasets they're authorized to access.
3. Insecure API Usage
No insecure API usage identified.
- OpenSearch client uses HTTPS (assuming
OPENSEARCH_HOSTis configured with HTTPS) - Authentication is via username/password (basic auth over HTTPS)
- The removed
/opensearch/searchendpoint was public and admin-only; its removal actually improves security by forcing all searches through the authorizedDatasetsServicemethods
4. Authentication Bypass
No authentication bypass vulnerabilities identified.
- Access control is properly implemented at multiple layers:
- Controller level: CASL policies via
@CheckPolicies()decorators - Service level:
DatasetsService.opensearchQuery()only proceeds ifthis.isOsEnabledand other conditions are met - Query level:
SearchQueryService.buildQuery()includes access filters in the OpenSearch query itself
- Controller level: CASL policies via
- The fallback to MongoDB maintains the same access control checks
Commits responsible: 6007f53 (fixed access logic)
Test Coverage
Current Coverage
The test coverage is good and has been significantly improved:
-
Unit tests:
query-builder.service.spec.tsnow has comprehensive coverage:- Query building with text and filters
- Empty text handling
- Unrestricted access (no filters)
- Published-only filtering
- Fast mode query structure
- Wildcard mode query structure
- Special character escaping
- Whitespace-only queries
-
Integration tests:
test/OpenSearch.jshas been enhanced with:- Nested scientific metadata search (3 levels deep)
- Nested scientific metadata search (4 levels deep)
- Top-level scientific metadata key search
- Negative test (ensuring dataset2 doesn't match dataset1's metadata)
- Proper cleanup in
afterhook
Gaps Identified
-
No tests for
toOpensearchDocument(): The document transformation function should have unit tests verifying:scientificMetadataTextis properly generated- All fields from
DATASET_OPENSEARCH_FIELDSare preserved _idis properly deleted
-
No tests for
flattenScientificMetadata(): Should test:- Simple nested objects
- Arrays at various levels
- Null/undefined values
- Boolean values (should be ignored)
- Circular references (potential edge case)
- Very large objects (chunking)
-
No tests for streaming bulk operations: The new
performBulkOperation()withReadable/AsyncIteratorsupport lacks tests. -
No tests for fallback behavior: When OpenSearch is disabled or returns no results, tests should verify the fallback to MongoDB.
-
No tests for access filtering in OpenSearch queries: Tests should verify that:
- Anonymous users only see published datasets
- Authenticated users see datasets in their groups
- Admin users see all datasets
- The
minimum_should_match: 1logic works correctly
-
No tests for result window clamping: Should verify that
sizeis properly clamped tomax_result_window - skip.
Suggestions for Improvement
// Example test for toOpensearchDocument
describe("toOpensearchDocument", () => {
it("should flatten scientificMetadata to scientificMetadataText", () => {
const doc = {
pid: "123",
scientificMetadata: { instrument: { model: "X" } }
};
const result = toOpensearchDocument(doc);
expect(result.scientificMetadataText).toContain("instrument");
expect(result.scientificMetadataText).toContain("model");
expect(result.scientificMetadataText).toContain("X");
});
it("should delete _id field", () => {
const doc = { _id: "mongo123", pid: "123" };
const result = toOpensearchDocument(doc);
expect(result._id).toBeUndefined();
expect(result.pid).toBe("123");
});
});
// Example test for access filtering
describe("access filtering", () => {
it("should apply isPublished filter for anonymous users", () => {
const query = service.buildQuery({
text: "test",
isPublished: true,
userGroups: undefined
}, "fast");
expect(query.bool?.filter).toContainEqual({
term: { isPublished: true }
});
});
});Relevant commits: 745a0cd (fix tests), all commits contribute to testable functionality
Security Examples
Empty section - No vulnerabilities were identified in the Security Review.
Testing for Security Use Cases
Not applicable - No vulnerabilities were identified in the Security Review. The existing test suite, combined with the suggestions above, would provide adequate coverage for security-relevant functionality.
Summary
Key Findings
-
Architecture: The changes represent a major, well-executed improvement to OpenSearch integration, moving from limited field search to comprehensive text search with intelligent fallback mechanisms.
-
Logic: All core logic is correct. The dual-mode search (fast + wildcard fallback), scientific metadata flattening, access filtering, and fallback to MongoDB are all properly implemented.
-
Edge Cases: The implementation handles numerous edge cases well: empty queries, special characters, nested data, large text fields, stream-based operations, and bulk failures.
-
Security: No vulnerabilities were identified. The implementation maintains proper access control at all levels and safely handles user input.
-
Testing: Unit and integration tests are good but could be expanded to cover document transformation, flattening, streaming, fallback behavior, and access filtering scenarios.
-
Documentation: The documentation has been thoroughly updated to reflect the new architecture and includes important warnings about required field mappings.
Recommendations
- High Priority: Add runtime validation of
opensearchConfig.jsonrequired fields during startup. - Medium Priority: Add unit tests for
toOpensearchDocument()andflattenScientificMetadata(). - Medium Priority: Add integration tests for fallback behavior and access filtering.
- Low Priority: Consider adding memory-efficient streaming for very large metadata objects.
- Low Priority: Clarify in documentation that sync requires explicit invocation (not automatic on startup).
Overall Assessment
The improve-open-search branch introduces significant, well-designed improvements that address critical limitations of the previous OpenSearch integration. The implementation is robust, secure, and well-tested. The changes are production-ready with only minor improvements suggested.
The most impactful improvements are:
- Comprehensive text search across all dataset fields
- Support for nested scientific metadata search
- Dual-mode search with automatic fallback
- Memory-efficient streaming sync
- Proper access control integration
Verdict: APPROVE with minor suggestions for additional validation and testing.
Generated by Mistral Vibe.
Co-Authored-By: Mistral Vibe vibe@mistral.ai
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
2 participants
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.
Description
Motivation
Fixes
Changes:
Tests included
Documentation
official documentation info
Summary by Sourcery
Improve Opensearch integration for dataset searching, indexing, and configuration examples.
New Features:
Bug Fixes:
Enhancements:
Documentation: