Spectrum: Filter deleted channels everywhere

[dmitryyankowski]

Note: Running locally, with a fresh install/migration/seed and no modifications to code.

When I click a specific community on the index page sidebar I get the following error in the API console.

Error: Cannot return null for non-nullable field Thread.channel.

The client looks like the following:

When I click on a channel however.. it loads all the threads fine. Also..when I click everything..it loads threads fine as-well!

Note: On https://spectrum.chat it loads all the threads fine when you click on the community in the sidebar.

Any ideas as to what the issue is? Thanks for any help!

[brianlovin]

Hm, this is a strange one, and actually an error that I'm seeing come up in production every once in a while. The reason would be that in the graphql resolver the thread.channel returns null but we mark it as a required return in the schema.

This could possibly be caused by a deleted channel that we are somehow fetching a thread for? I'm not totally sure - if you happen to find any more context here, it'd be awesome to know!

[a-golovanov]

We traced this issue down to execution of query getCommunityThreadConnection($id: ID, $after: String). It calls api/models/channel.js: getChannelsByUserAndCommunity which doesn't filter out deleted channels, while api/models/channel.js: getChannels does. So, if a deleted channel contains a thread not marked as deleted, that thread will end up in the query result and resolving its channel field will return null and fail.

This problem is present in the following channel from the seed, which contains 6 threads none of which are marked as deleted:

{
"communityId":  "1" ,
"createdAt": Sat Dec 31 2016 23:00:00 GMT+00:00 ,
"deletedAt": Sat Dec 31 2016 23:00:00 GMT+00:00 ,
"description":  "Testing deleted channel" ,
"id":  "6" ,
"isDefault": false ,
"isPrivate": false ,
"name":  "Deleted" ,
"slug":  "deleted"
}

Hope this helps!

[mxstbr]

@a-golovanov amazing work! Mind submitting a PR that filters deleted channels from getChannelsByUserAndCommunity?

[brianlovin]

💜💜💜

[dmitryyankowski]

Sorry for closing this.. after posting this I realized I was running a really old version and it was showing this bug. After updating I stopped seeing the error :) But glad to see that it is actually a bug that still exists for other people and we can get it fixed +1

[a-golovanov]

I started to look at this and I've got a few questions:

• Should model methods that return data for channels with provided ids filter out deleted channels?
  
  
  
  • getChannels (this one does the rest do not)
  
  
  • getChannelById
  
  
  • getChannelMetaData
  
  
  • getChannelsThreadCounts
  
  
  • getChannelsMemberCounts
  
  
• Should model methods that return data based on threads filter out deleted threads?
  
  
  
  • getChannelMetaData
  
  
  • getChannelsThreadCounts
  
  

[a-golovanov]

Also, getChannelsMemberCounts currently excludes not members while getChannelMetaData doesn't, should getChannelMetaData exclude not members as well?

[mxstbr]

@a-golovanov yes to all of these!

[brianlovin]

@a-golovanov yeah, agreed we should filter out all deleted channels. I think in the future if we ever need to query deleted records the model function itself should contain the explicit-ness, e.g. getChannelsByIdIncludingDeleted or something super obvious. We don't have any need for that yet, but...just as a pattern :)

[a-golovanov]

While working on the PR I discovered that the similar issue is present practically in every model, including threads, userThreads, userChannels, e.t.c. Also, channel model methods do not check if its community is deleted and deleting a community doesn't mark its channels as deleted.

This PR doesn't address these issues as I didn't want to postpone it and don't have enough time to pull through all these issues in one sit.

[brianlovin]

Also, channel model methods do not check if its community is deleted and deleting a community doesn't mark its channels as deleted

Damn, this is a good find. I'll dig into this and write a migration to backfill things we've missed!

[a-golovanov]

After some additional consideration, I think it's impractical to add checks for deleted parent objects to all models. An invariant that all children of a deleted parent are marked as deleted as well seems to be more practical. But I'm not sure how to enforce such invariant though.