fix(gen-ai): parse responses individually for find and aggregate COMPASS-10176 #7645
Conversation
mabaasit
added
feature flagged
There was a problem hiding this comment.
Pull request overview
This PR modifies the generative AI query parsing logic to handle find and aggregate operations separately, ensuring each operation type receives only its relevant response fields.
Key Changes:
- Modified
parseXmlToJsonResponseto accept atypeparameter ('find' or 'aggregate') to determine response structure - For 'aggregate' type, only the aggregation pipeline is returned; for 'find' type, both query and aggregation fields are returned
- Updated all call sites to pass the appropriate type parameter
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/compass-generative-ai/src/utils/parse-xml-response.ts | Refactored to accept type parameter and conditionally return response fields based on operation type |
| packages/compass-generative-ai/src/utils/parse-xml-response.spec.ts | Restructured tests into separate contexts for 'find' and 'aggregate' operations with updated expectations |
| packages/compass-generative-ai/src/atlas-ai-service.ts | Updated call sites to pass the type parameter ('find' or 'aggregate') to parseXmlToJsonResponse |
| packages/compass-generative-ai/src/atlas-ai-service.spec.ts | Added aggregation field to test expectation to match new response structure |
| limit: '10', | ||
| context('handles aggregate', function () { | ||
| const options = { logger: createNoopLogger(), type: 'aggregate' as const }; | ||
| it('returns empty pipeline its not available in the response', function () { |
There was a problem hiding this comment.
Corrected spelling: 'its' should be 'if it's' or 'when it's' for grammatical correctness.
| it('returns empty pipeline its not available in the response', function () { | |
| it("returns empty pipeline if it's not available in the response", function () { |
| : {}), | ||
| ...(isQueryEmpty ? {} : { query }), | ||
| query, | ||
| aggregation, |
There was a problem hiding this comment.
For the 'find' type, returning both query and aggregation fields means aggregation will always be included even when not relevant to find operations. Consider if the find path should only return query-related fields to maintain a clearer separation between operation types, similar to how aggregate only returns aggregation.
| aggregation, |
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes