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

Fixes a runtime error that occurs when deep-merging fragments #462

Closed
wants to merge 4 commits into from

Conversation

namenu
Copy link
Contributor

@namenu namenu commented Jun 26, 2024

Fixes a bug where inconsistent results occur based on the fragment's declared order.

I fixed a similar issue in #453, but in that case the fragment was nested, whereas this case is not.

This fails:

query MyQuery {
  node(id: "1000") {
    ... on Post {
      id
      ...PostFragment
    }
  }
}

fragment PostFragment on Post {
  author {
    alwaysFail
  }
  ...PostFragment2   # <--
}

fragment PostFragment2 on Post {
  author {
    name
  }
}

While this is not:

query MyQuery {
  node(id: "1000") {
    ... on Post {
      id
      ...PostFragment
    }
  }
}

fragment PostFragment on Post {
  ...PostFragment2   # <--
  author {
    alwaysFail
  }
}

fragment PostFragment2 on Post {
  author {
    name
  }
}

My guess is that deep-merge doesn't handle the situation where nil comes in instead of map.

(defn deep-merge
"Merges two maps together. Later map override earlier.
If a key is sequential, then each element in the list is merged."
[left right]
(cond
(or (null? left) (null? right))
Copy link
Contributor Author

@namenu namenu Jun 26, 2024

Choose a reason for hiding this comment

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

I observed that left/rigt can be ::null or nil.
It depends on whether the object that catches bubbled-up error is non-null or not.

Test case in L169 covers this.

@namenu namenu closed this Jun 27, 2024
@hlship hlship added the bug label Jun 28, 2024
@hlship hlship added this to the 1.2.3 milestone Jun 28, 2024
@hlship
Copy link
Member

hlship commented Jun 28, 2024

I'll review this once you re-open it.

@namenu
Copy link
Contributor Author

namenu commented Jun 28, 2024

@hlship Thanks. We have encountered a regression bug after applying this changes. I am not confident much so that I can handle this problem since it requires profound understanding of the internal design.
I'm still finding the way how to reproduce the bug. If you have any idea about this please let me know.

@namenu
Copy link
Contributor Author

namenu commented Jul 3, 2024

Reopened #463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants