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 #11

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

namenu
Copy link
Member

@namenu namenu commented Jun 25, 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.
It can be ::null, or it can actually be nil.
It depends on whether the object that catches bubbled-up error is non-null or not.

@namenu namenu changed the title Fragment merge error Fixes a runtime error that occurs when deep-merging fragments Jun 26, 2024
@namenu namenu merged commit f9538ca into master Jun 26, 2024
3 checks passed
Comment on lines +420 to +422
(or (null? left) (null? right))
:com.walmartlabs.lacinia.schema/null

Choose a reason for hiding this comment

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

그냥 요러케 고쳐보면 어떨까싶은 느낌이 있습니다.. nil또는 null을 반환하게..

(cond
    (null? left)
    left

    (null? right)
    right
...

null이 반환되는 경우에는 애초에 null이 들어왔다는건데, 이건 보통 필드리졸버가 null을 임시로 넣어두고 나중에 null-collapser로 nil처리가 될거같고요,
nil이 애초에 들어왔다면 이미 null-collapser를 통과한 값이거나 unauthorized 상황인거라 그냥 nil 반환해주면되는거 아닐까 싶은 생각인데.. 테스트를 좀해봐야. ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

#12

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.

2 participants