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 can-reach-null-producer? for union field #416

Merged

Conversation

igrishaev
Copy link
Contributor

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
Copy link

CLAassistant commented May 30, 2022

CLA assistant check
All committers have signed the CLA.

@igrishaev
Copy link
Contributor Author

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

@hlship
Copy link
Member

hlship commented Jul 20, 2022

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.

@hlship hlship self-assigned this Jul 20, 2022
@hlship hlship added the bug label Jul 20, 2022
@hlship hlship added this to the 1.2 milestone Jul 20, 2022
Copy link
Member

@hlship hlship left a comment

Choose a reason for hiding this comment

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

Might have used a map or mapv instead of a for, but since this is in schema compile stage (just once) it's fine. If I had been more prompt in reviewing, I would have insisted on some kind of test, but I'll add that.

@hlship hlship merged commit 76b25fa into walmartlabs:master Aug 12, 2022
@igrishaev
Copy link
Contributor Author

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

@igrishaev igrishaev deleted the fix-can-reach-null-producer-for-union branch August 12, 2022 16:10
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.

None yet

3 participants