Skip to content

feat(compass-data-modeling): update field type COMPASS-9659 #7248

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Sep 2, 2025
Merged

feat(compass-data-modeling): update field type COMPASS-9659 #7248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4ef8b3a
feat(compass-data-modeling): update field type COMPASS-9659
paula-stacho Aug 28, 2025
5708a0b
add getSchemaForNewTypes tests
paula-stacho Aug 28, 2025
c8af473
fixes
paula-stacho Aug 28, 2025
49d2e94
rename fn
paula-stacho Aug 28, 2025
2a050c4
fixes and empty type prevention
paula-stacho Aug 29, 2025
fffa78a
_id tests
paula-stacho Aug 29, 2025
7f6b5d2
lint
paula-stacho Aug 29, 2025
8d8f7a8
cleanup effect
paula-stacho Sep 1, 2025
02ccc54
cleanup
paula-stacho Sep 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 148 additions & 8 deletions packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ async function comboboxSelectItem(
});
}

async function multiComboboxToggleItem(
label: string,
value: string,
visibleLabel = value
) {
userEvent.click(screen.getByRole('textbox', { name: label }));
await waitFor(() => {
const listbox = screen.getByRole('listbox');
expect(listbox).to.be.visible;
const option = within(listbox).getByRole('option', { name: visibleLabel });
userEvent.click(option);
});
}

