isomerpages / isomercms-backend

A static website builder and host for the Singapore Government
5 stars 1 forks source link

feat: add checks for non-null req #1147

Closed harishv7 closed 7 months ago

harishv7 commented 8 months ago

Problem

There were 183 instances of this error in past 1 day (TypeError):

TypeError: Cannot destructure property 'pullRequestNumber' of 'req.reviewMeta' as it is null.

Closes ISOM-723

Solution

Filter the req before destructuring. Only change is to do a filter before the map

requests
        .filter((req) => {
          if (req && req.reviewMeta && req.reviewMeta.pullRequestNumber)
            return true
          return false
        })
        .map(async (req) => { ... }

Breaking Changes

kishore03109 commented 8 months ago

Hmm we had this error pop up because of an inconsistent state between db and github. Will this change make those errors now silent?

harishv7 commented 8 months ago

Hmm we had this error pop up because of an inconsistent state between db and github. Will this change make those errors now silent?

@kishore03109 Are you saying these errors were expected? If so, then I will close the PR as my understanding was this was a result of not checking the attributes of the reviewMeta properly

dcshzj commented 8 months ago

Hmm @harishv7 it seems like reviewMeta was null which resulted in this error, which means that there is a valid review request but no associated reviewMeta. This might be because there was a drift in the db somehow? There are 11,252 rows on the review_requests table but only 11,249 rows on the review_request_meta table, which means 3 review requests do not have an associated reviewMeta which can potentially throw this error.

harishv7 commented 8 months ago

Hmm @harishv7 it seems like reviewMeta was null which resulted in this error, which means that there is a valid review request but no associated reviewMeta. This might be because there was a drift in the db somehow? There are 11,252 rows on the review_requests table but only 11,249 rows on the review_request_meta table, which means 3 review requests do not have an associated reviewMeta which can potentially throw this error.

@dcshzj Yea which was weird because it's a non-null relationship that review req has reviewmeta. reflects a drift for sure, but am concerned if there is app level logic error here as well

kishore03109 commented 7 months ago

@harishv7 wait just to anchor this, we are aware of this. a fix was made here https://github.com/isomerpages/isomercms-backend/pull/1136

tldr, there was a blocking control flow that caused the drift, this has since been resolved at the app level

harishv7 commented 7 months ago

@harishv7 wait just to anchor this, we are aware of this. a fix was made here #1136

tldr, there was a blocking control flow that caused the drift, this has since been resolved at the app level

@kishore03109 understand the fix has been made at app level, but have we fixed the drift in the DB level for the records with null reviewMeta?

harishv7 commented 7 months ago

@isomerpages/iso-engineers we will need to fix the drift instead, above error is not expected in our DB state