Skip to content

GraphQL Entity Naming Changes#609

Merged
seantleonard merged 56 commits into
mainfrom
dev/seleonar/gqlNamingFix
Aug 12, 2022
Merged

GraphQL Entity Naming Changes#609
seantleonard merged 56 commits into
mainfrom
dev/seleonar/gqlNamingFix

Conversation

@seantleonard

@seantleonard seantleonard commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

Closes #601 - which identifies a bug where configuration defined entity names were not honored verbatim in GraphQL requests.

Note: This complements the work done in #619 to further honor singular/plural defined values for GraphQL entities.

  1. The GraphQL schema builder took an entity name from config and sanitized it to follow GraphQL best practices (pascal case/ upper-case first letter).
  2. Then, the schema builder used formatted name for the GQL object type definition.
  3. Using a sanitized/formatted name value would result in failed lookups within SQLMetadataProvider's EntityToDatabaseObject dictionary, when processing GraphQL requests.

Why this Change?

Our GraphQL Schema generation code takes the entity name present in our runtime config file:

"entities": {
        "todo": {
            "source": "s002.todos",
        }
    }

and formats it to adhere to the GraphQL Specification naming rules.

Names in GraphQL are limited to the Latin ASCII subset of SourceCharacter in order to support interoperation with as many other systems as possible.

and

GraphQL Naming Best Practices

Type names are in pascal case. That means they start with a capital letter. Type names should not contain whitespace dashes or underscores.

Before this change, the entity name todo was formatted to Todo (best practice rule formatting). When the formatted name was used in the SqlMetadataProvider method like: _sqlMetadataProvider.GetDatabaseObjectName("Todo"), the lookup fails due to case-sensitivity (not equal to todo).

Additionally, other schema fields that utilize the Singular/Plural naming like InputTypeFields and Query/Mutation definitions, honor the runtime config settings.

What is this Change?

Removes the name sanitization mechanism from the FormatNameForObject helper method used to convert an entityName in config to a best practice GraphQL name.

This is to adhere to decision made in this comment: #601 (comment)

"Let's generate the entity name using the same casing used in the configuration file"

Bug Fix for Improperly Generated Complex Input Types for Cosmos

A bug was also fixed which improperly created nested input type definitions for Cosmos DB as Cosmos supports nested entity mutations. In the files {Create/Update/Delete}MutationBuilder.cs, the method GetComplexInputType() improperly generated the InputTypeName using the wrong entity object. Because another method parameter, typeName, has the correct value (reference to nested entity), that value is used.
The lines of code that were corrected look like: NameNode inputTypeName = GenerateInputTypeName(typeName);

How was this tested?

  • Fix all broken GraphQL tests which had queries/mutations and references to entities with the wrong casing that did not honor the Singular/Plural name or Top-Level entity name defined in the runtime configuration.
  • Add validation to startup to ensure developers include valid GraphQL grammar in entity names.

Comment thread DataGateway.Service.GraphQLBuilder/Sql/SchemaConverter.cs Outdated
Comment thread DataGateway.Service.GraphQLBuilder/GraphQLNaming.cs Outdated

@Aniruddh25 Aniruddh25 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Need to honor singular plural values from config when generating GraphQL type name for a given entity.

@seantleonard
seantleonard marked this pull request as ready for review August 9, 2022 01:43
Comment thread src/Service/Configurations/RuntimeConfigValidator.cs Outdated
Comment thread src/Service/Configurations/RuntimeConfigValidator.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
Comment thread src/Service.Tests/Unittests/ConfigValidationUnitTests.cs Outdated
@Aniruddh25

Copy link
Copy Markdown
Collaborator

nit: edit PR description to reflect FormatNameForField is not done either

@Aniruddh25 Aniruddh25 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Waiting on

  1. clarification of model.Name property v/s model.Argument with name argument.
  2. space is invalid in names

Comment thread src/Service.GraphQLBuilder/Queries/QueryBuilder.cs Outdated
Comment thread src/Service.GraphQLBuilder/GraphQLUtils.cs Outdated

@Aniruddh25 Aniruddh25 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Thanks for streamlining our GraphQL naming conventions :)

@seantleonard
seantleonard dismissed aaronpowell’s stale review August 12, 2022 01:37

Addressed changes posted, will look into refactoring casting and received later approvals.

@seantleonard
seantleonard merged commit 7e4ce89 into main Aug 12, 2022
@seantleonard
seantleonard deleted the dev/seleonar/gqlNamingFix branch August 12, 2022 01:38
seantleonard added a commit that referenced this pull request Aug 15, 2022
* Fix naming casing issue by removing step to format entity names for GraphQL Object type definition.

* add comments.

* Add helpers to check GraphQL naming violations.

* validation of entity names for GraphQL.

* Remove commented code.

* Added comments to GraphQLNaming functions

* Remove extra name formatting and update comments.

* Add format naming back without santizing

* Updated comments and logic.

* Adding tests and comments.

* remove space

* Fix entity names to be case-sensitive to match test cases: Magazine -> magazine

* GraphQL Naming - deserialize json properly and set graphql schema naming according to config.

* Remove commented code and whitespace.

* Match test pattern, no catch block, as tests that throw exception will fail anyways.

* fix tests and merging of DAB name changes.

* Get Model name for graphQL entity in mutation engine for use in authorization resolver where entityName is needed. Move `UnderlyingGraphQLEntityType` helper to GraphQLUtils as it is used commonly across classes now.

* Update config files to set GraphQL singular plural, so that tests don't need to be rewritten. Keep PR file count smaller while utilizing functionality provided to us by flexibility of our runtime configuration.

* Update tests to use the proper casing of graphQL queries/field names to honor runtime configuration.

* Fix mutation tests.

* Fix spacing.

* Fix spacing and usings.

* fix using ordering.

* Fixing more tests. and updating configs to reflect graphqlnaming

* fix rest tests due to changed entity name "magazine" no longer having uppercase first letter.

* Fix the config files to properly set GraphQL singular/plural.

* Update model name for Cosmos GQL Schema.

* update MySql/PG config.

* Fix Cosmos Tests and Logic for building Mutations.

* Fix broken tests affected by GraphQL Naming.

* REmove unnecessary code.

* Remove old / extra code for formatting that is not used due to honoring config graphQL names verbatim.

* remove extra spacing not caught by local dotnet format. or was lost in the long log

* fix comments

* Updated comment.

* Updated comments/grammar, usage of regex to not allow spaces and fix of relevant unit tests.

* Update model directive to be properly formed.

* Conditionally validate graphql settings only if Globally enables and also enabled per entity.

* Properly format fields.

* fix entity creation in tests due to runtimeconfigvalidation.

* Update validation to include all forms of GraphQL entity settings.

* Fix cosmos query tests.

* Add capability to use graphQL settings if type is a string. Also updating some checks per comments and removed bad csproj file addition.

* removed no longer relevant tests.

* fix spacing

* Update usage of custom directive to properly cast and retrieve the value of model directive argument "name"

* simplify code.
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.

Error "Table Definition for <entity> has not been inferred."

4 participants