function getMultiComboboxValues(testId: string) {
const combobox = screen.getByTestId(testId);
expect(combobox).to.be.visible;
Expand Down Expand Up @@ -166,15 +180,15 @@ describe('DiagramEditorSidePanel', function () {
it('should render a nested field context drawer', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(
selectField('flights.routes', ['airline', 'id'])
selectField('flights.routes', ['airline', '_id'])
);

await waitForDrawerToOpen();
expect(screen.getByTitle('routes.airline.id')).to.be.visible;
expect(screen.getByTitle('routes.airline._id')).to.be.visible;

const nameInput = screen.getByLabelText('Field name');
expect(nameInput).to.be.visible;
expect(nameInput).to.have.value('id');
expect(nameInput).to.have.value('_id');

const selectedTypes = getMultiComboboxValues('lg-combobox-datatype');
expect(selectedTypes).to.have.lengthOf(1);
Expand All @@ -184,16 +198,16 @@ describe('DiagramEditorSidePanel', function () {
it('should delete a field', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(
selectField('flights.routes', ['airline', 'id'])
selectField('flights.routes', ['airline', '_id'])
);

await waitForDrawerToOpen();
expect(screen.getByTitle('routes.airline.id')).to.be.visible;
expect(screen.getByTitle('routes.airline._id')).to.be.visible;

userEvent.click(screen.getByLabelText(/delete field/i));

await waitFor(() => {
expect(screen.queryByText('routes.airline.id')).not.to.exist;
expect(screen.queryByText('routes.airline._id')).not.to.exist;
});
expect(screen.queryByLabelText('Name')).to.not.exist;

Expand All @@ -205,7 +219,7 @@ describe('DiagramEditorSidePanel', function () {

expect(
modifiedCollection?.jsonSchema.properties?.airline.properties
).to.not.have.property('id'); // deleted field
).to.not.have.property('_id'); // deleted field
expect(
modifiedCollection?.jsonSchema.properties?.airline.properties
).to.have.property('name'); // sibling field remains
Expand Down Expand Up @@ -254,13 +268,139 @@ describe('DiagramEditorSidePanel', function () {
await waitForDrawerToOpen();
expect(screen.getByTitle('routes.airline.name')).to.be.visible;

updateInputWithBlur('Field name', 'id');
updateInputWithBlur('Field name', '_id');

await waitFor(() => {
expect(screen.queryByText('Field already exists.')).to.exist;
expect(screen.queryByText('routes.airline.name')).to.exist;
});
});

it('should change the field type', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(
selectField('flights.routes', ['airline', 'name'])
);

await waitForDrawerToOpen();
expect(screen.getByTitle('routes.airline.name')).to.be.visible;

// before - string
const selectedTypesBefore = getMultiComboboxValues(
'lg-combobox-datatype'
);
expect(selectedTypesBefore).to.have.members(['string']);

// add int and bool and remove string
await multiComboboxToggleItem('Datatype', 'int');
await multiComboboxToggleItem('Datatype', 'bool');
await multiComboboxToggleItem('Datatype', 'string');

const modifiedCollection = selectCurrentModelFromState(
result.plugin.store.getState()
).collections.find((coll) => {
return coll.ns === 'flights.routes';
});
expect(
modifiedCollection?.jsonSchema.properties?.airline?.properties?.name
.bsonType
).to.have.members(['int', 'bool']);
});

it('should not completely remove the type', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(
selectField('flights.routes', ['airline', 'name'])
);

await waitForDrawerToOpen();
expect(screen.getByTitle('routes.airline.name')).to.be.visible;

// before - string
const selectedTypesBefore = getMultiComboboxValues(
'lg-combobox-datatype'
);
expect(selectedTypesBefore).to.have.members(['string']);

// remove string without adding anything else
await multiComboboxToggleItem('Datatype', 'string');

await waitFor(() => {
// error message shown
expect(screen.queryByText('Field must have a type.')).to.exist;
const modifiedCollection = selectCurrentModelFromState(
result.plugin.store.getState()
).collections.find((coll) => {
return coll.ns === 'flights.routes';
});
// type remains unchanged
expect(
modifiedCollection?.jsonSchema.properties?.airline?.properties?.name
.bsonType
).to.equal('string');
});

// finally, add some types
await multiComboboxToggleItem('Datatype', 'bool');
await multiComboboxToggleItem('Datatype', 'int');

await waitFor(() => {
// error goes away
expect(screen.queryByText('Field must have a type.')).not.to.exist;
const modifiedCollection = selectCurrentModelFromState(
result.plugin.store.getState()
).collections.find((coll) => {
return coll.ns === 'flights.routes';
});
// new type applied
expect(
modifiedCollection?.jsonSchema.properties?.airline?.properties?.name
.bsonType
).to.have.members(['bool', 'int']);
});
});

it('top level _id field is treated as readonly', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(selectField('flights.routes', ['_id']));

await waitForDrawerToOpen();
expect(screen.getByTitle('routes._id')).to.be.visible;

expect(screen.queryByLabelText(/delete field/i)).not.to.exist;
expect(screen.getByLabelText('Field name')).to.have.attribute(
'aria-disabled',
'true'
);
expect(screen.getByLabelText('Datatype')).to.have.attribute(
'aria-disabled',
'true'
);
});

it('nested _id field is not treated as readonly', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(
selectField('flights.routes', ['airline', '_id'])
);

await waitForDrawerToOpen();
expect(screen.getByTitle('routes.airline._id')).to.be.visible;

expect(screen.queryByLabelText(/delete field/i)).to.exist;
expect(screen.queryByLabelText(/delete field/i)).to.have.attribute(
'aria-disabled',
'false'
);
expect(screen.getByLabelText('Field name')).to.have.attribute(
'aria-disabled',
'false'
);
expect(screen.getByLabelText('Datatype')).to.have.attribute(
'aria-disabled',
'false'
);
});
});

it('should change the content of the drawer when selecting different items', async function () {
Expand Down
33 changes: 24 additions & 9 deletions packages/compass-data-modeling/src/components/drawer/field-drawer-content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useMemo } from 'react';
import React, { useMemo, useState } from 'react';
import { connect } from 'react-redux';
import type {
FieldPath,
Expand All @@ -11,6 +11,7 @@ import {
} from '@mongodb-js/compass-components';
import { BSONType } from 'mongodb';
import {
changeFieldType,
createNewRelationship,
deleteRelationship,
getCurrentDiagramFromState,
Expand Down Expand Up @@ -56,8 +57,7 @@ type FieldDrawerContentProps = {
onChangeFieldType: (
namespace: string,
fieldPath: FieldPath,
fromBsonType: string | string[],
toBsonType: string | string[]
newTypes: string[]
) => void;
};

Expand Down Expand Up @@ -117,6 +117,11 @@ const FieldDrawerContent: React.FunctionComponent<FieldDrawerContentProps> = ({
onRenameField,
onChangeFieldType,
}) => {
const [fieldTypeEditErrorMessage, setFieldTypeEditErrorMessage] = useState<
string | undefined
>();
const [fieldTypes, setFieldTypes] = useState<string[]>(types);

const { value: fieldName, ...nameInputProps } = useChangeOnBlur(
fieldPath[fieldPath.length - 1],
(fieldName) => {
Expand All @@ -137,17 +142,25 @@ const FieldDrawerContent: React.FunctionComponent<FieldDrawerContentProps> = ({
[fieldPath, fieldPaths, fieldName]
);

const handleTypeChange = (newTypes: string | string[]) => {
onChangeFieldType(namespace, fieldPath, types, newTypes);
const handleTypeChange = (newTypes: string[]) => {
setFieldTypes(newTypes);
if (newTypes.length === 0) {
setFieldTypeEditErrorMessage('Field must have a type.');
return;
}
setFieldTypeEditErrorMessage(undefined);
onChangeFieldType(namespace, fieldPath, newTypes);
};

const isReadOnly = useMemo(() => isIdField(fieldPath), [fieldPath]);

return (
<>
<DMDrawerSection label="Field properties">
<DMFormFieldContainer>
<TextInput
label="Field name"
disabled={isIdField(fieldPath)}
disabled={isReadOnly}
data-testid="data-model-collection-drawer-name-input"
sizeVariant="small"
value={fieldName}
Expand All @@ -162,12 +175,14 @@ const FieldDrawerContent: React.FunctionComponent<FieldDrawerContentProps> = ({
data-testid="lg-combobox-datatype"
label="Datatype"
aria-label="Datatype"
disabled={true} // TODO(COMPASS-9659): enable when field type change is implemented
value={types}
disabled={isReadOnly}
value={fieldTypes}
size="small"
multiselect={true}
clearable={false}
onChange={handleTypeChange}
state={fieldTypeEditErrorMessage ? 'error' : undefined}
errorMessage={fieldTypeEditErrorMessage}
>
{BSON_TYPES.map((type) => (
<ComboboxOption key={type} value={type} />
Expand Down Expand Up @@ -228,6 +243,6 @@ export default connect(
onEditRelationshipClick: selectRelationship,
onDeleteRelationshipClick: deleteRelationship,
onRenameField: renameField,
onChangeFieldType: () => {}, // TODO(COMPASS-9659): updateFieldSchema,
onChangeFieldType: changeFieldType,
}
)(FieldDrawerContent);
27 changes: 24 additions & 3 deletions packages/compass-data-modeling/src/store/apply-edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel {
jsonSchema: updateSchema({
jsonSchema: collection.jsonSchema,
fieldPath: edit.field,
update: 'removeField',
updateParameters: { update: 'removeField' },
}),
};
}),
Expand Down Expand Up @@ -198,8 +198,29 @@ export function applyEdit(edit: Edit, model?: StaticModel): StaticModel {
jsonSchema: updateSchema({
jsonSchema: collection.jsonSchema,
fieldPath: edit.field,
update: 'renameField',
newFieldName: edit.newName,
updateParameters: {
update: 'renameField',
newFieldName: edit.newName,
},
}),
};
}),
};
}
case 'ChangeFieldType': {
return {
...model,
collections: model.collections.map((collection) => {
if (collection.ns !== edit.ns) return collection;
return {
...collection,
jsonSchema: updateSchema({
jsonSchema: collection.jsonSchema,
fieldPath: edit.field,
updateParameters: {
update: 'changeFieldSchema',
newFieldSchema: edit.to,
},
}),
};
}),
Expand Down
34 changes: 33 additions & 1 deletion packages/compass-data-modeling/src/store/diagram.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ import type { MongoDBJSONSchema } from 'mongodb-schema';
import { getCoordinatesForNewNode } from '@mongodb-js/diagramming';
import { collectionToDiagramNode } from '../utils/nodes-and-edges';
import toNS from 'mongodb-ns';
import { traverseSchema } from '../utils/schema-traversal';
import {
getFieldFromSchema,
getSchemaWithNewTypes,
traverseSchema,
} from '../utils/schema-traversal';
import { applyEdit as _applyEdit } from './apply-edit';

function isNonEmptyArray<T>(arr: T[]): arr is [T, ...T[]] {
Expand Down Expand Up @@ -684,6 +688,34 @@ export function renameField(
return applyEdit({ type: 'RenameField', ns, field, newName });
}

export function changeFieldType(
ns: string,
fieldPath: FieldPath,
newTypes: string[]
): DataModelingThunkAction<void, ApplyEditAction | ApplyEditFailedAction> {
return (dispatch, getState) => {
const collectionSchema = selectCurrentModelFromState(
getState()
).collections.find((collection) => collection.ns === ns)?.jsonSchema;
if (!collectionSchema) throw new Error('Collection not found in model');
const field = getFieldFromSchema({
jsonSchema: collectionSchema,
fieldPath: fieldPath,
});
if (!field) throw new Error('Field not found in schema');
Comment on lines +700 to +705
Copy link
Collaborator

Choose a reason for hiding this comment

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

For other edits like relationships we do validations like this in the applyEdit method, why are we breaking this pattern here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually for the name we do validation in the component and display the error next to the field. But this is a good question, we have these editErrors that are not displayed anywhere anymore. They must've fallen through the cracks when we were moving away from the placeholder editor. But I don't see the store validating relationships specifically, just checking the edit against the zod schema? Which is superfluous now, this was needed for the original editor where you'd submit a json edit so we couldn't depend on types. Or am I looking at something else than you found?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm not talking about form field validation here, keeping it near form UI makes sense. Specifically for apply edit we're checking whether or not the model or relationship is missing in the applyEdit method that generates the schema itself, not in the corresponding apply methods, so let's just figure out where we want it, otherwise we are ending up in a weird situation where similar types of errors in edits will have different effects on the application because validation is happening in different places 🙂 Right now the pattern seems to be to validate these in applyEdit and not when the action for adding the edit is dispached

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, sorry, I didn't notice which line you were commenting on :D
The problem here is, we're storing the Edit with the new field jsonSchema - so these failures happen already when we're prepping the Edit to be stored.
To change that, and to move the failure to applyEdit (which gets repeated when you move around in the history or open the diagram), we'd have to change the approach and not store the jsonSchema in the Edit. Instead we'd just store the new types and recalc the field schema each time the Edit is applied.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, wouldn't we want to do that in the first place? This apply method is the source of truth for calculating the final schema, I don't think we want to spread this logic around too much

Copy link
Collaborator

Choose a reason for hiding this comment

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

(we can leave this for a follow-up, but should figure out and agree on a consistent way of handling this)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern also is that if we're not keeping the edits as source of truth for final state and modify them like that it would make generating migrations harder as we're losing information about the actual edit replacing it with the final state of the edit instead

const to = getSchemaWithNewTypes(field.jsonSchema, newTypes);
dispatch(
applyEdit({
type: 'ChangeFieldType',
ns,
field: fieldPath,
from: field.jsonSchema,
to,
})
);
};
}

function getPositionForNewCollection(
existingCollections: DataModelCollection[],
newCollection: Omit<DataModelCollection, 'displayPosition'>
Expand Down
Loading
Loading