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

Merged
merged 38 commits into from
Mar 11, 2025

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 share 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

@@ -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 from 0a59f34 to 6a33e9b Compare March 7, 2025 16:53
@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
@prastoin prastoin force-pushed the prastoin-bugfix-names-object-model-empty-string branch from 8231668 to f789324 Compare March 9, 2025 20:15
Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

Nice PR!
FYI I had the idea of using class-validator (which is more standard with nestjs backend) with custom validation decorators over our input classes but I think with your implementation it's a bit less important, will see how it scales 👍

@prastoin
Copy link
Contributor Author

@greptileai trigger

@prastoin prastoin force-pushed the prastoin-bugfix-names-object-model-empty-string branch from 9560ce2 to 99ca95f Compare March 11, 2025 09:54
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Based on the provided information, I'll summarize the key changes in this PR focusing on the most recent updates without repeating previous points:

Enhanced server-side validation for ObjectMetadata with new utility functions:

  • Added validateMetadataNameIsCamelCaseOrThrow to enforce camelCase naming convention
  • Implemented validateMetadataNameIsNotTooShort/LongOrThrow to enforce length constraints (1-63 chars)
  • Created validateMetadataNameContainsWhitelistedCharacters to restrict to alphanumeric characters
  • Introduced validateLowerCasedAndTrimmedStringsAreDifferentOrThrow to ensure unique singular/plural names

The changes consolidate error handling by:

  • Replacing multiple specific exceptions with unified InvalidMetadataNameException
  • Improving error messages with detailed validation failure reasons
  • Adding case-insensitive comparison for name uniqueness checks

These updates provide a more robust validation system while maintaining clean code organization and improved error handling.

24 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

Great job @prastoin!! LGTM

@Weiko Weiko merged commit 41f3a63 into main Mar 11, 2025
49 checks passed
@Weiko Weiko deleted the prastoin-bugfix-names-object-model-empty-string branch March 11, 2025 11:14
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
3 participants