Skip to content

check on label metadata #12852

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

Merged
merged 9 commits into from
Jun 24, 2025
Merged

check on label metadata #12852

merged 9 commits into from
Jun 24, 2025

Conversation

guillim
Copy link
Contributor

@guillim guillim commented Jun 24, 2025

Better catching label input

  • there were absolutely no check on label when creating the target field while doing a relation : we crearted these checks here.

    • We keep the label quite open to special char as discussed with Felix. so mostly checking length of label.
    • We check that label does not already exists on the targetted object
  • making sure the Target fieldinput label is checked before we create it. The previous checks are not enough since the label goes through anoteher merthod before going in the database

  • validate-metadata-name-is-camel-case.utils.ts : making sure we can use this error message for metadata name and for target label

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

Enhanced label validation across metadata modules with improved error handling and messaging.

  • Modified error message format in validateMetadataNameIsCamelCase to improve clarity by placing the invalid name at the start
  • Added new validateMetadataTargetLabelOrThrow utility that combines multiple validation checks for label validation
  • Implemented additional validation in field-metadata.service.ts to prevent duplicate field names and ensure valid target labels in relations
  • Added NOT_FIRST_LETTER_UPPER_CASE validation rule to enforce consistent label capitalization standards

4 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile

@@ -0,0 +1,13 @@
import { validateMetadataNameIsNotReservedKeywordOrThrow } from 'src/engine/metadata-modules/utils/validate-metadata-name-is-not-reserved-keyword';
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Import path missing '.utils' extension - should match the pattern of other imports

validateMetadataNameIsNotReservedKeywordOrThrow,
];

validators.forEach((validator) => validator(label));
Copy link
Contributor

Choose a reason for hiding this comment

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

style: label.trim() should be used before validation to maintain consistency with existing label validation functions

Copy link
Contributor

github-actions bot commented Jun 24, 2025

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:55093

This environment will automatically shut down when the PR is closed or after 5 hours.

@@ -773,13 +798,28 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
);
}

