Fix genbank.Feature.GetSequence() error handling

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
  - Made it possible to simulate primers shorter than design minimum.
+ - `genbank.Feature.GetSequence()` and `polyjson.Feature.GetSequence()` now return a real error instead of always `nil`, for a feature with no parent sequence or a location out of bounds of its parent sequence, instead of panicking. (#352)
 
 ## [0.31.1] - 2024-01-31
 

io/genbank/example_test.go

@@ -166,3 +166,16 @@ func ExampleFeature_GetSequence() {
 
 	// Output: true
 }
+
+// This example shows that GetSequence returns an error, instead of
+// panicking, if the feature has not been added to a parent sequence.
+func ExampleFeature_GetSequence_error() {
+	var feature genbank.Feature
+	feature.Location.Start = 0
+	feature.Location.End = 10
+
+	_, err := feature.GetSequence()
+	fmt.Println(err)
+
+	// Output: feature '' has no parent sequence to get its sequence from
+}

io/genbank/genbank.go

@@ -141,13 +141,23 @@ func (feature Feature) GetSequence() (string, error) {
 func getFeatureSequence(feature Feature, location Location) (string, error) {
 	var sequenceBuffer bytes.Buffer
 	var sequenceString string
+
+	if feature.ParentSequence == nil {
+		return "", fmt.Errorf("feature '%s' has no parent sequence to get its sequence from", feature.Type)
+	}
 	parentSequence := feature.ParentSequence.Sequence
 
 	if len(location.SubLocations) == 0 {
+		if location.Start < 0 || location.End > len(parentSequence) || location.Start > location.End {
+			return "", fmt.Errorf("location [%d:%d] is out of bounds for parent sequence of length %d", location.Start, location.End, len(parentSequence))
+		}
 		sequenceBuffer.WriteString(parentSequence[location.Start:location.End])
 	} else {
 		for _, subLocation := range location.SubLocations {
-			sequence, _ := getFeatureSequence(feature, subLocation)
+			sequence, err := getFeatureSequence(feature, subLocation)
+			if err != nil {
+				return "", err
+			}
 
 			sequenceBuffer.WriteString(sequence)
 		}

io/genbank/genbank_test.go

@@ -206,6 +206,37 @@ func TestGetSequenceMethod(t *testing.T) {
 	}
 }
 
+func TestGetSequenceMethod_Errors(t *testing.T) {
+	gbk, _ := Read("../../data/t4_intron.gb")
+
+	// A feature with no parent sequence should return an error rather than
+	// panicking with a nil pointer dereference.
+	orphanFeature := gbk.Features[1]
+	orphanFeature.ParentSequence = nil
+	if _, err := orphanFeature.GetSequence(); err == nil {
+		t.Error("Expected an error when getting the sequence of a feature with no parent sequence, got nil")
+	}
+
+	// A location outside the bounds of the parent sequence should return an
+	// error rather than panicking with an index out of range.
+	outOfBoundsFeature := gbk.Features[1]
+	outOfBoundsFeature.Location.End = len(outOfBoundsFeature.ParentSequence.Sequence) + 1000
+	if _, err := outOfBoundsFeature.GetSequence(); err == nil {
+		t.Error("Expected an error when getting the sequence of a feature with an out-of-bounds location, got nil")
+	}
+
+	// An out-of-bounds sub-location nested inside a joined feature should
+	// have its error propagated up, rather than being silently discarded.
+	joinFeature := gbk.Features[6] // join(893..1441,2459..2770)
+	subLocations := make([]Location, len(joinFeature.Location.SubLocations))
+	copy(subLocations, joinFeature.Location.SubLocations)
+	subLocations[0].End = len(joinFeature.ParentSequence.Sequence) + 1000
+	joinFeature.Location.SubLocations = subLocations
+	if _, err := joinFeature.GetSequence(); err == nil {
+		t.Error("Expected an error when a joined feature's sub-location is out of bounds, got nil")
+	}
+}
+
 func TestLocationParser(t *testing.T) {
 	gbk, _ := Read("../../data/t4_intron.gb")
 

io/polyjson/example_test.go

@@ -151,3 +151,16 @@ func ExampleFeature_GetSequence() {
 
 	// Output: true
 }
+
+// This example shows that GetSequence returns an error, instead of
+// panicking, if the feature has not been added to a parent sequence.
+func ExampleFeature_GetSequence_error() {
+	var feature polyjson.Feature
+	feature.Location.Start = 0
+	feature.Location.End = 10
+
+	_, err := feature.GetSequence()
+	fmt.Println(err)
+
+	// Output: feature '' has no parent sequence to get its sequence from
+}

io/polyjson/polyjson.go

@@ -9,6 +9,7 @@ package polyjson
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"io"
 	"os"
 	"time"
@@ -86,14 +87,21 @@ func (feature Feature) GetSequence() (string, error) {
 func getFeatureSequence(feature Feature, location Location) (string, error) {
 	var sequenceBuffer bytes.Buffer
 	var sequenceString string
+
+	if feature.ParentSequence == nil {
+		return "", fmt.Errorf("feature '%s' has no parent sequence to get its sequence from", feature.Name)
+	}
 	parentSequence := feature.ParentSequence.Sequence
 
 	if len(location.SubLocations) == 0 {
+		if location.Start < 0 || location.End > len(parentSequence) || location.Start > location.End {
+			return "", fmt.Errorf("location [%d:%d] is out of bounds for parent sequence of length %d", location.Start, location.End, len(parentSequence))
+		}
 		sequenceBuffer.WriteString(parentSequence[location.Start:location.End])
 	} else {
 		for _, subLocation := range location.SubLocations {
 			sequence, err := getFeatureSequence(feature, subLocation)
-			if err != nil { // todo: test error
+			if err != nil {
 				return "", err
 			}
 			sequenceBuffer.WriteString(sequence)

io/polyjson/polyjson_test.go

@@ -66,6 +66,45 @@ func TestFeature_GetSequence(t *testing.T) {
 	}
 }
 
+func TestFeature_GetSequence_Errors(t *testing.T) {
+	var sequence Poly
+	sequence.Sequence = "ATGGCTAGCAAAGGAGAAGAACTTTTCACTGGAGTTGTCCCAATTCTTGTTGAATTAGATGGTGATGTT"
+
+	var feature Feature
+	feature.Location.Start = 0
+	feature.Location.End = 10
+	_ = sequence.AddFeature(&feature)
+
+	// A feature with no parent sequence should return an error rather than
+	// panicking with a nil pointer dereference.
+	orphanFeature := feature
+	orphanFeature.ParentSequence = nil
+	if _, err := orphanFeature.GetSequence(); err == nil {
+		t.Error("Expected an error when getting the sequence of a feature with no parent sequence, got nil")
+	}
+
+	// A location outside the bounds of the parent sequence should return an
+	// error rather than panicking with an index out of range.
+	outOfBoundsFeature := feature
+	outOfBoundsFeature.Location.End = len(sequence.Sequence) + 1000
+	if _, err := outOfBoundsFeature.GetSequence(); err == nil {
+		t.Error("Expected an error when getting the sequence of a feature with an out-of-bounds location, got nil")
+	}
+
+	// An out-of-bounds sub-location nested inside a joined feature should
+	// have its error propagated up, rather than being silently discarded.
+	joinFeature := feature
+	joinFeature.Location.Start = 0
+	joinFeature.Location.End = 0
+	joinFeature.Location.SubLocations = []Location{
+		{Start: 0, End: 5},
+		{Start: 5, End: len(sequence.Sequence) + 1000},
+	}
+	if _, err := joinFeature.GetSequence(); err == nil {
+		t.Error("Expected an error when a joined feature's sub-location is out of bounds, got nil")
+	}
+}
+
 func TestParse_error(t *testing.T) {
 	unmarshalErr := errors.New("unmarshal error")
 	oldUnmarshalFn := unmarshalFn