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

ConvertEmptyStringsToNullDirective not executed when applied on fields where the argument is an input type #2610

Closed
dennis-koster opened this issue Sep 5, 2024 · 4 comments
Labels
bug An error within Lighthouse

Comments

@dennis-koster
Copy link
Contributor

dennis-koster commented Sep 5, 2024

Describe the bug
The ConvertEmptyStringsToNullDirective is not executed when it is applied globally or to an input, where the expected behaviour would be that it would apply the conversion recursively. It appears to me that this change has introduced this bug, because the added if check now makes it so that the sanitize method is only applied if the type of input is a string, negating the intended recursive nature.

Expected behavior/Solution
The directive should be executed when applied to an input or globally, as described in the documentation.

Steps to reproduce

Given the following schema:

extend type Mutation @guard {
    updateMe(input: UpdateMeInput): User!
        @field(resolver: "App\\GraphQL\\Mutations\\UpdateMe")
}

input UpdateMeInput {
    firstName: String @rules(apply: ["filled"])
    insertion: String @rules(apply: ["nullable"])
    lastName: String @rules(apply: ["filled"])
}

Execute the following mutation:

updateMe(input: {
    insertion: "",
}) {
    insertion
}

Expected outcome would be for the insertion attribute to be converted to null in the following scenarios:
1.\Nuwave\Lighthouse\Schema\Directives\ConvertEmptyStringsToNullDirective::class, is added to the field_middleware array in the lighthouse.php configuration file.
2. The directive is applied to the input as such:

extend type Mutation {
    updateMe(input: UpdateMeInput): User! @convertEmptyStringsToNull
        @field(resolver: "App\\GraphQL\\Mutations\\UpdateMe")
}

Lighthouse Version
v6.43.1

@spawnia spawnia added the needs reproduction Failing test case needed label Sep 5, 2024
@spawnia
Copy link
Collaborator

spawnia commented Sep 5, 2024

Can you add provide a pull request with a failing test case?

@dennis-koster
Copy link
Contributor Author

dennis-koster commented Sep 5, 2024

@spawnia Let me know if this is sufficient for you to reproduce the error: #2611.

It seems like situations where the @field directive is used are not covered.

@spawnia
Copy link
Collaborator

spawnia commented Sep 6, 2024

The issue in your tests was that @convertEmptyStringsToNull does not apply to non-nullable inputs when used on fields, see 5ee83d5. I improved the docs for this in 45e1cbb.

Please let me know if there is another issue, and please consider sponsoring.

@spawnia spawnia closed this as completed Sep 6, 2024
@spawnia spawnia reopened this Sep 6, 2024
@spawnia spawnia changed the title ConvertEmptyStringsToNullDirective not executed when applied globally or to input ConvertEmptyStringsToNullDirective not executed when applied on fields where the argument is an input type Sep 6, 2024
@spawnia
Copy link
Collaborator

spawnia commented Sep 6, 2024

Thank you for your persistence @dennis-koster, fixed with https://github.com/nuwave/lighthouse/releases/tag/v6.44.2.

@spawnia spawnia closed this as completed Sep 6, 2024
@spawnia spawnia added bug An error within Lighthouse and removed needs reproduction Failing test case needed labels Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error within Lighthouse
Projects
None yet
Development

No branches or pull requests

2 participants