Skip to content

feat(compass-data-modeling): delete and rename field COMPASS-9659 #7237

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 15 commits into from
Aug 28, 2025
Merged

feat(compass-data-modeling): delete and rename field COMPASS-9659 #7237

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
060e63b
feat(compass-data-modeling): delete and rename field COMPASS-9659
paula-stacho Aug 26, 2025
ce70661
relationship updating
paula-stacho Aug 26, 2025
54e086a
field validation
paula-stacho Aug 26, 2025
7694648
add tests and update selection
paula-stacho Aug 26, 2025
7e6c4ba
more tests
paula-stacho Aug 26, 2025
850b54f
more tests
paula-stacho Aug 26, 2025
cc5a7ff
add more tests
paula-stacho Aug 27, 2025
343f0c5
add file
paula-stacho Aug 27, 2025
a854267
improve readability
paula-stacho Aug 27, 2025
1ec4e54
rename
paula-stacho Aug 27, 2025
c5b4f14
cleanup
paula-stacho Aug 27, 2025
eb716a9
restrict idField
paula-stacho Aug 28, 2025
09ef6f1
implement PR suggestions
paula-stacho Aug 28, 2025
307a26c
move out applyEdit
paula-stacho Aug 28, 2025
48cc245
use memoized fn
paula-stacho Aug 28, 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
119 changes: 97 additions & 22 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 @@ -36,6 +36,15 @@ const waitForDrawerToOpen = async () => {
});
};

const updateInputWithBlur = (label: string, text: string) => {
const input = screen.getByLabelText(label);
userEvent.clear(input);
if (text.length) userEvent.type(input, text);

// Blur/unfocus the input.
userEvent.click(document.body);
};

async function comboboxSelectItem(
label: string,
value: string,
Expand Down Expand Up @@ -136,8 +145,8 @@ describe('DiagramEditorSidePanel', function () {
).to.be.visible;
});

describe('Field context drawer', function () {
it('should render a field context drawer when field is clicked', async function () {
describe('When a field is selected', function () {
it('should render a field context drawer', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(selectField('flights.airlines', ['alias']));

Expand All @@ -154,7 +163,7 @@ describe('DiagramEditorSidePanel', function () {
expect(selectedTypes).to.include('int');
});

it('should render a nested field context drawer when field is clicked', async function () {
it('should render a nested field context drawer', async function () {
const result = renderDrawer();
result.plugin.store.dispatch(
selectField('flights.routes', ['airline', 'id'])
Expand All @@ -171,6 +180,87 @@ describe('DiagramEditorSidePanel', function () {
expect(selectedTypes).to.have.lengthOf(1);
expect(selectedTypes).to.include('string');
});

it('should delete a field', 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;

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

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

const modifiedCollection = selectCurrentModelFromState(
result.plugin.store.getState()
).collections.find((coll) => {
return coll.ns === 'flights.routes';
});

expect(
modifiedCollection?.jsonSchema.properties?.airline.properties
).to.not.have.property('id'); // deleted field
expect(
modifiedCollection?.jsonSchema.properties?.airline.properties
).to.have.property('name'); // sibling field remains
});

it('should rename a field and keep it selected', 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;

updateInputWithBlur('Field name', 'new_name');

await waitFor(() => {
expect(screen.queryByText('routes.airline.name')).not.to.exist;
expect(screen.queryByText('routes.airline.new_name')).to.exist;
});
});

it('should not rename a field to an empty string', 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;

updateInputWithBlur('Field name', '');

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

it('should not rename a field to a duplicate', 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;

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 content of the drawer when selecting different items', async function () {
Expand Down Expand Up @@ -456,11 +546,7 @@ describe('DiagramEditorSidePanel', function () {
expect(screen.getByLabelText('Name')).to.have.value('countries');

// Update the name.
userEvent.clear(screen.getByLabelText('Name'));
userEvent.type(screen.getByLabelText('Name'), 'pineapple');

// Blur/unfocus the input.
userEvent.click(document.body);
updateInputWithBlur('Name', 'pineapple');

// Check the name in the model.
const modifiedCollection = selectCurrentModelFromState(
Expand All @@ -486,11 +572,7 @@ describe('DiagramEditorSidePanel', function () {
expect(activeElement).to.equal(nameInput);

// Update the name.
userEvent.clear(nameInput);
userEvent.type(nameInput, 'pineapple');

// Blur/unfocus the input - now the collection should be names
userEvent.click(document.body);
updateInputWithBlur('Name', 'pineapple');

// Check the name in the model.
const newCollection = selectCurrentModelFromState(
Expand All @@ -516,7 +598,7 @@ describe('DiagramEditorSidePanel', function () {
'false'
);

userEvent.clear(screen.getByLabelText('Name'));
updateInputWithBlur('Name', '');

await waitFor(() => {
expect(screen.getByLabelText('Name')).to.have.attribute(
Expand All @@ -525,9 +607,6 @@ describe('DiagramEditorSidePanel', function () {
);
});

// Blur/unfocus the input.
userEvent.click(document.body);

const notModifiedCollection = selectCurrentModelFromState(
result.plugin.store.getState()
).collections.find((c: DataModelCollection) => {
Expand All @@ -549,8 +628,7 @@ describe('DiagramEditorSidePanel', function () {
'false'
);

userEvent.clear(screen.getByLabelText('Name'));
userEvent.type(screen.getByLabelText('Name'), 'airlines');
updateInputWithBlur('Name', 'airlines');

await waitFor(() => {
expect(screen.getByLabelText('Name')).to.have.attribute(
Expand All @@ -559,9 +637,6 @@ describe('DiagramEditorSidePanel', function () {
);
});

// Blur/unfocus the input.
userEvent.click(document.body);

const notModifiedCollection = selectCurrentModelFromState(
result.plugin.store.getState()
).collections.find((c: DataModelCollection) => {
Expand Down
9 changes: 7 additions & 2 deletions packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import RelationshipDrawerContent from './relationship-drawer-content';
import {
deleteCollection,
deleteRelationship,
removeField,
selectCurrentModelFromState,
type SelectedItems,
} from '../../store/diagram';
Expand Down Expand Up @@ -109,7 +110,11 @@ function DiagramEditorSidePanel({
></FieldDrawerContent>
),
actions: [
{ action: 'delete', label: 'Delete', icon: 'Trash' as const },
{
action: 'delete',
label: 'Delete Field',
icon: 'Trash' as const,
},
],
handleAction: (actionName: string) => {
if (actionName === 'delete') {
Expand Down Expand Up @@ -241,6 +246,6 @@ export default connect(
{
onDeleteCollection: deleteCollection,
onDeleteRelationship: deleteRelationship,
onDeleteField: () => {}, // TODO(COMPASS-9659) part 2 - implement onDeleteField,
onDeleteField: removeField,
}
)(DiagramEditorSidePanel);
59 changes: 32 additions & 27 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
Expand Up @@ -13,6 +13,8 @@ import { BSONType } from 'mongodb';
import {
createNewRelationship,
deleteRelationship,
extractFieldsFromSchema,
renameField,
selectCurrentModelFromState,
selectRelationship,
} from '../../store/diagram';
Expand All @@ -24,7 +26,7 @@ import {
import { useChangeOnBlur } from './use-change-on-blur';
import { RelationshipsSection } from './relationships-section';
import { getFieldFromSchema } from '../../utils/schema-traversal';
import { areFieldPathsEqual } from '../../utils/utils';
import { areFieldPathsEqual, isRelationshipOfAField } from '../../utils/utils';

type FieldDrawerContentProps = {
namespace: string;
Expand All @@ -44,7 +46,7 @@ type FieldDrawerContentProps = {
onRenameField: (
namespace: string,
fromFieldPath: FieldPath,
toFieldPath: FieldPath
newName: string
) => void;
onChangeFieldType: (
namespace: string,
Expand Down Expand Up @@ -72,11 +74,19 @@ export function getIsFieldNameValid(
};
}

const fieldsNamesWithoutCurrent = existingFields
.filter((fieldPath) => !areFieldPathsEqual(fieldPath, currentFieldPath))
const siblingFields = existingFields
.filter(
(fieldPath) =>
fieldPath.length === currentFieldPath.length &&
areFieldPathsEqual(
fieldPath.slice(0, fieldPath.length - 1),
currentFieldPath.slice(0, fieldPath.length - 1)
) &&
!areFieldPathsEqual(fieldPath, currentFieldPath)
Copy link
Collaborator

@gribnoysup gribnoysup Aug 27, 2025

Choose a reason for hiding this comment

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

Nit: it's kinda peanuts here, so feel free to ignore, but you're running this in a loop over an arbitrary number of items, so the more cheap checks we can do to short circuit the expression, the better. Suggesting mostly as an example to keep in mind when writing code like that

Suggested change
fieldPath.length === currentFieldPath.length &&
areFieldPathsEqual(
fieldPath.slice(0, fieldPath.length - 1),
currentFieldPath.slice(0, fieldPath.length - 1)
) &&
!areFieldPathsEqual(fieldPath, currentFieldPath)
fieldPath.length === currentFieldPath.length &&
fieldPath[fieldPath.length - 1] !== currentFieldPath[currentFieldPath.length - 1] &&
areFieldPathsEqual(
fieldPath.slice(0, fieldPath.length - 1),
currentFieldPath.slice(0, currentFieldPath.length - 1)
)

Copy link
Collaborator Author

@paula-stacho paula-stacho Aug 28, 2025

Choose a reason for hiding this comment

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

Thanks! The slice check is indeed better, but I'm not sure if the order needs to change. The 3rd check only eliminates fields with the same name, so unless there is a lot of field name repetition, both checks would be executed most of the time. The other check eliminates non-siblings, so it has a higher chance of reducing the number of checks.. even if it does get more expensive with a long path and high number of close relatives

)
.map((fieldPath) => fieldPath[fieldPath.length - 1]);

const isDuplicate = fieldsNamesWithoutCurrent.some(
const isDuplicate = siblingFields.some(
(fieldName) => fieldName === trimmedName
);

Expand Down Expand Up @@ -108,10 +118,7 @@ const FieldDrawerContent: React.FunctionComponent<FieldDrawerContentProps> = ({
if (!isFieldNameValid) {
return;
}
onRenameField(namespace, fieldPath, [
...fieldPath.slice(0, fieldPath.length - 1),
trimmedName,
]);
onRenameField(namespace, fieldPath, trimmedName);
}
);

Expand All @@ -131,7 +138,6 @@ const FieldDrawerContent: React.FunctionComponent<FieldDrawerContentProps> = ({
<DMFormFieldContainer>
<TextInput
label="Field name"
disabled={true} // TODO: enable when field renaming is implemented
data-testid="data-model-collection-drawer-name-input"
sizeVariant="small"
value={fieldName}
Expand Down Expand Up @@ -182,34 +188,33 @@ export default connect(
ownProps: { namespace: string; fieldPath: FieldPath }
) => {
const model = selectCurrentModelFromState(state);
const collectionSchema = model.collections.find(
(collection) => collection.ns === ownProps.namespace
)?.jsonSchema;
if (!collectionSchema) {
throw new Error('Collection not found');
}
return {
types:
getFieldFromSchema({
jsonSchema:
model.collections.find(
(collection) => collection.ns === ownProps.namespace
)?.jsonSchema ?? {},
jsonSchema: collectionSchema,
fieldPath: ownProps.fieldPath,
})?.fieldTypes ?? [],
fieldPaths: [], // TODO(COMPASS-9659): get existing field paths
relationships: model.relationships.filter((r) => {
const [local, foreign] = r.relationship;
return (
(local.ns === ownProps.namespace &&
local.fields &&
areFieldPathsEqual(local.fields, ownProps.fieldPath)) ||
(foreign.ns === ownProps.namespace &&
foreign.fields &&
areFieldPathsEqual(foreign.fields, ownProps.fieldPath))
);
}),
fieldPaths: extractFieldsFromSchema(collectionSchema),
Copy link
Collaborator

@gribnoysup gribnoysup Aug 27, 2025

Choose a reason for hiding this comment

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

I'd suggest to make the interface of your component tighther here and only accept the actual data that you need inside the component and pick field paths for the parent of the field you're rendering this for, roughly something like:

Suggested change
fieldPaths: extractFieldsFromSchema(collectionSchema),
fieldPaths: Object.keys(getFieldFromSchema({jsonSchema: collectionSchema, fieldPath: ownProps.fieldPath.slice(0, ownProps.fieldPath - 1)}).properties),

You're only using these to validate whether or not the field is duplicated with another field, so precalculating this array of fields here makes the interface of the component clearer.

Just generally, from the perspective of how to think about component interfaces, I would strongly suggest to limit it to the absolute minimum of what the component needs. Start from component itself, define the bare minimum of what you need without connecting it to state first, then proceed to map state to required properties. This usually leads to way cleaner component inteface, usually more composable, and as a side-effect usually means that your connect functions actually have a chance of avoiding unnecessary re-renders (that's almost impossible when non primitive values are passed, so we're not going to get it here, but you'd generally get more simple values if that's how you approach this)

relationships: model.relationships.filter(({ relationship }) =>
isRelationshipOfAField(
relationship,
ownProps.namespace,
ownProps.fieldPath
)
),
};
},
{
onCreateNewRelationshipClick: createNewRelationship,
onEditRelationshipClick: selectRelationship,
onDeleteRelationshipClick: deleteRelationship,
onRenameField: () => {}, // TODO(COMPASS-9659): renameField,
onRenameField: renameField,
onChangeFieldType: () => {}, // TODO(COMPASS-9659): updateFieldSchema,
}
)(FieldDrawerContent);
4 changes: 2 additions & 2 deletions packages/compass-data-modeling/src/services/data-model-storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ const EditSchemaVariants = z.discriminatedUnion('type', [
z.object({
type: z.literal('RenameField'),
ns: z.string(),
from: FieldPathSchema,
to: FieldPathSchema,
field: FieldPathSchema,
newName: z.string(),
}),
z.object({
type: z.literal('RemoveField'),
Expand Down
Loading
Loading