Null equivalence - remove feature flag#16222
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| return transformFullNameField(validatedValue, isNullEquivalenceEnabled); | ||
| return transformFullNameField(validatedValue); | ||
| } | ||
|
|
||
| case FieldMetadataType.ADDRESS: { | ||
| const validatedValue = validateAddressFieldOrThrow(value, key); | ||
|
|
||
| return transformAddressField(validatedValue, isNullEquivalenceEnabled); | ||
| return transformAddressField(validatedValue); | ||
| } | ||
| case FieldMetadataType.CURRENCY: { | ||
| const validatedValue = validateCurrencyFieldOrThrow(value, key); |
There was a problem hiding this comment.
Bug: transformEmailsValue lacks null equivalence logic, preventing conversion of empty email values to null.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The transformEmailsValue function fails to convert null-equivalent email values (e.g., empty strings or objects) to null. This occurs because its implementation was not updated to include null equivalence logic, despite the call site in data-arg-processor.ts being modified to remove the isNullEquivalenceEnabled parameter. This omission leads to inconsistent data handling for email fields, as they will retain their null-equivalent values instead of being normalized to null, breaking the intended behavior of the pull request.
💡 Suggested Fix
Implement null equivalence logic within transformEmailsValue, potentially by calling a new isNullEquivalentEmailFieldValue utility function, to ensure empty email values are correctly converted to null.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/data-arg.processor.ts#L241-L258
Potential issue: The `transformEmailsValue` function fails to convert null-equivalent
email values (e.g., empty strings or objects) to `null`. This occurs because its
implementation was not updated to include null equivalence logic, despite the call site
in `data-arg-processor.ts` being modified to remove the `isNullEquivalenceEnabled`
parameter. This omission leads to inconsistent data handling for email fields, as they
will retain their null-equivalent values instead of being normalized to `null`, breaking
the intended behavior of the pull request.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4609673
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Enum value IS_NULL_EQUIVALENCE_ENABLED was removed from enum FeatureFlagKey GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Enum value IS_NULL_EQUIVALENCE_ENABLED was removed from enum FeatureFlagKey
|
Greptile OverviewGreptile SummaryThis PR removes the The changes span multiple areas: removing the feature flag from enums, removing it from default workspace feature flags, removing feature flag parameters from data transformation utilities (text fields, arrays, currency, full names, actors, emails, raw JSON, and addresses), updating test files to match the new function signatures, and cleaning up related migration commands. Additionally, a utility file This represents the final step in graduating the null equivalence feature from experimental/feature-flagged status to permanent functionality, indicating the feature has been thoroughly tested and validated in production environments. Important Files Changed
Confidence score: 4/5
Sequence DiagramsequenceDiagram
participant User
participant "Pull Request System" as PR
participant "GraphQL Schema" as GQL
participant "Feature Flag Service" as FFS
participant "Data Processor" as DP
participant "Database" as DB
User->>PR: "Merges PR to remove IS_NULL_EQUIVALENCE_ENABLED"
PR->>GQL: "Updates FeatureFlagKey enum"
GQL-->>GQL: "Removes IS_NULL_EQUIVALENCE_ENABLED from enum"
Note over FFS: Feature flag no longer exists in system
User->>DP: "Submits data with null equivalent values"
DP->>DP: "Processes field transformations"
DP->>DP: "Applies null equivalence logic unconditionally"
Note over DP: transformTextField, transformArrayField, etc.<br/>now always convert empty values to null
DP->>DB: "Stores transformed data with null values"
DB-->>DP: "Confirms data stored"
DP-->>User: "Returns processed record"
Note over User,DB: Null equivalence behavior is now<br/>permanently enabled for all workspaces
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
packages/twenty-server/src/database/commands/upgrade-version-command/1-12/1-12-clean-null-equivalent-values.ts, line 56-57 (link)style: Feature flag repository is still injected but no longer used in the code
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
22 files reviewed, 2 comments
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| it('should return the string when value is empty array', () => { |
There was a problem hiding this comment.
syntax: Test description says 'return the string' but expects null
| it('should return the string when value is empty array', () => { | |
| it('should return null when value is empty array', () => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/transformer-utils/__tests__/transform-raw-json-field.util.spec.ts
Line: 16:16
Comment:
**syntax:** Test description says 'return the string' but expects null
```suggestion
it('should return null when value is empty array', () => {
```
How can I resolve this? If you propose a fix, please make it concise.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:28479 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
Hey @etiennejouan! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
2 participants
No description provided.