Common api - Add field create input validation integration testing#15026
Common api - Add field create input validation integration testing#15026etiennejouan merged 14 commits intomainfrom
Conversation
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces comprehensive field create input validation integration testing by restructuring the test directory from args-validation to inputs-validation and adding extensive validation coverage for all field metadata types. The changes include:
-
Directory restructuring: Moved all existing filter validation tests from
args-validation/toinputs-validation/filter-validation/with updated import paths and terminology (changing "args validation" to "input validation" throughout) -
New create input validation suite: Added a complete
inputs-validation/create-validation/directory with dedicated integration test files for each field metadata type (TEXT, NUMBER, BOOLEAN, UUID, ARRAY, etc.), each testing both GraphQL and REST API endpoints for successful and failing validation scenarios -
Shared testing infrastructure: Added utility functions for testing GraphQL and REST scenarios, constants for test data, and helper functions for setup/teardown of test objects with comprehensive field coverage
-
Type system updates: Introduced new type definitions to categorize which field metadata types should be tested for create vs filter input validation, replacing the previous more generic type system
The new testing framework ensures consistent validation behavior across all field types and API interfaces, providing comprehensive coverage for input validation scenarios in the Twenty CRM system.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/constants/failing-create-input-by-field-metadata-type.constant.ts | 2/5 | New test data file with field validation bugs - incorrect field names used in NUMBER and BOOLEAN test cases |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/constants/successful-create-input-by-field-metadata-type.constant.ts | 2/5 | New test data file with validation bugs in MULTI_SELECT, PHONES, and LINKS field validation functions |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/utils/test-rest-successful-scenario.util.ts | 3/5 | New utility function with type safety issues and missing null checks |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/utils/test-gql-failing-scenario.util.ts | 3/5 | New utility function with naming inconsistencies and missing error bounds checking |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/utils/test-rest-failing-scenario.util.ts | 3/5 | New utility function with type safety issues and brittle error message validation |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/utils/get-field-metadata-creation-inputs.util.ts | 4/5 | Moved and updated utility with new field type support but complex union type handling |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/utils/setup-test-objects-with-all-field-types.util.ts | 4/5 | Core setup utility with good restructuring but simplified GraphQL field selection |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/actor-field-create-input.integration-spec.ts | 4/5 | New test file with good structure but missing failing test scenarios |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/full-name-field-create-input-validation.integration-spec.ts | 4/5 | New test file with good structure but missing failing test scenarios |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/address-field-create-input-validation.integration-spec.ts | 4/5 | New test file with good structure but missing failing test scenarios |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/currency-field-create-input-validation.integration-spec.ts | 4/5 | New test file with good structure but missing failing test scenarios |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/create-validation/rich-text-v2-field-create-input-validation.integration-spec.ts | 4/5 | New test file with good structure but only covers successful scenarios |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/filter-validation/constants/failing-filter-input-by-field-metadata-type.constant.ts | 4/5 | Moved constants file with comprehensive test data but many commented TODO sections |
| All other create validation test files | 5/5 | Comprehensive integration test files following consistent patterns with proper setup/teardown |
| All moved filter validation test files | 5/5 | Successfully moved and updated files with consistent structural changes |
| packages/twenty-server/test/integration/graphql/suites/inputs-validation/types/field-metadata-type-to-test.ts | 5/5 | New type definitions that properly categorize testable field types |
Confidence score: 2/5
- This PR has significant issues that prevent safe merging due to critical bugs in test data constants
- Score reflects serious validation bugs in multiple test constant files that would cause tests to fail or produce incorrect results
- Pay close attention to the constant files with field name mismatches and validation function bugs that must be fixed before deployment
Sequence Diagram
sequenceDiagram
participant User
participant TestSuite
participant SetupUtil
participant ObjectMetadataAPI
participant FieldMetadataAPI
participant GraphQLAPI
participant RestAPI
participant ValidationEngine
participant Database
User->>TestSuite: "Run field validation tests"
TestSuite->>SetupUtil: "setupTestObjectsWithAllFieldTypes()"
SetupUtil->>ObjectMetadataAPI: "createOneObjectMetadata() - main object"
ObjectMetadataAPI->>Database: "Create test object metadata"
Database-->>ObjectMetadataAPI: "Object metadata created"
ObjectMetadataAPI-->>SetupUtil: "objectMetadataId"
SetupUtil->>ObjectMetadataAPI: "createOneObjectMetadata() - target object"
ObjectMetadataAPI->>Database: "Create target object metadata"
Database-->>ObjectMetadataAPI: "Target object metadata created"
ObjectMetadataAPI-->>SetupUtil: "targetObjectMetadataId"
loop For each field type (TEXT, UUID, SELECT, etc.)
SetupUtil->>FieldMetadataAPI: "createOneFieldMetadata()"
FieldMetadataAPI->>Database: "Create field metadata"
Database-->>FieldMetadataAPI: "Field created"
FieldMetadataAPI-->>SetupUtil: "Field metadata created"
end
SetupUtil->>GraphQLAPI: "createMany() - seed test records"
GraphQLAPI->>Database: "Insert test records"
Database-->>GraphQLAPI: "Records inserted"
GraphQLAPI-->>SetupUtil: "Test data seeded"
SetupUtil-->>TestSuite: "Setup complete with object IDs"
loop For each field type and test case
alt Successful Create Input Tests
TestSuite->>GraphQLAPI: "testGqlSuccessfulScenario()"
GraphQLAPI->>ValidationEngine: "Validate input"
ValidationEngine-->>GraphQLAPI: "Input valid"
GraphQLAPI->>Database: "Create record with input"
Database-->>GraphQLAPI: "Record created"
GraphQLAPI-->>TestSuite: "Success response"
TestSuite->>RestAPI: "testRestSuccessfulScenario()"
RestAPI->>ValidationEngine: "Validate input"
ValidationEngine-->>RestAPI: "Input valid"
RestAPI->>Database: "Create record via REST"
Database-->>RestAPI: "Record created"
RestAPI-->>TestSuite: "Success response"
else
alt Failing Create Input Tests
TestSuite->>GraphQLAPI: "testGqlFailingScenario()"
GraphQLAPI->>ValidationEngine: "Validate invalid input"
ValidationEngine-->>GraphQLAPI: "Validation error"
GraphQLAPI-->>TestSuite: "Error response with message"
TestSuite->>RestAPI: "testRestFailingScenario()"
RestAPI->>ValidationEngine: "Validate invalid input"
ValidationEngine-->>RestAPI: "Validation error"
RestAPI-->>TestSuite: "Error response with message"
end
end
end
loop For each field type and filter test case
TestSuite->>GraphQLAPI: "testGqlSuccessfulFilterScenario()"
GraphQLAPI->>ValidationEngine: "Validate filter input"
ValidationEngine-->>GraphQLAPI: "Filter valid"
GraphQLAPI->>Database: "Query with filter"
Database-->>GraphQLAPI: "Filtered results"
GraphQLAPI-->>TestSuite: "Filtered records"
TestSuite->>RestAPI: "testRestSuccessfulFilterScenario()"
RestAPI->>ValidationEngine: "Validate filter input"
ValidationEngine-->>RestAPI: "Filter valid"
RestAPI->>Database: "Query with filter via REST"
Database-->>RestAPI: "Filtered results"
RestAPI-->>TestSuite: "Filtered records"
end
TestSuite->>SetupUtil: "destroyManyObjectsMetadata()"
SetupUtil->>ObjectMetadataAPI: "updateOneObjectMetadata() - deactivate"
ObjectMetadataAPI->>Database: "Deactivate objects"
Database-->>ObjectMetadataAPI: "Objects deactivated"
SetupUtil->>ObjectMetadataAPI: "deleteOneObjectMetadata()"
ObjectMetadataAPI->>Database: "Delete test objects"
Database-->>ObjectMetadataAPI: "Objects deleted"
ObjectMetadataAPI-->>SetupUtil: "Cleanup complete"
SetupUtil-->>TestSuite: "Test cleanup finished"
TestSuite-->>User: "All validation tests completed"
Additional Comments (5)
-
packages/twenty-server/test/integration/graphql/suites/inputs-validation/filter-validation/utils/test-rest-successful-scenario.util.ts, line 8 (link)style: Consider validating that the filter parameter is properly serializable before encoding to prevent runtime errors
-
packages/twenty-server/test/integration/graphql/suites/inputs-validation/filter-validation/utils/test-rest-successful-scenario.util.ts, line 21 (link)logic: The optional chaining
validateFilter?.(record)could return undefined, which would make theevery()call return false unexpectedly. Consider usingvalidateFilter && validateFilter(record)or providing a default validator -
packages/twenty-server/test/integration/graphql/suites/inputs-validation/filter-validation/utils/test-rest-failing-scenario.util.ts, line 5 (link)style: The
filterparameter usesanytype which loses type safety. Consider using a more specific type or generic constraint.Context Used: Context from
dashboard- Ensure that type assertions are safe and consider using type guards instead of direct assertions. (source) -
packages/twenty-server/test/integration/graphql/suites/inputs-validation/filter-validation/utils/test-rest-failing-scenario.util.ts, line 8 (link)logic: Consider handling potential
encodeURIComponentfailures whenfilteris not serializable to string. -
packages/twenty-server/test/integration/graphql/suites/inputs-validation/filter-validation/utils/test-rest-failing-scenario.util.ts, line 15 (link)style: Using
JSON.stringifyon error messages may be brittle. Consider checkingresponse.body.messagesdirectly or using a more robust error message validation.
56 files reviewed, 21 comments
...nputs-validation/create-validation/utils/expect-rest-create-input-validation-success.util.ts
Show resolved
Hide resolved
...nputs-validation/create-validation/utils/expect-rest-create-input-validation-success.util.ts
Show resolved
Hide resolved
...nputs-validation/create-validation/utils/expect-rest-create-input-validation-success.util.ts
Show resolved
Hide resolved
...s/inputs-validation/create-validation/utils/expect-gql-create-input-validation-error.util.ts
Show resolved
Hide resolved
...s/inputs-validation/create-validation/utils/expect-gql-create-input-validation-error.util.ts
Outdated
Show resolved
Hide resolved
...ation/create-validation/constants/successful-create-input-by-field-metadata-type.constant.ts
Outdated
Show resolved
Hide resolved
...ation/create-validation/constants/successful-create-input-by-field-metadata-type.constant.ts
Outdated
Show resolved
Hide resolved
...puts-validation/create-validation/currency-field-create-input-validation.integration-spec.ts
Show resolved
Hide resolved
...inputs-validation/create-validation/utils/expect-gql-create-input-validation-success.util.ts
Show resolved
Hide resolved
...inputs-validation/create-validation/utils/expect-gql-create-input-validation-success.util.ts
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:54160 This environment will automatically shut down when the PR is closed or after 5 hours. |
ijreilly
left a comment
There was a problem hiding this comment.
Overall looks good, but there is a lot of commented code, are they intended to be removed in the near future?
I also wonder whether we would want to tie the running of these tests to the modification of certain files - it feels like we are adding a lot lot of tests on code that we will rarely touch (dynamic schema endpoints). is this something we have impemented @prastoin / @Weiko ?
| gqlErrorMessage: 'violates not-null constraint', | ||
| restErrorMessage: 'violates not-null constraint', | ||
| }, | ||
| // { |
There was a problem hiding this comment.
what is the commented part for?
There was a problem hiding this comment.
A TODO is left, each time a scenario is comment. Commented scenarii are unregular behaviour we need to fix with validation.
After validation layer added in common, it will be uncomment (it's in common refactor scope, so comment should be removed soon).
It was easier to build all the expected behaviour and comment them when not failing. It shows what to work on for validation.
...lidation/create-validation/constants/failing-create-input-by-field-metadata-type.constant.ts
Outdated
Show resolved
Hide resolved
| gqlErrorMessage: 'violates not-null constraint', | ||
| restErrorMessage: 'violates not-null constraint', | ||
| }, | ||
| // { |
There was a problem hiding this comment.
same remark about comment
There was a problem hiding this comment.
|
|
||
| const FIELD_METADATA_TYPE = FieldMetadataType.ADDRESS; | ||
|
|
||
| const successfulTestCases = |
There was a problem hiding this comment.
what's the rationale with having only successfulTestCases for some field types and both failing and successful for others? It's because adress relies on the same rules as text field under the hood?
There was a problem hiding this comment.
Composite fields don't have failing tests as subfields rely on atomic type, already tested.
For specific composite field validation (phones, links, email) we could add failing tests. I think it can be done in a second time.
| @@ -53,7 +53,7 @@ describe(`Filter args validation - ${FIELD_METADATA_TYPE}`, () => { | |||
| // }); | |||
|
|
|||
| // // TODO : Refacto-common - Uncomment this | |||
There was a problem hiding this comment.
is this planned to be tackled later?
There was a problem hiding this comment.
Yes, all "Refacto-common" comment will be address and removed during common refactor
There was a problem hiding this comment.
Hey there, will review this PR in depth a bit later ! left few comments on the global ftm !
Cool to see strong dynamic coverage though !\
Note: centralizing every tests cases in the same files might be a bit redundant, we could store them in their dedicated tests suite instead + strict type with a centralized testCase type
...n/graphql/suites/inputs-validation/create-validation/utils/test-gql-failing-scenario.util.ts
Outdated
Show resolved
Hide resolved
...lidation/create-validation/constants/failing-create-input-by-field-metadata-type.constant.ts
Outdated
Show resolved
Hide resolved
5e257b0 to
e65e912
Compare
closes twentyhq/core-team-issues#1624