-
Notifications
You must be signed in to change notification settings - Fork 249
Prevent empty input node generation in mutation builder. #2729
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the GraphQL mutation builders for update/patch and create operations by converting several input type generation methods to return nullable values and adding corresponding null checks. Key changes include updating method signatures to support nullable returns, inserting conditional blocks to handle cases where input types may not be generated, and adjusting related logic in both UpdateAndPatchMutationBuilder and CreateMutationBuilder.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/Service.GraphQLBuilder/Mutations/UpdateAndPatchMutationBuilder.cs | Updated method signatures (e.g. GenerateUpdateInputType and GetComplexInputType) and added null checks to improve null safety when generating update/patch input types. |
src/Service.GraphQLBuilder/Mutations/CreateMutationBuilder.cs | Updated method signatures (e.g. GenerateCreateInputTypeForRelationalDb and GenerateComplexInputTypeForRelationalDb) and added conditional checks to ensure that input objects are only used when successfully created. |
Comments suppressed due to low confidence (2)
src/Service.GraphQLBuilder/Mutations/UpdateAndPatchMutationBuilder.cs:95
- Consider defining 'inputFields' as a List rather than a List<InputValueDefinitionNode?> if null values are not expected. This change would eliminate the need to use the null-forgiving operator and improve type safety.
List<InputValueDefinitionNode> inputFieldsList = inputFields.Select(i => i!).ToList();
src/Service.GraphQLBuilder/Mutations/CreateMutationBuilder.cs:77
- Consider updating the declaration of 'inputFields' to use a non-nullable list if null entries are not expected. This revision would remove the need for the null-forgiving operator and enhance code clarity.
inputFields.AddRange(inputFields!)
/azp |
Supported commands
See additional documentation. |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
src/Service.GraphQLBuilder/Mutations/UpdateAndPatchMutationBuilder.cs
Outdated
Show resolved
Hide resolved
@Alekhya-Polavarapu could you update the description for the PR? I see that you wanted to create some new lines but it shows as |
if (inputFields.Any()) | ||
{ | ||
List<InputValueDefinitionNode> inputFieldsList = inputFields | ||
.Select(i => i!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the inputFields IEnumerable contains some null and some non-null fields based on if GenerateSimpleType returns null or non-null? Can we still safely cast it to ignore the null by doing i => i!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i understand your comment correct, inputFields IEnumerable ends up with null node fields only in the case of complex object and the complex object has all fields auto generated inside of it and in that case it ends up with null and we can safely ignore those null's when creating the updateInput for original type.
otherthan that, i don't think Null's can end up in IEnumerable, even if in some edge case, it can be safely ignored.
example: i have a type publisher_PP which has only auto generated fields, i defined a relationship with Publisher_PP and Book for Publisher, and for the creaatePubliser, i do not see Publihser_PP showing up(which is getting filtered as it returned with null in the IEnumerable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The graphic you showed makes sense for the createPublisher
mutation. But if you test updatePublisher
workflow, updatePublisher_PP
will be null, as you rightly pointed out -> it will be null if complex object has all fields auto generated inside of it.
Now isnt the following statement saying use the null updatePublisher_PP
input type and other fields of the original type Publisher to convert it into a list?
List<InputValueDefinitionNode> inputFieldsList = inputFields .Select(i => i!)
I think this should instead be .Where(i => i !=null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but waiting for clarification on one of the comments.
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
thats actually part of error trace, i updated the error trace within code block |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
@@ -194,6 +194,39 @@ type Foo @model(name:""Foo"") { | |||
Assert.AreEqual("bar", argType.Fields[0].Name.Value); | |||
} | |||
|
|||
[TestMethod] | |||
[TestCategory("Mutation Builder - Create")] | |||
public void CreateMutationExcludeIdForAllAutogeneratedFields() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a similar test for UpdateMutationExcludeIdForAllAutogeneratedFields() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i asserted the update and delete in the same test, i missed adding the correspodning test category, will add them.
please let me know if am missing anything!
Why make this change?
Currently today when the tables has only autogenerated fileds, then the schema generation is failing with an error saying that "Empty input for create and update mutation input".
Error trace:
For more details look at the
Errorsproperty.\r\n\r\n1. InputObject
CreateNewTableInputhas no fields declared. (HotChocolate.Types.InputObjectType)\r\n2. InputObject
UpdateNewTableInputhas no fields declared.
#2739
What is this change?
This PR addresses the issue by conditionally generating create and update mutation input types only when the table contains at least one non-auto-generated field. This ensures that the schema remains valid and avoids generating empty input objects.
How was this tested?
screenshots
For create:
for update: