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

Fix ObjectType casing and conflict between Relation and RelationMetadata #9849

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

FelixMalfait
Copy link
Member

@FelixMalfait FelixMalfait commented Jan 26, 2025

Fixes #9827

Also uncovered a conflict with @objectType('Relation') and @objectType('relation)

I don't want to address it in this PR so I will create a followup issue when we close this but I think there's a confusion between Relation/RelationMetadata, it's unclear what is what

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

This PR standardizes GraphQL type naming conventions across the codebase by fixing casing inconsistencies and resolving naming conflicts between 'Relation' and 'RelationMetadata'.

  • Renamed Relation to RelationMetadata in GraphQL schema and updated related mutations (e.g., createOneRelationcreateOneRelationMetadata)
  • Removed explicit name parameters from @ObjectType decorators across server entities to follow NestJS best practices
  • Updated filter type names from lowercase to PascalCase (e.g., objectFilterObjectFilter) in GraphQL queries
  • Fixed casing in metadata DTOs by changing lowercase type names to proper PascalCase (e.g., fieldField, objectObject)
  • Standardized GraphQL type definitions in generated code to use consistent PascalCase naming

31 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@twentyhq twentyhq deleted a comment from greptile-apps bot Jan 26, 2025
@magrinj
Copy link
Member

magrinj commented Jan 27, 2025

@FelixMalfait We need to be careful with renaming as it's a breaking change, I don't think people are directly using our GraphQL API for now. But in case it happens all the apps that doesn't re-generate the front-end schema will brake.

@FelixMalfait
Copy link
Member Author

@magrinj yes good point! Would you mind checking and adjusting the PR if you feel that the renaming is not the good one? I don't have much context on relations

@AMoreaux AMoreaux self-requested a review January 27, 2025 14:59
Updated GraphQL typings by removing unnecessary FeatureFlag type usage and aligning the `formatRelationMetadataInput` input structure. Simplified and reformatted mock data for better readability and maintainability.
@AMoreaux
Copy link
Contributor

I fixed the typing.

What's the decision about the breaking change @magrinj?

Update __typename literal values to use proper casing (e.g., 'Object', 'Field', 'Index', 'IndexField') for consistency with expected schema definitions. This ensures compatibility and prevents potential issues with type validation in the application.
@charlesBochet
Copy link
Member

@magrinj could you take a look
This looks tied to what @magrinj is working on. I'm not sure this is the right approach

@charlesBochet
Copy link
Member

charlesBochet commented Jan 27, 2025

Hold before merge, not sure it's the right fix to me

@charlesBochet
Copy link
Member

maybe it's the right fix actually, I leave it to @magrinj

@charlesBochet charlesBochet requested a review from magrinj January 27, 2025 15:40
Renamed `relation` to `relationMetadata` in mock variables to ensure consistency with updated schema expectations. This change aligns the mocked data structure with the actual API requirements.
@@ -17,7 +17,7 @@ import { FeatureFlagKey } from 'src/engine/core-modules/feature-flag/enums/featu
import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity';

@Entity({ name: 'featureFlag', schema: 'core' })
@ObjectType('FeatureFlag')
@ObjectType()
Copy link
Member

Choose a reason for hiding this comment

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

@FelixMalfait @AMoreaux Not sure about this one FeatureFlagEntity name will be used instead of FeatureFlag.
I don't think we're directly using for now the entity type as a graphQL DTO, but it's better to keep the name for that one

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we should just rename the class FeatureFlagEntity to FeatureFlag if we don't have any conflict with other types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to rename FeatureFlagEntity to FeatureFlag to avoid an exception for FeatureFlag.

WDYT @FelixMalfait?

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with that, let's just double check if we have any conflicts

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes great let's rename FeatureFlagEntity to FeatureFlag

Replaced all instances of `FeatureFlagEntity` with `FeatureFlag` across the codebase for consistency and better naming alignment. This change affects module imports, service injections, TypeORM entities, and GraphQL field definitions.
Renamed `FeatureFlagEntity` to `FeatureFlag` and updated related references for clarity. Refactored mutation names like `CreateOneRelationMetadata` to `CreateOneRelationMetadataItem` for better alignment with naming conventions. These changes improve readability and maintain consistency across the codebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent casing for @ObjectType
4 participants