Bugfix- Fetch parent feature using topLevel flag

[shashankbrgowda]

The current behavior is returning the top-level feature (gene) for all requests, but the desired behavior is to return the immediate parent of the featureId.

[garrettjstevens]

Can we simplify this more by not even calling getFeatureFromId if topLevel is true? i.e. does this change also fix the bug you were seeing?

diff --git a/packages/apollo-collaboration-server/src/features/features.service.ts b/packages/apollo-collaboration-server/src/features/features.service.ts
--- a/packages/apollo-collaboration-server/src/features/features.service.ts
+++ b/packages/apollo-collaboration-server/src/features/features.service.ts
@@ -86,12 +86,14 @@ export class FeaturesService {
       this.logger.error(errMsg)
       throw new NotFoundException(errMsg)
     }
+    if (topLevel) {
+      return topLevelFeature
+    }
 
     // Now we need to find correct top level feature or sub-feature inside the feature
     const foundFeature = this.getFeatureFromId(
       topLevelFeature,
       featureId,
-      topLevel,
     )
     if (!foundFeature) {
       const errMsg = 'ERROR when searching feature by featureId'
@@ -111,8 +113,6 @@ export class FeaturesService {
   getFeatureFromId(
     feature: Feature,
     featureId: string,
-    topLevel?: boolean,
-    parent?: Feature | null,
   ): Feature | null {
     this.logger.verbose(`Entry=${JSON.stringify(feature)}`)
 
@@ -120,9 +120,6 @@ export class FeaturesService {
       this.logger.debug(
         `Top level featureId matches in object ${JSON.stringify(feature)}`,
       )
-      if (topLevel && parent) {
-        return parent
-      }
       return feature
     }
     // Check if there is also childFeatures in parent feature and it's not empty
@@ -134,13 +131,8 @@ export class FeaturesService {
       const subFeature = this.getFeatureFromId(
         childFeature,
         featureId,
-        topLevel,
-        feature,
       )
       if (subFeature) {
-        if (topLevel) {
-          return feature
-        }
         return subFeature
       }
     }

[shashankbrgowda]

Can we simplify this more by not even calling getFeatureFromId if topLevel is true? i.e. does this change also fix the bug you were seeing?

diff --git a/packages/apollo-collaboration-server/src/features/features.service.ts b/packages/apollo-collaboration-server/src/features/features.service.ts
--- a/packages/apollo-collaboration-server/src/features/features.service.ts
+++ b/packages/apollo-collaboration-server/src/features/features.service.ts
@@ -86,12 +86,14 @@ export class FeaturesService {
       this.logger.error(errMsg)
       throw new NotFoundException(errMsg)
     }
+    if (topLevel) {
+      return topLevelFeature
+    }
 
     // Now we need to find correct top level feature or sub-feature inside the feature
     const foundFeature = this.getFeatureFromId(
       topLevelFeature,
       featureId,
-      topLevel,
     )
     if (!foundFeature) {
       const errMsg = 'ERROR when searching feature by featureId'
@@ -111,8 +113,6 @@ export class FeaturesService {
   getFeatureFromId(
     feature: Feature,
     featureId: string,
-    topLevel?: boolean,
-    parent?: Feature | null,
   ): Feature | null {
     this.logger.verbose(`Entry=${JSON.stringify(feature)}`)
 
@@ -120,9 +120,6 @@ export class FeaturesService {
       this.logger.debug(
         `Top level featureId matches in object ${JSON.stringify(feature)}`,
       )
-      if (topLevel && parent) {
-        return parent
-      }
       return feature
     }
     // Check if there is also childFeatures in parent feature and it's not empty
@@ -134,13 +131,8 @@ export class FeaturesService {
       const subFeature = this.getFeatureFromId(
         childFeature,
         featureId,
-        topLevel,
-        feature,
       )
       if (subFeature) {
-        if (topLevel) {
-          return feature
-        }
         return subFeature
       }
     }

This will return topLevelFeature (gene) for all requests. We need to return immediate parent of requested featureId.

Sorry for the confusion with description, I've updated it now.

[garrettjstevens]

I guess I don't understand what we're trying to do. Could you provide a small example of what it's doing now and what you want it to do?

[shashankbrgowda]

The current behaviour is returning top level feature for all requests when topLevel flag is true. Here subFeature is the immediate parent of requested feature and instead of returning subFeature it returns feature which is the top level feature.

      const subFeature = this.getFeatureFromId(
        childFeature,
        featureId,
        topLevel,
        feature,
      )
      if (subFeature) {
        if (topLevel) {
          return feature
        }
        return subFeature
      }

The desired behaviour is to return the immediate parent of requested feature when topLevel flag is true and return the requested feature when the flag is false.

Ex:

• gene1
  • transcript1
    • exon1
    • cds
    • exon2
    • exon3

When topLevel flag is true,

• If the requested feature is transcript1 then return its parent feature gene1
• If the requested feature is exon1/cds/exon2/exon3 then return its parent feature transcript1

Immediate parent is determined by below base case of the getFeatureFromId recursive function.

      if (topLevel && parent) {
        return parent
      }

https://github.com/GMOD/Apollo3/blob/main/packages/apollo-collaboration-server/src/features/features.service.ts#L123-L125

Maybe the topLevel flag is causing confusion here, Shall we rename the flag as parent like before ?

[garrettjstevens]

The desired behaviour is to return the immediate parent of requested feature when topLevel flag is true and return the requested feature when the flag is false.

That wasn't the original intention of topLevel, see here: #357 (comment)

I think we need to always return the top-level feature if topLevel is true. If consumers of the endpoint need an intermediate parent/child, they can inspect the top-level feature and find it.