Skip to content

Conversation

@JoonLeeNIH
Copy link
Collaborator

@JoonLeeNIH JoonLeeNIH requested a review from jonkiky June 30, 2025 14:55
@jonkiky jonkiky requested a review from Copilot June 30, 2025 19:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses the INS-1421 ticket by refining the logic in the query generation to account for additional filtering criteria.

  • Updated query condition to include the existence of a filter in addition to the existing "must" conditions.

}

if (compoundQuery.bool.must.length > 0) {
if (compoundQuery.bool.must.length > 0 || compoundQuery.bool.filter) {
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider explicitly checking if compoundQuery.bool.filter contains usable values (e.g., has a length or is non-empty) to avoid passing an empty or undefined filter into the query.

Suggested change
if (compoundQuery.bool.must.length > 0 || compoundQuery.bool.filter) {
if (compoundQuery.bool.must.length > 0 || (Array.isArray(compoundQuery.bool.filter) && compoundQuery.bool.filter.length > 0) || (typeof compoundQuery.bool.filter === 'object' && Object.keys(compoundQuery.bool.filter).length > 0)) {

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the time line 318 is reached, compoundQuery.bool.filter is either undefined or an array, because of getFiltersClause() in line 152. Therefore, it's unnecessary to check whether it's an array.

If compoundQuery.bool.filter isn't undefined, then it must have length greater than 0, because of line 159. Therefore, it's unnecessary to check whether its length is greater than 0.

Lastly, compoundQuery.bool.filter isn't a map (object). Therefore, it's irrelevant to check whether it has at least one key.

I reject Copilot's suggestion. It suffices to check whether compoundQuery.bool.filter is truthy.

@jonkiky jonkiky merged commit a203b46 into 3.2.0 Jul 2, 2025
1 of 2 checks passed
@JoonLeeNIH JoonLeeNIH deleted the INS-1421 branch July 2, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants