Skip to content
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

[BUGFIX] ObjectMetadata item server validation #10699

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Mar 6, 2025

Introduction

This PR contains several SNAPSHOT files explaining big +

While refactoring the Object Model settings page in #10653, encountered a critical issue when submitting either one or both names with "" empty string hard corrupting a workspace.

This motivate this PR reviewing server side validation

I feel like we could shared zod schema between front and back

Refactored server validation

What to expect from Names:

  • Plural and singular have to be different ( case insensitive and trimmed check )
  • Contains only a-z A-Z and 0-9
  • Follows camelCase
  • Is not empty => Is not too short ( 1 )
  • Is not too long ( 63 )
  • Is case insensitive( fooBar and fOoBar now rejected )

What to expect from Labels:

  • Plural and singular have to be different ( case insensitive and trimmed check )
  • Is not empty => Is not too short ( 1 )
  • Is not too long ( 63 )
  • Is case insensitive ( fooBar and fOoBar now rejected )

close #10694

Creation integrations tests

Created new integrations tests, following EachTesting pattern and uses snapshot to assert errors message. These tests cover several failing use cases and started to implement ones for the happy path but object metadata item deletion is currently broken unless I'm mistaken @Weiko is on it

Notes

  • As we've added new validation rules towards names and labels we should scan db in order to standardize existing values using either a migration command or manual check
  • Will review in an other PR the update path, adding integrations tests and so on

@@ -67,8 +67,8 @@ export class RelationMetadataService extends TypeOrmQueryService<RelationMetadat
);

try {
validateMetadataNameValidityOrThrow(relationMetadataInput.fromName);
validateMetadataNameValidityOrThrow(relationMetadataInput.toName);
validateObjectMetadataInputNameOrThrow(relationMetadataInput.fromName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review: Unless I'm mistaken we need the same strictness level here than along object model creation

@@ -199,7 +215,7 @@ export class ObjectMetadataService extends TypeOrmQueryService<ObjectMetadataEnt
input: UpdateOneObjectInput,
workspaceId: string,
): Promise<ObjectMetadataEntity> {
validateObjectMetadataInputOrThrow(input.update);
validateObjectMetadataInputNamesOrThrow(input.update);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Will pass on update in an other PR

@prastoin prastoin force-pushed the prastoin-bugfix-names-object-model-empty-string branch 2 times, most recently from 8a2f744 to cdccc57 Compare March 9, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create ObjectMetadataItem with empty string names breaks workspace
1 participant