walmartlabs / lacinia

GraphQL implementation in pure Clojure
http://lacinia.readthedocs.io/en/latest/
Other
1.82k stars 160 forks source link

Fix `can-reach-null-producer?` for union field #416

Closed igrishaev closed 2 years ago

igrishaev commented 2 years ago

Hi Walmart Team!

In Clashapp.co, we've faced an issue when the ::null values remain in the response, for example:

{:creator
 {:backgroundGradient :OCEAN,
  :bannedAt nil,
  :bio "",
  :createdAt "1970-01-01T00:00:01Z",
  :isSubscriber false},
 :createdAt "1970-01-01T00:00:00Z",
 :updatedAt "2022-05-27T10:43:26.390451Z",
 :isReported false,
 :likeCount 1,
 :text "",
 :video :com.walmartlabs.lacinia.schema/null, ;; <--- !!
 :replies nil,
 :mentions [],
 :isSubscriberOfVideoCreator false}

We expected the whole submap to collapse into nil because of the nested ::null value. At least it was so before we updated from 1.0 to 1.1.

While investigating that, I noticed that only happens to the fields which are of the union type. In our case, the field was the VideoCommentActivity which is a member of the global Activity type:

 :unions
 {:Activity
  {:members [:CommentMentionActivity
             :CommentReactionActivity
             ;; ... plenty of them
             :HuddleMentionActivity
             :HuddleReplyActivity]}

The problem is, the can-reach-null-producer? function in the com.walmartlabs.lacinia.schema namespaces doesn't take unions into account and skips them. As a result, it always returns False, so the following check:

            produces-null? (and map-type?
                                (can-reach-null-producer? schema element-def))

will always be False as well, and the null values won't be collapsed.

With this PR, the function takes into account the members of the union fields and processes them. You're welcome to review and share you feedback. Also, should I add any tests for that, please give me a hit what would the best way of doing that (what do you expect and where, etc).

Thank you, Ivan.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

igrishaev commented 2 years ago

UPD: The tests pass locally but CI fails because of some troubles with my personal CircleCI account.

hlship commented 2 years ago

The CI is a pain, as I didn't even have admin access to the repo to fix things while I was still at Walmart.

I'll check this out and run tests locally.

igrishaev commented 2 years ago

Thank you @hlship for merging this! You're right about mapv, good catch indeed.