const targetFieldMetadataName = computeMetadataNameFromLabel(
Copy link
Member

Choose a reason for hiding this comment

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

remove!

@@ -12,6 +12,7 @@ export enum InvalidMetadataExceptionCode {
EXCEEDS_MAX_LENGTH = 'Exceeds max length',
RESERVED_KEYWORD = 'Reserved keyword',
NOT_CAMEL_CASE = 'Not camel case',
NOT_FIRST_LETTER_UPPER_CASE = 'Not first letter upper case',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: IMO would be better as FIRST_LETTER_LOWER_CASE or NOT_CAPITALIZED

@@ -8,7 +8,7 @@ import {
export const validateMetadataNameIsCamelCaseOrThrow = (name: string) => {
if (name !== camelCase(name)) {
throw new InvalidMetadataException(
`Name should be in camelCase: ${name}`,
`${name} should be in camelCase`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remark: That's why you have so many snapshots errors

@prastoin prastoin enabled auto-merge (squash) June 24, 2025 17:42
@prastoin
Copy link
Contributor

prastoin commented Jun 24, 2025

@guillim I would highly recommend to implement integration testing on each relation label exceptions
We can do some peer on the subject, would recommend following the update object metadata testing tables

@prastoin prastoin disabled auto-merge June 24, 2025 17:45
@prastoin
Copy link
Contributor

Blocking test

  [Nest] 6252  - 06/24/2025, 5:46:26 PM   ERROR [WorkspaceMigrationRunnerService] Error executing migration: Enum type name not found for column createdBySource in table _house while trying to alter enum

@prastoin prastoin self-requested a review June 24, 2025 17:57
Copy link
Contributor

@prastoin prastoin left a comment

Choose a reason for hiding this comment

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

Looks like this has broken more things than expected
Will release 1.0.0 without it from now

[Nest] 6252  - 06/24/2025, 5:46:26 PM   ERROR [WorkspaceMigrationRunnerService] Error executing migration: Enum type name not found for column createdBySource in 
table _house while trying to alter enum

@prastoin
Copy link
Contributor

Unable to reproduce in local tho 🤔
Might be flakiness, rerunning failed jobs

@prastoin
Copy link
Contributor

Indeed there's flakiness

@prastoin prastoin self-requested a review June 24, 2025 18:20
@prastoin prastoin merged commit fb0cf11 into main Jun 24, 2025
48 of 50 checks passed
@prastoin prastoin deleted the label-check-metadata branch June 24, 2025 18:20
prastoin added a commit that referenced this pull request Jun 25, 2025
# Introduction
Following #12852

Discovered that:
- `relationCreationPayload` does not seem to be validated through the
input decorators
```ts
  // TODO @prastoin implement validation for this with validate nested and dedicated class instance
  @IsOptional()
  @field(() => GraphQLJSON, { nullable: true })
  relationCreationPayload?: {
    targetObjectMetadataId: string;
    targetFieldLabel: string;
    targetFieldIcon: string;
    type: RelationType;
  };
```
- Sending an unknown `targetObjectMetadataId` generates an
`internal_server_error` `500` @guillim on the go
## Coverage
```ts
 PASS  test/integration/metadata/suites/object-metadata/failing-field-metadata-relation-creation.integration-spec.ts
  Field metadata relation creation should fail
    ✓ relation when targetFieldLabel is empty (109 ms)
    ✓ relation when targetFieldLabel exceeds maximum length (100 ms)
    ✓ relation when targetObjectMetadataId is unknown (97 ms)
    ✓ relation when targetFieldLabel contains only whitespace (103 ms)
    ✓ relation when targetFieldLabel conflicts with an existing field on target object metadata id (108 ms)

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   5 passed, 5 total
Time:        2.629 s, estimated 3 s
```
abdulrahmancodes pushed a commit to abdulrahmancodes/twenty that referenced this pull request Jun 26, 2025
Better catching label input

- there were absolutely no check on label when creating the target field
while doing a relation : we crearted these checks here.

- We keep the label quite open to special char as discussed with Felix.
so mostly checking length of label.
  - We check that label does not already exists on the targetted object


- making sure the Target fieldinput label is checked before we create
it. The previous checks are not enough since the label goes through
anoteher merthod before going in the database

- validate-metadata-name-is-camel-case.utils.ts : making sure we can use
this error message for metadata name and for target label

---------

Co-authored-by: Charles Bochet <[email protected]>
Co-authored-by: prastoin <[email protected]>
abdulrahmancodes pushed a commit to abdulrahmancodes/twenty that referenced this pull request Jun 26, 2025
# Introduction
Following twentyhq#12852

Discovered that:
- `relationCreationPayload` does not seem to be validated through the
input decorators
```ts
  // TODO @prastoin implement validation for this with validate nested and dedicated class instance
  @IsOptional()
  @field(() => GraphQLJSON, { nullable: true })
  relationCreationPayload?: {
    targetObjectMetadataId: string;
    targetFieldLabel: string;
    targetFieldIcon: string;
    type: RelationType;
  };
```
- Sending an unknown `targetObjectMetadataId` generates an
`internal_server_error` `500` @guillim on the go
## Coverage
```ts
 PASS  test/integration/metadata/suites/object-metadata/failing-field-metadata-relation-creation.integration-spec.ts
  Field metadata relation creation should fail
    ✓ relation when targetFieldLabel is empty (109 ms)
    ✓ relation when targetFieldLabel exceeds maximum length (100 ms)
    ✓ relation when targetObjectMetadataId is unknown (97 ms)
    ✓ relation when targetFieldLabel contains only whitespace (103 ms)
    ✓ relation when targetFieldLabel conflicts with an existing field on target object metadata id (108 ms)

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   5 passed, 5 total
Time:        2.629 s, estimated 3 s
```
@guillim guillim moved this from 🔦 In QA to ✅ Done in 🎯 Roadmap & Sprints Jun 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants