-
Notifications
You must be signed in to change notification settings - Fork 231
feat(compass-collection): Output Validation Followup - Mock Data Generator LLM CLOUDP-347048 #7365
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?
Changes from 1 commit
d9f6a23
406aa2b
0b079fb
df49911
b471586
06258a8
84ee0cf
a8b814b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,12 @@ import { | |
palette, | ||
Select, | ||
spacing, | ||
TextInput, | ||
} from '@mongodb-js/compass-components'; | ||
import React from 'react'; | ||
import { UNRECOGNIZED_FAKER_METHOD } from '../../modules/collection-tab'; | ||
import type { MongoDBFieldType } from '@mongodb-js/compass-generative-ai'; | ||
import type { FakerArg } from './script-generation-utils'; | ||
|
||
const fieldMappingSelectorsStyles = css({ | ||
width: '50%', | ||
|
@@ -24,16 +26,106 @@ const labelStyles = css({ | |
fontWeight: 600, | ||
}); | ||
|
||
/** | ||
* Renders read-only TextInput components for each key-value pair in a faker arguments object. | ||
*/ | ||
const getFakerArgsInputFromObject = (fakerArgsObject: Record<string, any>) => { | ||
return Object.entries(fakerArgsObject).map(([key, item]: [string, any]) => { | ||
if (typeof item === 'string' || typeof item === 'boolean') { | ||
|
||
return ( | ||
<TextInput | ||
key={`faker-arg-${key}`} | ||
type="text" | ||
label={key} | ||
aria-label={`Faker Arg ${key}`} | ||
readOnly | ||
value={item.toString()} | ||
/> | ||
); | ||
} else if (typeof item === 'number') { | ||
return ( | ||
<TextInput | ||
key={`faker-arg-${key}`} | ||
type="number" | ||
label={key} | ||
aria-label={`Faker Arg ${key}`} | ||
readOnly | ||
value={item.toString()} | ||
/> | ||
); | ||
} else if ( | ||
Array.isArray(item) && | ||
item.length > 0 && | ||
typeof item[0] === 'string' | ||
) { | ||
return ( | ||
<TextInput | ||
key={`faker-arg-${key}`} | ||
type="text" | ||
label={key} | ||
aria-label={`Faker Arg ${key}`} | ||
readOnly | ||
value={item.join(', ')} | ||
/> | ||
); | ||
} | ||
return null; | ||
}); | ||
}; | ||
|
||
/** | ||
* Renders TextInput components for each faker argument based on its type. | ||
*/ | ||
const getFakerArgsInput = (fakerArgs: FakerArg[]) => { | ||
return fakerArgs.map((arg, idx) => { | ||
if (typeof arg === 'string' || typeof arg === 'boolean') { | ||
return ( | ||
<TextInput | ||
key={`faker-arg-${idx}`} | ||
type="text" | ||
label="Faker Arg" | ||
|
||
required | ||
ncarbon marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
value={arg.toString()} | ||
/> | ||
); | ||
} else if (typeof arg === 'number') { | ||
return ( | ||
<TextInput | ||
key={`faker-arg-${idx}`} | ||
type="number" | ||
label="Faker Arg" | ||
readOnly | ||
value={arg.toString()} | ||
/> | ||
); | ||
} else if (typeof arg === 'object' && 'json' in arg) { | ||
// parse the object | ||
let parsedArg; | ||
try { | ||
parsedArg = JSON.parse(arg.json); | ||
} catch { | ||
// If parsing fails, skip rendering this arg | ||
return null; | ||
} | ||
if (typeof parsedArg === 'object') { | ||
return getFakerArgsInputFromObject(parsedArg); | ||
} | ||
} | ||
}); | ||
}; | ||
|
||
interface Props { | ||
activeJsonType: string; | ||
activeFakerFunction: string; | ||
onJsonTypeSelect: (jsonType: MongoDBFieldType) => void; | ||
activeFakerArgs: FakerArg[]; | ||
onFakerFunctionSelect: (fakerFunction: string) => void; | ||
} | ||
|
||
const FakerMappingSelector = ({ | ||
activeJsonType, | ||
activeFakerFunction, | ||
activeFakerArgs, | ||
onJsonTypeSelect, | ||
onFakerFunctionSelect, | ||
}: Props) => { | ||
|
@@ -72,7 +164,7 @@ const FakerMappingSelector = ({ | |
string "Unrecognized" | ||
</Banner> | ||
)} | ||
{/* TODO(CLOUDP-344400): Render faker function parameters once we have a way to validate them. */} | ||
{getFakerArgsInput(activeFakerArgs)} | ||
</div> | ||
); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,7 @@ describe('MockDataGeneratorModal', () => { | |
}, | ||
}; | ||
|
||
// @ts-expect-error // TODO: fix ts error | ||
ncarbon marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
const store = createStore( | ||
collectionTabReducer, | ||
initialState, | ||
|
@@ -538,6 +539,44 @@ describe('MockDataGeneratorModal', () => { | |
); | ||
}); | ||
|
||
it('does not show faker args that are too large', async () => { | ||
const largeLengthArgs = Array.from({ length: 11 }, () => 'test'); | ||
const mockServices = createMockServices(); | ||
mockServices.atlasAiService.getMockDataSchema = () => | ||
Promise.resolve({ | ||
fields: [ | ||
{ | ||
fieldPath: 'name', | ||
mongoType: 'String', | ||
fakerMethod: 'person.firstName', | ||
fakerArgs: [JSON.stringify(largeLengthArgs)], | ||
isArray: false, | ||
probability: 1.0, | ||
}, | ||
], | ||
}); | ||
|
||
await renderModal({ | ||
mockServices, | ||
schemaAnalysis: { | ||
...defaultSchemaAnalysisState, | ||
processedSchema: { | ||
name: { | ||
type: 'String', | ||
probability: 1.0, | ||
}, | ||
}, | ||
sampleDocument: { name: 'Peaches' }, | ||
}, | ||
}); | ||
|
||
// advance to the schema editor step | ||
userEvent.click(screen.getByText('Confirm')); | ||
await waitFor(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems to be an assertion missing? I'm not connecting how "faker args that are too large" are not shown There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I added the assertion in |
||
expect(screen.getByTestId('faker-schema-editor')).to.exist; | ||
}); | ||
}); | ||
|
||
it('disables the Next button when the faker schema mapping is not confirmed', async () => { | ||
await renderModal({ | ||
mockServices: mockServicesWithMockDataResponse, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
import { expect } from 'chai'; | ||
import { areFakerArgsValid, isValidFakerMethod } from './utils'; | ||
|
||
describe('Mock Data Generator Utils', function () { | ||
describe('areFakerArgsValid', () => { | ||
it('returns true for empty array', () => { | ||
expect(areFakerArgsValid([])).to.be.true; | ||
}); | ||
|
||
it('returns false if array length exceeds max faker args length', () => { | ||
const arr = Array(11).fill(1); | ||
expect(areFakerArgsValid(arr)).to.be.false; | ||
}); | ||
|
||
it('returns true for valid numbers, strings, booleans', () => { | ||
expect(areFakerArgsValid([1, 'foo', true, false, 0])).to.be.true; | ||
}); | ||
|
||
it('returns false for non-finite numbers', () => { | ||
expect(areFakerArgsValid([Infinity])).to.be.false; | ||
expect(areFakerArgsValid([NaN])).to.be.false; | ||
}); | ||
|
||
it('returns false for strings exceeding max faker string length', () => { | ||
const longStr = 'a'.repeat(1001); | ||
expect(areFakerArgsValid([longStr])).to.be.false; | ||
}); | ||
|
||
it('returns true for nested valid arrays', () => { | ||
expect( | ||
areFakerArgsValid([ | ||
[1, 'foo', true], | ||
[2, false], | ||
]) | ||
).to.be.true; | ||
}); | ||
|
||
it('returns false for nested arrays exceeding max faker args length', () => { | ||
const nested = [Array(11).fill(1)]; | ||
expect(areFakerArgsValid(nested)).to.be.false; | ||
}); | ||
|
||
it('returns true for valid object with json property', () => { | ||
const obj = { json: JSON.stringify({ a: 1, b: 'foo', c: true }) }; | ||
expect(areFakerArgsValid([obj])).to.be.true; | ||
}); | ||
|
||
it('returns false for object with invalid json property', () => { | ||
const obj = { json: '{invalid json}' }; | ||
expect(areFakerArgsValid([obj])).to.be.false; | ||
}); | ||
|
||
it('returns false for object with json property containing invalid values', () => { | ||
const obj = { json: JSON.stringify({ a: Infinity }) }; | ||
expect(areFakerArgsValid([obj])).to.be.false; | ||
}); | ||
|
||
it('returns false for unrecognized argument types', () => { | ||
expect(areFakerArgsValid([undefined as any])).to.be.false; | ||
expect(areFakerArgsValid([null as any])).to.be.false; | ||
expect(areFakerArgsValid([(() => {}) as any])).to.be.false; | ||
}); | ||
|
||
it('returns true for deeply nested valid structures', () => { | ||
const obj = { | ||
json: JSON.stringify({ | ||
a: [1, { json: JSON.stringify({ b: 'foo' }) }], | ||
}), | ||
}; | ||
expect(areFakerArgsValid([obj])).to.be.true; | ||
}); | ||
|
||
it('returns false for deeply nested invalid structures', () => { | ||
const obj = { | ||
json: JSON.stringify({ | ||
a: [1, { json: JSON.stringify({ b: Infinity }) }], | ||
}), | ||
}; | ||
expect(areFakerArgsValid([obj])).to.be.false; | ||
}); | ||
|
||
describe('isValidFakerMethod', () => { | ||
it('returns false for invalid method format', () => { | ||
expect(isValidFakerMethod('invalidMethod', [])).to.deep.equal({ | ||
isValid: false, | ||
fakerArgs: [], | ||
}); | ||
expect(isValidFakerMethod('internet.email.extra', [])).to.deep.equal({ | ||
isValid: false, | ||
fakerArgs: [], | ||
}); | ||
}); | ||
|
||
it('returns false for non-existent faker module', () => { | ||
expect(isValidFakerMethod('notamodule.email', [])).to.deep.equal({ | ||
isValid: false, | ||
fakerArgs: [], | ||
}); | ||
}); | ||
|
||
it('returns false for non-existent faker method', () => { | ||
expect(isValidFakerMethod('internet.notamethod', [])).to.deep.equal({ | ||
isValid: false, | ||
fakerArgs: [], | ||
}); | ||
}); | ||
|
||
it('returns true for valid method without arguments', () => { | ||
const result = isValidFakerMethod('internet.email', []); | ||
expect(result.isValid).to.be.true; | ||
expect(result.fakerArgs).to.deep.equal([]); | ||
}); | ||
|
||
it('returns true for valid method with valid arguments', () => { | ||
// name.firstName takes optional gender argument | ||
const result = isValidFakerMethod('name.firstName', ['female']); | ||
expect(result.isValid).to.be.true; | ||
expect(result.fakerArgs).to.deep.equal(['female']); | ||
}); | ||
|
||
it('returns true for valid method with no args if args are invalid but fallback works', () => { | ||
// internet.email does not take arguments, so passing one should fallback to [] | ||
const result = isValidFakerMethod('internet.email', []); | ||
expect(result.isValid).to.be.true; | ||
expect(result.fakerArgs).to.deep.equal([]); | ||
}); | ||
|
||
it('returns false for valid method with invalid arguments and fallback fails', () => { | ||
|
||
// date.month expects at most one argument, passing an object will fail both attempts | ||
const result = isValidFakerMethod('date.month', [ | ||
{ foo: 'bar' } as any, | ||
]); | ||
expect(result.isValid).to.be.false; | ||
expect(result.fakerArgs).to.deep.equal([]); | ||
}); | ||
|
||
it('returns false for helpers methods except arrayElement', () => { | ||
expect(isValidFakerMethod('helpers.fake', [])).to.deep.equal({ | ||
isValid: false, | ||
fakerArgs: [], | ||
}); | ||
expect(isValidFakerMethod('helpers.slugify', [])).to.deep.equal({ | ||
isValid: false, | ||
fakerArgs: [], | ||
}); | ||
}); | ||
|
||
it('returns true for helpers.arrayElement with valid arguments', () => { | ||
const arr = ['a', 'b', 'c']; | ||
const result = isValidFakerMethod('helpers.arrayElement', [arr]); | ||
expect(result.isValid).to.be.true; | ||
expect(result.fakerArgs).to.deep.equal([arr]); | ||
}); | ||
|
||
it('returns false for helpers.arrayElement with invalid arguments', () => { | ||
// Exceeding max args length | ||
const arr = Array(11).fill('x'); | ||
const result = isValidFakerMethod('helpers.arrayElement', [arr]); | ||
expect(result.isValid).to.be.false; | ||
expect(result.fakerArgs).to.deep.equal([]); | ||
}); | ||
|
||
it('returns false for method with invalid fakerArgs', () => { | ||
|
||
// Passing Infinity as argument | ||
const result = isValidFakerMethod('name.firstName', [Infinity]); | ||
expect(result.isValid).to.be.false; | ||
expect(result.fakerArgs).to.deep.equal([]); | ||
}); | ||
}); | ||
}); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.
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: use render in name to clarify React Elements are returned, something like
renderFakerArgsInput
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 is no longer applicable since we will show a preview of the faker call.