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: non-null field returning null sometimes triggers TypeError #209

Conversation

bbnjmn
Copy link
Contributor

@bbnjmn bbnjmn commented Jun 21, 2023

Bug description

Under some fields resolution timing conditions, non null fields that returns nulls may sometime trigger a TypeError and make a whole query fail.

This happens inremoveBranch function . If we are trying to remove a branch of tree that has already been removed by a non nullable field (a sibling of any ancestor) because it has returned null.

Proposed solution

We can protect us from TypeError by ignoring branches that are no longer part of the tree when calling removeBranch. Error will be preserved and returned back to the client to indicate that a non null field returned a null value (= same behavior as currently).

Note : I’ve split my PR into 2 commits so the first one includes only tests that are failing currently, and the second one actually contains the fix.

@bbnjmn bbnjmn changed the title Fix typeerror removing already removed branch fix: non-null field returning null sometimes triggers TypeError Jun 21, 2023
@boopathi
Copy link
Member

boopathi commented Jul 4, 2023

Thank you for your contributions with this PR. I will conduct local testing to understand the bug and its potential effects more thoroughly before continuing with the PR review.

@boopathi
Copy link
Member

👍

@bbnjmn
Copy link
Contributor Author

bbnjmn commented Nov 20, 2023

Hello @boopathi,

How can we make this PR progress as it seems it requires an additional approval ?

Thanks in advance 🙂

@bbnjmn
Copy link
Contributor Author

bbnjmn commented Mar 15, 2024

@ruiaraujo Can you check this out? Thanks 🙇

@oporkka
Copy link
Member

oporkka commented Mar 15, 2024

👍

@oporkka oporkka merged commit f0ea7ee into zalando-incubator:main Mar 15, 2024
12 checks passed
@oporkka
Copy link
Member

oporkka commented Mar 15, 2024

@bbnjmn thanks for the contribution and sorry to let you wait. I triggered already a new release

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.

TypeError: Cannot set properties of null (setting 'fieldName')
3 participants