-
Notifications
You must be signed in to change notification settings - Fork 234
feat(compass-collection): Preview Screen for Mock Data Generator CLOUDP-333857 #7433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…or API - Change 'sample_values' to 'sampleValues' in FieldInfo interface to match backend API expectations - Update all test files to use the new field name - Resolves 400 Bad Request error when generating faker mappings The backend API expects 'sampleValues' (camelCase) but frontend was sending 'sample_values' (snake_case), causing the LLM request to fail with 'Invalid request' error. Fixes CLOUDP-350280
…or API - Change 'sample_values' to 'sampleValues' in FieldInfo interface to match backend API expectations - Update all test files to use the new field name - Resolves 400 Bad Request error when generating faker mappings The backend API expects 'sampleValues' (camelCase) but frontend was sending 'sample_values' (snake_case), causing the LLM request to fail with 'Invalid request' error. Fixes CLOUDP-350280
expect(document).to.be.an('object'); | ||
expect(document).to.have.property('tags'); | ||
expect(document.tags).to.be.an('array').with.length(2); | ||
(document.tags as string[]).forEach((tag: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]: for (const tag of document.tags)
makes it a bit more readable. Same at other places
// Default to 1.0 for invalid probability values | ||
let probability = 1.0; | ||
if ( | ||
mapping.probability !== undefined && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]: we can remove this extra guard in favour of number check?
if (probability < 1.0) { | ||
// Use Math.random for conditional field inclusion | ||
if (Math.random() < probability) { | ||
const fakerValue = generateFakerValue(mapping); | ||
if (fakerValue !== undefined) { | ||
result[fieldName] = fakerValue; | ||
} | ||
} | ||
} else { | ||
// Normal field inclusion | ||
const fakerValue = generateFakerValue(mapping); | ||
if (fakerValue !== undefined) { | ||
result[fieldName] = fakerValue; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This make it slightly more readable
if (probability < 1.0) { | |
// Use Math.random for conditional field inclusion | |
if (Math.random() < probability) { | |
const fakerValue = generateFakerValue(mapping); | |
if (fakerValue !== undefined) { | |
result[fieldName] = fakerValue; | |
} | |
} | |
} else { | |
// Normal field inclusion | |
const fakerValue = generateFakerValue(mapping); | |
if (fakerValue !== undefined) { | |
result[fieldName] = fakerValue; | |
} | |
} | |
const shouldIncludeField = probability >= 1.0 || Math.random() < probability; | |
const fakerValue = generateFakerValue(mapping); | |
if (fakerValue !== undefined && shouldIncludeField) { | |
result[fieldName] = fakerValue; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
try { | ||
if ('mongoType' in elementType) { | ||
// Array of primitives | ||
const mapping = elementType as FakerFieldMapping; | ||
const value = generateFakerValue(mapping); | ||
if (value !== undefined) { | ||
result.push(value); | ||
} | ||
} else if ('type' in elementType && elementType.type === 'array') { | ||
// Nested array (e.g., matrix[][]) - append another [] to the path | ||
const fieldPath = currentFieldPath + '[]'; | ||
const nestedArrayValue = constructArrayValues( | ||
elementType as ArrayStructure, | ||
fieldName, | ||
arrayLengthMap, | ||
fieldPath | ||
); | ||
result.push(nestedArrayValue); | ||
} else { | ||
// Array of objects | ||
const objectValue = constructDocumentValues( | ||
elementType as DocumentStructure, | ||
arrayLengthMap, | ||
currentFieldPath | ||
); | ||
result.push(objectValue); | ||
} | ||
} catch { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can not we reuse constructDocumentValues
here to get the data for the array? Array eventually will be a list of scalar or nested array or document - which is all handled by constructDocumentValues
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this, but to reuse constructDocumentValues
, we'd have to wrap elements in a temporary structure like { element: elementType }
and then also extract the result after calling. This creates extra object overhead, and we'd have to break it up by case anyway due to having to conditionally construct the temp structures
script | ||
</Body> | ||
<Code language="javascript" copyable={false}> | ||
{JSON.stringify(sampleDocuments, null, 2)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON can't represent a lot of BSON values, you probably don't ever want to use JSON.stringify()
to display BSON documents.
Using EJSON.stringify()
is the lazy workaround for that, but practically speaking, we would strongly prefer something like using DocumentList
from compass-components
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcobis If you really want to stick to EJSON.stringify()
for now, can you leave a TODO item + JIRA ticket to use DocumentList
instead?
...es/compass-collection/src/components/mock-data-generator-modal/mock-data-generator-modal.tsx
Show resolved
Hide resolved
…godb-js/compass into mock-data-generator-preview-screen-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Implements the Preview Screen for the Mock Data Generator feature by updating schema field property naming and adding document generation capabilities.
- Updated field property naming from
sample_values
tosampleValues
for consistency with JavaScript naming conventions - Implemented document generation functionality with
generateDocument
function and related utilities - Added PreviewScreen component to display sample generated documents
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
transform-schema-to-field-info.ts | Updated property name from sample_values to sampleValues |
transform-schema-to-field-info.spec.ts | Updated test expectations to use sampleValues property |
schema-analysis-types.ts | Updated type definition and documentation to use sampleValues |
to-simplified-field-info.spec.ts | Updated test data to use sampleValues property |
script-generation-utils.ts | Added document generation functions and faker execution utilities |
script-generation-utils.spec.ts | Added comprehensive tests for document generation functionality |
preview-screen.tsx | New component that displays generated sample documents |
mock-data-generator-modal.tsx | Integrated PreviewScreen component into modal workflow |
mock-data-generator-modal.spec.tsx | Updated test data to use sampleValues property |
collection-header.spec.tsx | Updated test data to use sampleValues property |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Description
Implements the Preview Screen for the Mock Data Generator feature.
Basing partially off this old draft PR, restarting due to lots of changes to main branch since
Checklist
Types of changes