-
Notifications
You must be signed in to change notification settings - Fork 246
feat(compass-collection): CLOUDP-348131: Schema Editor Dropdowns: Add mongo type to faker method mapping CLOUDP-348131 #7425
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
Changes from 5 commits
dbeb256
0aaa8f1
73f6b49
b478cd2
85bf859
2ba4849
7228c83
d3baf49
e1d8a5f
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 |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import type { MongoDBFieldType } from '@mongodb-js/compass-generative-ai'; | ||
| import { MockDataGeneratorStep } from './types'; | ||
|
|
||
| export const StepButtonLabelMap = { | ||
|
|
@@ -10,3 +11,72 @@ export const StepButtonLabelMap = { | |
|
|
||
| export const DEFAULT_DOCUMENT_COUNT = 1000; | ||
| export const MAX_DOCUMENT_COUNT = 100000; | ||
|
|
||
| /** | ||
| * Map of MongoDB types to available Faker methods. | ||
| * Not all Faker methods are included here. | ||
| * More can be found in the Faker.js API: https://fakerjs.dev/api/ | ||
| */ | ||
| export const MONGO_TYPE_TO_FAKER_METHODS: Record< | ||
|
Collaborator
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. Gave Augment link to https://v9.fakerjs.dev/api/ and had it come up with some more additions. (If you upgrade to v10 can do the same with the new API link):
Collaborator
Author
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. Adding these! |
||
| MongoDBFieldType, | ||
| Array<string> | ||
| > = { | ||
| String: [ | ||
| 'lorem.word', | ||
| 'lorem.words', | ||
| 'lorem.sentence', | ||
| 'lorem.paragraph', | ||
| 'person.firstName', | ||
| 'person.lastName', | ||
| 'person.fullName', | ||
| 'person.jobTitle', | ||
| 'internet.email', | ||
| 'internet.url', | ||
| 'internet.domainName', | ||
| 'internet.userName', | ||
| 'phone.number', | ||
| 'location.city', | ||
| 'location.country', | ||
| 'location.streetAddress', | ||
| 'location.zipCode', | ||
| 'location.state', | ||
| 'company.name', | ||
| 'company.catchPhrase', | ||
| 'color.human', | ||
| 'commerce.productName', | ||
| 'commerce.department', | ||
| 'finance.accountName', | ||
| 'finance.currencyCode', | ||
| 'git.commitSha', | ||
| 'string.uuid', | ||
| 'string.alpha', | ||
| 'string.alphanumeric', | ||
| ], | ||
| Number: [ | ||
| 'number.int', | ||
| 'number.float', | ||
| 'finance.amount', | ||
| 'location.latitude', | ||
| 'location.longitude', | ||
| ], | ||
| Int32: ['number.int', 'finance.amount'], | ||
| Long: ['number.int', 'number.bigInt'], | ||
| Decimal128: ['number.float', 'finance.amount'], | ||
| Boolean: ['datatype.boolean'], | ||
| Date: [ | ||
| 'date.recent', | ||
| 'date.past', | ||
| 'date.future', | ||
| 'date.anytime', | ||
| 'date.birthdate', | ||
| ], | ||
| Timestamp: ['date.recent', 'date.past', 'date.future', 'date.anytime'], | ||
| ObjectId: ['database.mongodbObjectId'], | ||
| Binary: ['string.hexadecimal', 'string.binary'], | ||
| RegExp: ['lorem.word', 'string.alpha'], | ||
| Code: ['lorem.sentence', 'lorem.paragraph', 'git.commitMessage'], | ||
| MinKey: ['number.int'], | ||
| MaxKey: ['number.int'], | ||
| Symbol: ['lorem.word', 'string.symbol'], | ||
| DBRef: ['database.mongodbObjectId'], | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| import { expect } from 'chai'; | ||
| import React from 'react'; | ||
| import { | ||
| screen, | ||
| render, | ||
| cleanup, | ||
| waitFor, | ||
| userEvent, | ||
| } from '@mongodb-js/testing-library-compass'; | ||
| import sinon from 'sinon'; | ||
| import FakerMappingSelector from './faker-mapping-selector'; | ||
| import { UNRECOGNIZED_FAKER_METHOD } from '../../modules/collection-tab'; | ||
|
Collaborator
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. [Drive-by suggestion]: This made me realize, due to parallel implementation there's a slight mismatch with |
||
| import { MONGO_TYPE_TO_FAKER_METHODS } from './constants'; | ||
| import { MongoDBFieldTypeValues } from '@mongodb-js/compass-generative-ai'; | ||
|
|
||
| const mockActiveJsonType = MongoDBFieldTypeValues.String; | ||
| const mockActiveFakerFunction = 'lorem.word'; | ||
| const onJsonTypeSelectStub = sinon.stub(); | ||
| const onFakerFunctionSelectStub = sinon.stub(); | ||
|
|
||
| describe('FakerMappingSelector', () => { | ||
| afterEach(() => { | ||
| cleanup(); | ||
| }); | ||
|
|
||
| it('should display all MongoDB types in the dropdown', async () => { | ||
| // Check that all MongoDB types from the constant are present | ||
| const mongoTypes = Object.keys(MongoDBFieldTypeValues); | ||
|
|
||
| render( | ||
| <FakerMappingSelector | ||
| activeJsonType={mockActiveJsonType} | ||
| activeFakerFunction="lorem.word" | ||
| onJsonTypeSelect={onJsonTypeSelectStub} | ||
| onFakerFunctionSelect={onFakerFunctionSelectStub} | ||
| /> | ||
| ); | ||
|
|
||
| const jsonTypeSelect = screen.getByLabelText('JSON Type'); | ||
| userEvent.click(jsonTypeSelect); | ||
|
|
||
| for (const type of mongoTypes) { | ||
| await waitFor(() => { | ||
| expect(screen.getByRole('option', { name: type })).to.exist; | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| describe('should display faker methods for each MongoDB type', () => { | ||
| for (const [mongoType, methods] of Object.entries( | ||
| MONGO_TYPE_TO_FAKER_METHODS | ||
| )) { | ||
| it(`should display faker methods for ${mongoType}`, () => { | ||
| const firstMethod = methods[0]; | ||
|
|
||
| render( | ||
| <FakerMappingSelector | ||
| activeJsonType={ | ||
| mongoType as keyof typeof MONGO_TYPE_TO_FAKER_METHODS | ||
| } | ||
| activeFakerFunction={firstMethod} | ||
| onJsonTypeSelect={onJsonTypeSelectStub} | ||
| onFakerFunctionSelect={onFakerFunctionSelectStub} | ||
| /> | ||
| ); | ||
|
|
||
| const fakerFunctionSelect = screen.getByLabelText('Faker Function'); | ||
| userEvent.click(fakerFunctionSelect); | ||
|
|
||
| methods.forEach((method) => { | ||
| expect(screen.getByRole('option', { name: method })).to.exist; | ||
| }); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| it('should call onJsonTypeSelect when MongoDB type changes', async () => { | ||
| render( | ||
| <FakerMappingSelector | ||
| activeJsonType={mockActiveJsonType} | ||
| activeFakerFunction={mockActiveFakerFunction} | ||
| onJsonTypeSelect={onJsonTypeSelectStub} | ||
| onFakerFunctionSelect={onFakerFunctionSelectStub} | ||
| /> | ||
| ); | ||
|
|
||
| const jsonTypeSelect = screen.getByLabelText('JSON Type'); | ||
| userEvent.click(jsonTypeSelect); | ||
|
|
||
| const numberOption = await screen.findByRole('option', { name: 'Number' }); | ||
| userEvent.click(numberOption); | ||
|
|
||
| expect(onJsonTypeSelectStub).to.have.been.calledOnceWith('Number'); | ||
| }); | ||
|
|
||
| it('should call onFakerFunctionSelect when faker function changes', async () => { | ||
| render( | ||
| <FakerMappingSelector | ||
| activeJsonType={mockActiveJsonType} | ||
| activeFakerFunction={mockActiveFakerFunction} | ||
| onJsonTypeSelect={onJsonTypeSelectStub} | ||
| onFakerFunctionSelect={onFakerFunctionSelectStub} | ||
| /> | ||
| ); | ||
|
|
||
| const fakerFunctionSelect = screen.getByLabelText('Faker Function'); | ||
| userEvent.click(fakerFunctionSelect); | ||
|
|
||
| const emailOption = await screen.findByRole('option', { | ||
| name: 'internet.email', | ||
| }); | ||
| userEvent.click(emailOption); | ||
|
|
||
| expect(onFakerFunctionSelectStub).to.have.been.calledOnceWith( | ||
| 'internet.email' | ||
| ); | ||
| }); | ||
|
|
||
| it('should show warning banner when faker method is unrecognized', () => { | ||
| render( | ||
| <FakerMappingSelector | ||
| activeJsonType={mockActiveJsonType} | ||
| activeFakerFunction={UNRECOGNIZED_FAKER_METHOD} | ||
| onJsonTypeSelect={onJsonTypeSelectStub} | ||
| onFakerFunctionSelect={onFakerFunctionSelectStub} | ||
| /> | ||
| ); | ||
|
|
||
| expect( | ||
| screen.getByText( | ||
| /Please select a function or we will default fill this field/ | ||
| ) | ||
| ).to.exist; | ||
| }); | ||
|
|
||
| it('should not show warning banner when faker method is recognized', () => { | ||
| render( | ||
| <FakerMappingSelector | ||
| activeJsonType={mockActiveJsonType} | ||
| activeFakerFunction={mockActiveFakerFunction} | ||
| onJsonTypeSelect={onJsonTypeSelectStub} | ||
| onFakerFunctionSelect={onFakerFunctionSelectStub} | ||
| /> | ||
| ); | ||
|
|
||
| expect( | ||
| screen.queryByText( | ||
| /Please select a function or we will default fill this field/ | ||
| ) | ||
| ).to.not.exist; | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,8 +12,13 @@ import { | |||||||||||||||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||||||||||||||||
| import FieldSelector from './schema-field-selector'; | ||||||||||||||||||||||||||||||||
| import FakerMappingSelector from './faker-mapping-selector'; | ||||||||||||||||||||||||||||||||
| import type { FakerSchema, MockDataGeneratorState } from './types'; | ||||||||||||||||||||||||||||||||
| import type { | ||||||||||||||||||||||||||||||||
| FakerSchema, | ||||||||||||||||||||||||||||||||
| FakerFieldMapping, | ||||||||||||||||||||||||||||||||
| MockDataGeneratorState, | ||||||||||||||||||||||||||||||||
| } from './types'; | ||||||||||||||||||||||||||||||||
| import type { MongoDBFieldType } from '@mongodb-js/compass-generative-ai'; | ||||||||||||||||||||||||||||||||
| import { getDefaultFakerMethod } from './script-generation-utils'; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| import { getDefaultFakerMethod } from './script-generation-utils'; |
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 just copying right? Could we simplify to something like
| const originalLlmMappings = React.useRef<Record<string, FakerFieldMapping>>( | |
| Object.fromEntries( | |
| Object.entries(fakerSchema).map(([field, mapping]) => [ | |
| field, | |
| { | |
| mongoType: mapping.mongoType, | |
| fakerMethod: mapping.fakerMethod, | |
| fakerArgs: mapping.fakerArgs, | |
| }, | |
| ]) | |
| ) | |
| ); | |
| const originalLlmMappings = React.useRef<Record<string, FakerFieldMapping>>({ | |
| ...fakerSchema | |
| }); |
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.
Yeah, good catch on this cleanup. The spread will copy it over too
Outdated
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.
np: isSwitchingToOriginalType already checks for originalLlmMapping, so I think we can drop the second check for originalLlmMapping here
Outdated
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.
Per this comment I think same here
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.
This is linking to faker v10, while we're currently on v9— maybe we should just take this opportunity to bump to v10. Think we could just update in
package.json:"@faker-js/faker": "^10.0.0", I think should be straightforward, only change I see on first pass would beinternet.userName->internet.usernamein this fileThere 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.
The compass repo is set up to use common JS and not ESM, which is what faker v10 requires. I'm wondering if we can tackle this in a separate PR if we want to upgrade to v10 so that we can get this ticket in first.
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.
Compass repo can handle esm in application code just fine, but I think you downgraded this to 9 because that's what the model generates better, at least based on this comment #7295 (comment)
Would be nice if you document this somewhere so that the reason is known
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 What do you think about this? Can you document which version we want in the docs?
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.
Ok sounds good to me, we can stick with v9 for now