diff --git a/internal/app/service/feature_annotation.go b/internal/app/service/feature_annotation.go index 94d4317..7ae1886 100644 --- a/internal/app/service/feature_annotation.go +++ b/internal/app/service/feature_annotation.go @@ -281,6 +281,7 @@ func (srv *FeatureAnnotationService) RemoveTags( } // ListFeatureAnnotationsByPubmedId lists all feature annotations associated with a PubMed ID. +// //nolint:staticcheck // must match gRPC-generated interface name func (srv *FeatureAnnotationService) ListFeatureAnnotationsByPubmedId( ctx context.Context, diff --git a/internal/repository/arangodb/annotation_read.go b/internal/repository/arangodb/annotation_read.go index 4de623d..d037188 100644 --- a/internal/repository/arangodb/annotation_read.go +++ b/internal/repository/arangodb/annotation_read.go @@ -74,6 +74,34 @@ func (ar *arangorepository) GetAnnotationByEntry( return mann, nil } +// buildListAnnoBindVars constructs the AQL bind variable map for +// ListAnnotations. The set of collection bind parameters depends on the +// statement type: SecondFilter queries traverse cvterm first (no +// @@anno_collection), while all other types traverse annotations first (no +// @@cvterm_collection). Providing an unreferenced key causes ArangoDB to +// reject the query, so the two collection parameters are mutually exclusive. +func buildListAnnoBindVars( + stmtType StatementType, + annoCollection, cvCollection, cvtermCollection, graphName string, + limit, cursor int64, +) map[string]any { + bindVars := map[string]any{ + cvCollectionBind: cvCollection, + "anno_cvterm_graph": graphName, + "limit": limit + 1, + } + if cursor != 0 { + bindVars["cursor"] = cursor + } + if stmtType == SecondFilter { + bindVars[cvtermCollectionBind] = cvtermCollection + } else { + bindVars["@anno_collection"] = annoCollection + } + + return bindVars +} + func (ar *arangorepository) ListAnnotations( params *repository.ListAnnotationsParams, ) ([]*model.AnnoDoc, error) { @@ -81,22 +109,19 @@ func (ar *arangorepository) ListAnnotations( return nil, fmt.Errorf("error in valdating parameters %w", err) } annoModel := make([]*model.AnnoDoc, 0) - bindVars := map[string]any{ - "@anno_collection": ar.anno.annot.Name(), - cvCollectionBind: ar.onto.Cv.Name(), - "anno_cvterm_graph": ar.anno.annotg.Name(), - "limit": params.Limit + 1, - } - if params.Cursor != 0 { - bindVars["cursor"] = params.Cursor - } result := getListAnnoStatement(params.Filter, params.Cursor) - if result.Type == SecondFilter { - bindVars[cvtermCollectionBind] = ar.onto.Term.Name() - } if result.Err != nil { return nil, result.Err } + bindVars := buildListAnnoBindVars( + result.Type, + ar.anno.annot.Name(), + ar.onto.Cv.Name(), + ar.onto.Term.Name(), + ar.anno.annotg.Name(), + params.Limit, + params.Cursor, + ) res, err := ar.database.SearchRows(result.Statement, bindVars) if err != nil { return annoModel, fmt.Errorf("error in searching rows %s", err) diff --git a/internal/repository/arangodb/annotation_read_bind_test.go b/internal/repository/arangodb/annotation_read_bind_test.go new file mode 100644 index 0000000..8aa4ea2 --- /dev/null +++ b/internal/repository/arangodb/annotation_read_bind_test.go @@ -0,0 +1,93 @@ +package arangodb + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestBuildListAnnoBindVars(t *testing.T) { + t.Parallel() + const ( + anno = "annotation" + cv = "cv" + cvterm = "cvterm" + graph = "anno_cvterm_graph" + limit = int64(10) + ) + + t.Run( + "FirstFilter includes anno_collection and excludes cvterm_collection", + func(t *testing.T) { + t.Parallel() + assert := require.New(t) + vars := buildListAnnoBindVars(FirstFilter, anno, cv, cvterm, graph, limit, 0) + assert.Equal(anno, vars["@anno_collection"]) + assert.Equal(cv, vars[cvCollectionBind]) + assert.NotContains(vars, cvtermCollectionBind) + assert.NotContains(vars, "cursor") + assert.Equal(limit+1, vars["limit"]) + }, + ) + + t.Run( + "SecondFilter excludes anno_collection and includes cvterm_collection", + func(t *testing.T) { + t.Parallel() + assert := require.New(t) + vars := buildListAnnoBindVars(SecondFilter, anno, cv, cvterm, graph, limit, 0) + assert.NotContains(vars, "@anno_collection") + assert.Equal(cv, vars[cvCollectionBind]) + assert.Equal(cvterm, vars[cvtermCollectionBind]) + assert.NotContains(vars, "cursor") + }, + ) + + t.Run( + "BothFilters includes anno_collection and excludes cvterm_collection", + func(t *testing.T) { + t.Parallel() + assert := require.New(t) + vars := buildListAnnoBindVars(BothFilters, anno, cv, cvterm, graph, limit, 0) + assert.Equal(anno, vars["@anno_collection"]) + assert.Equal(cv, vars[cvCollectionBind]) + assert.NotContains(vars, cvtermCollectionBind) + }, + ) + + t.Run("cursor key is present when cursor is non-zero", func(t *testing.T) { + t.Parallel() + assert := require.New(t) + vars := buildListAnnoBindVars(FirstFilter, anno, cv, cvterm, graph, limit, 12345) + assert.Equal(int64(12345), vars["cursor"]) + }) + + t.Run("cursor key is absent when cursor is zero", func(t *testing.T) { + t.Parallel() + assert := require.New(t) + vars := buildListAnnoBindVars(SecondFilter, anno, cv, cvterm, graph, limit, 0) + assert.NotContains(vars, "cursor") + }) + + t.Run( + "SecondFilter with cursor includes cvterm_collection and cursor but not anno_collection", + func(t *testing.T) { + t.Parallel() + assert := require.New(t) + vars := buildListAnnoBindVars(SecondFilter, anno, cv, cvterm, graph, 5, 99999) + assert.NotContains(vars, "@anno_collection") + assert.Equal(cvterm, vars[cvtermCollectionBind]) + assert.Equal(int64(99999), vars["cursor"]) + assert.Equal(int64(6), vars["limit"]) + }, + ) + + t.Run("anno_cvterm_graph is always present", func(t *testing.T) { + t.Parallel() + assert := require.New(t) + for _, stype := range []StatementType{FirstFilter, SecondFilter, BothFilters} { + vars := buildListAnnoBindVars(stype, anno, cv, cvterm, graph, limit, 0) + assert.Equal(graph, vars["anno_cvterm_graph"], "graph missing for type %s", stype) + } + }) +} diff --git a/internal/repository/arangodb/annotation_read_test.go b/internal/repository/arangodb/annotation_read_test.go index bf26e1c..dbe7f31 100644 --- a/internal/repository/arangodb/annotation_read_test.go +++ b/internal/repository/arangodb/annotation_read_test.go @@ -10,9 +10,10 @@ import ( ) const ( - filterOne = `entry_id==DDB_G0286429;tag==private note;ontology==dicty_annotation` - filterTwo = `entry_id==DDB_G0294491;tag==name description;ontology==dicty_annotation` - filterThree = `entry_id==jumbo` + filterOne = `entry_id==DDB_G0286429;tag==private note;ontology==dicty_annotation` + filterTwo = `entry_id==DDB_G0294491;tag==name description;ontology==dicty_annotation` + filterThree = `entry_id==jumbo` + filterTagOnly = `tag==private note;ontology==dicty_annotation` ) //nolint:tparallel @@ -610,3 +611,95 @@ func testAddAnnotationSuccessThird( ) assert.Equal(nta.Data.Attributes.Tag, annm.Tag, "should match the tag") } + +// TestListAnnoTagOnlyFilter exercises the SecondFilter (tag+ontology only, no +// entry_id) code path in ListAnnotations — the path that previously failed +// with AQL bind parameter errors. +// +//nolint:tparallel +func TestListAnnoTagOnlyFilter(t *testing.T) { + t.Parallel() + assert, anrepo := setUp(t) + defer tearDown(anrepo) + + tal := newTestTaggedAnnotationsListForFiltering(10) + for _, anno := range tal { + _, err := anrepo.AddAnnotation(anno) + assert.NoErrorf(err, "setup: expect no error adding annotation, received %s", err) + } + + var firstPage []*model.AnnoDoc + + t.Run("FirstPage", func(t *testing.T) { + firstPage = testListAnnoTagOnlyFirstPage(t, assert, anrepo) + }) + + t.Run("SecondPage", func(t *testing.T) { + testListAnnoTagOnlySecondPage(t, assert, anrepo, firstPage) + }) + + t.Run("NoResults", func(t *testing.T) { + testListAnnoTagOnlyNotFound(t, assert, anrepo) + }) +} + +func testListAnnoTagOnlyFirstPage( + t *testing.T, + assert *require.Assertions, + anrepo repository.TaggedAnnotationRepository, +) []*model.AnnoDoc { + t.Helper() + mla, err := anrepo.ListAnnotations( + &repository.ListAnnotationsParams{Limit: 3, Filter: filterTagOnly}, + ) + assert.NoErrorf(err, "SecondFilter query must not return AQL bind error, got %s", err) + assert.NotEmpty(mla, "should return at least one annotation") + for _, m := range mla { + assert.Equal(tags[0], m.Tag, "tag should match filter") + assert.Equal("dicty_annotation", m.Ontology, "ontology should match filter") + } + testModelListSort(t, mla) + + return mla +} + +func testListAnnoTagOnlySecondPage( + t *testing.T, + assert *require.Assertions, + anrepo repository.TaggedAnnotationRepository, + prevResult []*model.AnnoDoc, +) { + t.Helper() + assert.NotEmpty(prevResult, "previous result should not be empty") + ml2, err := anrepo.ListAnnotations(&repository.ListAnnotationsParams{ + Cursor: toTimestamp(prevResult[len(prevResult)-1].CreatedAt), + Limit: 3, + Filter: filterTagOnly, + }) + assert.NoErrorf( + err, + "SecondFilter+cursor query must not return AQL bind error, got %s", + err, + ) + assert.Exactly(prevResult[len(prevResult)-1], ml2[0], "pages should overlap by one") + testModelListSort(t, ml2) +} + +func testListAnnoTagOnlyNotFound( + t *testing.T, + assert *require.Assertions, + anrepo repository.TaggedAnnotationRepository, +) { + t.Helper() + _, err := anrepo.ListAnnotations( + &repository.ListAnnotationsParams{ + Limit: 4, + Filter: `tag==nonexistent tag;ontology==dicty_annotation`, + }, + ) + assert.Error(err, "expect not-found error") + assert.True( + repository.IsAnnotationListNotFound(err), + "error should be annotationListNotFound", + ) +} diff --git a/internal/repository/arangodb/feature_annotation.go b/internal/repository/arangodb/feature_annotation.go index fe30784..8059c3b 100644 --- a/internal/repository/arangodb/feature_annotation.go +++ b/internal/repository/arangodb/feature_annotation.go @@ -81,8 +81,8 @@ func (fann *featureAnnoRepo) GetFeatureAnnotation( featureGetByIDQ, map[string]any{ collectionBind: fann.feature.Name(), - "graph": fann.featPub.Name(), - "id": fid, + "graph": fann.featPub.Name(), + "id": fid, }, ) if err != nil { @@ -107,8 +107,8 @@ func (fann *featureAnnoRepo) GetFeatureAnnotationByName( featAnnoGetByNameQ, map[string]any{ collectionBind: fann.feature.Name(), - "graph": fann.featPub.Name(), // Add graph name - "name": name, + "graph": fann.featPub.Name(), // Add graph name + "name": name, }, ) if err != nil { @@ -343,9 +343,9 @@ func (fann *featureAnnoRepo) ListByPublicationID( ) ([]*model.FeatureAnnotationDoc, error) { binds := map[string]any{ collectionBind: fann.pub.Name(), - "graph": fann.featPub.Name(), - "id": publicationID, - "source": source, + "graph": fann.featPub.Name(), + "id": publicationID, + "source": source, } resultSet, err := fann.database.SearchRows(featureByPublicationIDQ, binds) @@ -385,7 +385,7 @@ func (fann *featureAnnoRepo) RemoveFeatureAnnotation( ) error { bindVars := map[string]any{ collectionBind: fann.feature.Name(), - "id": fid, + "id": fid, } // Check if document exists @@ -488,8 +488,8 @@ func (fann *featureAnnoRepo) AddTags( } result, err := txr.DoRun(featurePropsAppendQ, map[string]any{ collectionBind: fann.feature.Name(), - "key": doc.Key, - "newprops": collection.Map(req.Tags, convertNewTagToModel), + "key": doc.Key, + "newprops": collection.Map(req.Tags, convertNewTagToModel), }) if err != nil { if abortErr := txr.Abort(); abortErr != nil { @@ -542,8 +542,8 @@ func (fann *featureAnnoRepo) SetTags( result, err := txr.DoRun(featurePropsSetQ, map[string]any{ collectionBind: fann.feature.Name(), - "key": doc.Key, - "newprops": collection.Map(req.Tags, convertNewTagToModel), + "key": doc.Key, + "newprops": collection.Map(req.Tags, convertNewTagToModel), }) if err != nil { if abortErr := txr.Abort(); abortErr != nil { @@ -615,7 +615,7 @@ func (fann *featureAnnoRepo) upsertPublicationsTx( result, err := txr.DoRun( pubUpsertQ, map[string]any{ - "ids": ids, + "ids": ids, collectionBind: fann.pub.Name(), }, ) diff --git a/internal/repository/arangodb/organism.go b/internal/repository/arangodb/organism.go index 6bb61ba..8ec7612 100644 --- a/internal/repository/arangodb/organism.go +++ b/internal/repository/arangodb/organism.go @@ -93,8 +93,8 @@ func (org *organismRepo) GetOrganismByName( res, err := org.database.GetRow(orgGetByNameQ, map[string]any{ collectionBind: org.organism.Name(), - "genus": genus, - "species": species, + "genus": genus, + "species": species, }) if err != nil { return nil, fmt.Errorf("error executing query: %w", err)