From f423cf903ca68d78a938afef00b480fb65d56da0 Mon Sep 17 00:00:00 2001 From: Michael Reimsbach Date: Tue, 22 Oct 2024 10:07:57 +0200 Subject: [PATCH] feat(issue): refactor issue aggregations (#296) * feat(issue): refactor issue aggregations * feat(issue): add e2e test for aggregation --- internal/api/graphql/graph/model/models.go | 2 +- .../issue/withMetadata.graphql | 19 ++- internal/database/mariadb/entity.go | 12 +- internal/database/mariadb/issue.go | 141 ++++++++++++------ internal/database/mariadb/issue_test.go | 36 ++++- internal/database/mariadb/service.go | 27 ---- internal/e2e/issue_query_test.go | 40 +++++ internal/entity/issue.go | 2 +- internal/entity/test/issue.go | 2 +- 9 files changed, 192 insertions(+), 89 deletions(-) diff --git a/internal/api/graphql/graph/model/models.go b/internal/api/graphql/graph/model/models.go index 912747bf..bdac542a 100644 --- a/internal/api/graphql/graph/model/models.go +++ b/internal/api/graphql/graph/model/models.go @@ -172,7 +172,7 @@ func NewIssueWithAggregations(issue *entity.IssueResult) Issue { if issue.IssueAggregations != nil { metadata = IssueMetadata{ ServiceCount: int(issue.IssueAggregations.AffectedServices), - ActivityCount: int(issue.IssueAggregations.Activites), + ActivityCount: int(issue.IssueAggregations.Activities), IssueMatchCount: int(issue.IssueAggregations.IssueMatches), ComponentInstanceCount: int(issue.IssueAggregations.AffectedComponentInstances), ComponentVersionCount: int(issue.IssueAggregations.ComponentVersions), diff --git a/internal/api/graphql/graph/queryCollection/issue/withMetadata.graphql b/internal/api/graphql/graph/queryCollection/issue/withMetadata.graphql index de78498f..669a1022 100644 --- a/internal/api/graphql/graph/queryCollection/issue/withMetadata.graphql +++ b/internal/api/graphql/graph/queryCollection/issue/withMetadata.graphql @@ -20,8 +20,23 @@ query ($filter: IssueFilter, $first: Int, $after: String) { earliestDiscoveryDate earliestTargetRemediationDate } + issueMatches { + totalCount + edges { + node { + componentInstance { + count + service { + id + } + } + } + } + } + activities { + totalCount + } } - cursor } } -} \ No newline at end of file +} diff --git a/internal/database/mariadb/entity.go b/internal/database/mariadb/entity.go index e839d67b..198f3896 100644 --- a/internal/database/mariadb/entity.go +++ b/internal/database/mariadb/entity.go @@ -112,7 +112,7 @@ type GetIssuesByRow struct { } type IssueAggregationsRow struct { - Activites sql.NullInt64 `db:"agg_activities"` + Activities sql.NullInt64 `db:"agg_activities"` IssueMatches sql.NullInt64 `db:"agg_issue_matches"` AffectedServices sql.NullInt64 `db:"agg_affected_services"` ComponentVersions sql.NullInt64 `db:"agg_component_versions"` @@ -124,11 +124,11 @@ type IssueAggregationsRow struct { func (ibr *GetIssuesByRow) AsIssueWithAggregations() entity.IssueWithAggregations { return entity.IssueWithAggregations{ IssueAggregations: entity.IssueAggregations{ - Activites: GetInt64Value(ibr.IssueAggregationsRow.Activites), - IssueMatches: GetInt64Value(ibr.IssueAggregationsRow.IssueMatches), - AffectedServices: GetInt64Value(ibr.IssueAggregationsRow.AffectedServices), - ComponentVersions: GetInt64Value(ibr.IssueAggregationsRow.ComponentVersions), - AffectedComponentInstances: GetInt64Value(ibr.IssueAggregationsRow.AffectedComponentInstances), + Activities: lo.Max([]int64{0, GetInt64Value(ibr.IssueAggregationsRow.Activities)}), + IssueMatches: lo.Max([]int64{0, GetInt64Value(ibr.IssueAggregationsRow.IssueMatches)}), + AffectedServices: lo.Max([]int64{0, GetInt64Value(ibr.IssueAggregationsRow.AffectedServices)}), + ComponentVersions: lo.Max([]int64{0, GetInt64Value(ibr.IssueAggregationsRow.ComponentVersions)}), + AffectedComponentInstances: lo.Max([]int64{0, GetInt64Value(ibr.IssueAggregationsRow.AffectedComponentInstances)}), EarliestTargetRemediationDate: GetTimeValue(ibr.IssueAggregationsRow.EarliestTargetRemediationDate), EarliestDiscoveryDate: GetTimeValue(ibr.IssueAggregationsRow.EarliestDiscoveryDate), }, diff --git a/internal/database/mariadb/issue.go b/internal/database/mariadb/issue.go index 19c0c0ea..dd7674ca 100644 --- a/internal/database/mariadb/issue.go +++ b/internal/database/mariadb/issue.go @@ -19,6 +19,26 @@ const ( wildCardFilterParamCount = 2 ) +func (s *SqlDatabase) buildIssueFilterParameters(filter *entity.IssueFilter, withCursor bool, cursor entity.Cursor) []interface{} { + var filterParameters []interface{} + filterParameters = buildQueryParameters(filterParameters, filter.ServiceName) + filterParameters = buildQueryParameters(filterParameters, filter.Id) + filterParameters = buildQueryParameters(filterParameters, filter.IssueMatchStatus) + filterParameters = buildQueryParameters(filterParameters, filter.ActivityId) + filterParameters = buildQueryParameters(filterParameters, filter.IssueMatchId) + filterParameters = buildQueryParameters(filterParameters, filter.ComponentVersionId) + filterParameters = buildQueryParameters(filterParameters, filter.IssueVariantId) + filterParameters = buildQueryParameters(filterParameters, filter.Type) + filterParameters = buildQueryParameters(filterParameters, filter.PrimaryName) + filterParameters = buildQueryParametersCount(filterParameters, filter.Search, wildCardFilterParamCount) + if withCursor { + filterParameters = append(filterParameters, cursor.Value) + filterParameters = append(filterParameters, cursor.Limit) + } + + return filterParameters +} + func (s *SqlDatabase) getIssueFilterString(filter *entity.IssueFilter) string { var fl []string fl = append(fl, buildFilterQuery(filter.ServiceName, "S.service_name = ?", OP_OR)) @@ -36,20 +56,20 @@ func (s *SqlDatabase) getIssueFilterString(filter *entity.IssueFilter) string { return combineFilterQueries(fl, OP_AND) } -func (s *SqlDatabase) getIssueJoins(filter *entity.IssueFilter, withAggregations bool) string { +func (s *SqlDatabase) getIssueJoins(filter *entity.IssueFilter) string { joins := "" - if len(filter.ActivityId) > 0 || withAggregations { + if len(filter.ActivityId) > 0 { joins = fmt.Sprintf("%s\n%s", joins, ` LEFT JOIN ActivityHasIssue AHI on I.issue_id = AHI.activityhasissue_issue_id LEFT JOIN Activity A on AHI.activityhasissue_activity_id = A.activity_id `) } - if len(filter.IssueMatchStatus) > 0 || len(filter.ServiceName) > 0 || len(filter.IssueMatchId) > 0 || withAggregations { + if len(filter.IssueMatchStatus) > 0 || len(filter.ServiceName) > 0 || len(filter.IssueMatchId) > 0 { joins = fmt.Sprintf("%s\n%s", joins, ` LEFT JOIN IssueMatch IM ON I.issue_id = IM.issuematch_issue_id `) } - if len(filter.ServiceName) > 0 || withAggregations { + if len(filter.ServiceName) > 0 { joins = fmt.Sprintf("%s\n%s", joins, ` LEFT JOIN ComponentInstance CI ON CI.componentinstance_id = IM.issuematch_component_instance_id LEFT JOIN ComponentVersion CV ON CI.componentinstance_component_version_id = CV.componentversion_id @@ -57,7 +77,7 @@ func (s *SqlDatabase) getIssueJoins(filter *entity.IssueFilter, withAggregations `) } - if len(filter.ComponentVersionId) > 0 || withAggregations { + if len(filter.ComponentVersionId) > 0 { joins = fmt.Sprintf("%s\n%s", joins, ` LEFT JOIN ComponentVersionIssue CVI ON I.issue_id = CVI.componentversionissue_issue_id `) @@ -117,14 +137,13 @@ func (s *SqlDatabase) getIssueUpdateFields(issue *entity.Issue) string { return strings.Join(fl, ", ") } -func (s *SqlDatabase) buildIssueStatement(baseQuery string, filter *entity.IssueFilter, aggregations []string, withCursor bool, l *logrus.Entry) (*sqlx.Stmt, []interface{}, error) { +func (s *SqlDatabase) buildIssueStatement(baseQuery string, filter *entity.IssueFilter, withCursor bool, l *logrus.Entry) (*sqlx.Stmt, []interface{}, error) { var query string filter = s.ensureIssueFilter(filter) l.WithFields(logrus.Fields{"filter": filter}) filterStr := s.getIssueFilterString(filter) - withAggreations := len(aggregations) > 0 - joins := s.getIssueJoins(filter, withAggreations) + joins := s.getIssueJoins(filter) cursor := getCursor(filter.Paginated, filterStr, "I.issue_id > ?") whereClause := "" @@ -132,12 +151,6 @@ func (s *SqlDatabase) buildIssueStatement(baseQuery string, filter *entity.Issue whereClause = fmt.Sprintf("WHERE %s", filterStr) } - ags := "" - if len(aggregations) > 0 { - ags = fmt.Sprintf(", %s", strings.Join(aggregations, ", ")) - baseQuery = fmt.Sprintf(baseQuery, ags, "%s", "%s", "%s") - } - // construct final query if withCursor { query = fmt.Sprintf(baseQuery, joins, whereClause, cursor.Statement) @@ -162,21 +175,7 @@ func (s *SqlDatabase) buildIssueStatement(baseQuery string, filter *entity.Issue } //adding parameters - var filterParameters []interface{} - filterParameters = buildQueryParameters(filterParameters, filter.ServiceName) - filterParameters = buildQueryParameters(filterParameters, filter.Id) - filterParameters = buildQueryParameters(filterParameters, filter.IssueMatchStatus) - filterParameters = buildQueryParameters(filterParameters, filter.ActivityId) - filterParameters = buildQueryParameters(filterParameters, filter.IssueMatchId) - filterParameters = buildQueryParameters(filterParameters, filter.ComponentVersionId) - filterParameters = buildQueryParameters(filterParameters, filter.IssueVariantId) - filterParameters = buildQueryParameters(filterParameters, filter.Type) - filterParameters = buildQueryParameters(filterParameters, filter.PrimaryName) - filterParameters = buildQueryParametersCount(filterParameters, filter.Search, wildCardFilterParamCount) - if withCursor { - filterParameters = append(filterParameters, cursor.Value) - filterParameters = append(filterParameters, cursor.Limit) - } + filterParameters := s.buildIssueFilterParameters(filter, withCursor, cursor) return stmt, filterParameters, nil } @@ -188,34 +187,78 @@ func (s *SqlDatabase) GetIssuesWithAggregations(filter *entity.IssueFilter) ([]e "event": "database.GetIssuesWithAggregations", }) - baseQuery := ` - SELECT I.* %s FROM Issue I + baseCiQuery := ` + SELECT I.*, SUM(CI.componentinstance_count) AS agg_affected_component_instances FROM Issue I + LEFT JOIN IssueMatch IM on I.issue_id = IM.issuematch_issue_id + LEFT JOIN ComponentInstance CI on IM.issuematch_component_instance_id = CI.componentinstance_id + %s + %s + %s GROUP BY I.issue_id ORDER BY I.issue_id LIMIT ? + ` + + baseAggQuery := ` + SELECT I.*, + count(distinct issuematch_id) as agg_issue_matches, + count(distinct activity_id) as agg_activities, + count(distinct service_name) as agg_affected_services, + count(distinct componentversionissue_component_version_id) as agg_component_versions, + min(issuematch_target_remediation_date) as agg_earliest_target_remediation_date, + min(issuematch_created_at) agg_earliest_discovery_date + FROM Issue I + LEFT JOIN ActivityHasIssue AHI on I.issue_id = AHI.activityhasissue_issue_id + LEFT JOIN Activity A on AHI.activityhasissue_activity_id = A.activity_id + LEFT JOIN IssueMatch IM on I.issue_id = IM.issuematch_issue_id + LEFT JOIN ComponentInstance CI ON CI.componentinstance_id = IM.issuematch_component_instance_id + LEFT JOIN ComponentVersion CV ON CI.componentinstance_component_version_id = CV.componentversion_id + LEFT JOIN Service S ON S.service_id = CI.componentinstance_service_id + LEFT JOIN ComponentVersionIssue CVI ON I.issue_id = CVI.componentversionissue_issue_id %s %s %s GROUP BY I.issue_id ORDER BY I.issue_id LIMIT ? - ` + ` - aggregations := []string{ - "count(distinct issuematch_id) as agg_issue_matches", - "count(distinct activity_id) as agg_activities", - "count(distinct service_name) as agg_affected_services", - "count(distinct componentversionissue_component_version_id) as agg_component_versions", - "sum(componentinstance_count) as agg_affected_component_instances", - "min(issuematch_target_remediation_date) as agg_earliest_target_remediation_date", - "min(issuematch_created_at) agg_earliest_discovery_date", - } + baseQuery := ` + With ComponentInstanceCounts AS ( + %s + ), + Aggs AS ( + %s + ) + SELECT A.*, CIC.* + FROM ComponentInstanceCounts CIC + JOIN Aggs A ON CIC.issue_id = A.issue_id; + ` - stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, aggregations, true, l) + filter = s.ensureIssueFilter(filter) + filterStr := s.getIssueFilterString(filter) + joins := s.getIssueJoins(filter) + cursor := getCursor(filter.Paginated, filterStr, "I.issue_id > ?") + whereClause := fmt.Sprintf("WHERE %s", filterStr) + + ciQuery := fmt.Sprintf(baseCiQuery, joins, whereClause, cursor.Statement) + aggQuery := fmt.Sprintf(baseAggQuery, joins, whereClause, cursor.Statement) + query := fmt.Sprintf(baseQuery, ciQuery, aggQuery) + + var stmt *sqlx.Stmt + var err error + stmt, err = s.db.Preparex(query) if err != nil { msg := ERROR_MSG_PREPARED_STMT l.WithFields( logrus.Fields{ - "error": err, - "aggregations": aggregations, + "error": err, + "query": query, + "stmt": stmt, }).Error(msg) return nil, fmt.Errorf("%s", msg) } + + // parameters for component instance query + filterParameters := s.buildIssueFilterParameters(filter, true, cursor) + // parameters for agg query + filterParameters = append(filterParameters, s.buildIssueFilterParameters(filter, true, cursor)...) + defer stmt.Close() return performListScan( @@ -238,7 +281,7 @@ func (s *SqlDatabase) CountIssues(filter *entity.IssueFilter) (int64, error) { %s %s ` - stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, []string{}, false, l) + stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, false, l) if err != nil { return -1, err @@ -261,7 +304,7 @@ func (s *SqlDatabase) CountIssueTypes(filter *entity.IssueFilter) (*entity.Issue GROUP BY I.issue_type ` - stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, []string{}, false, l) + stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, false, l) if err != nil { return nil, err @@ -308,7 +351,7 @@ func (s *SqlDatabase) GetAllIssueIds(filter *entity.IssueFilter) ([]int64, error %s GROUP BY I.issue_id ORDER BY I.issue_id ` - stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, []string{}, false, l) + stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, false, l) if err != nil { return nil, err @@ -333,7 +376,7 @@ func (s *SqlDatabase) GetIssues(filter *entity.IssueFilter) ([]entity.Issue, err filter = s.ensureIssueFilter(filter) - stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, []string{}, true, l) + stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, true, l) if err != nil { return nil, err @@ -502,7 +545,7 @@ func (s *SqlDatabase) GetIssueNames(filter *entity.IssueFilter) ([]string, error filter = s.ensureIssueFilter(filter) // Builds full statement with possible joins and filters - stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, []string{}, false, l) + stmt, filterParameters, err := s.buildIssueStatement(baseQuery, filter, false, l) if err != nil { l.Error("Error preparing statement: ", err) return nil, err diff --git a/internal/database/mariadb/issue_test.go b/internal/database/mariadb/issue_test.go index b1f6fe83..b56db30b 100644 --- a/internal/database/mariadb/issue_test.go +++ b/internal/database/mariadb/issue_test.go @@ -416,10 +416,39 @@ var _ = Describe("Issue", Label("database", "Issue"), func() { }) }) When("Getting Issues with Aggregations", Label("GetIssuesWithAggregations"), func() { - BeforeEach(func() { - _ = seeder.SeedDbWithNFakeData(10) + Context("and the database contains service without aggregations", func() { + BeforeEach(func() { + newIssueRow := test.NewFakeIssue() + newIssue := newIssueRow.AsIssue() + db.CreateIssue(&newIssue) + }) + It("returns the issues with aggregations", func() { + entriesWithAggregations, err := db.GetIssuesWithAggregations(nil) + + By("throwing no error", func() { + Expect(err).To(BeNil()) + }) + + By("returning some aggregations", func() { + for _, entryWithAggregations := range entriesWithAggregations { + Expect(entryWithAggregations).NotTo( + BeEquivalentTo(entity.IssueAggregations{})) + Expect(entryWithAggregations.IssueAggregations.Activities).To(BeEquivalentTo(0)) + Expect(entryWithAggregations.IssueAggregations.IssueMatches).To(BeEquivalentTo(0)) + Expect(entryWithAggregations.IssueAggregations.AffectedServices).To(BeEquivalentTo(0)) + Expect(entryWithAggregations.IssueAggregations.AffectedComponentInstances).To(BeEquivalentTo(0)) + Expect(entryWithAggregations.IssueAggregations.ComponentVersions).To(BeEquivalentTo(0)) + } + }) + By("returning all issues", func() { + Expect(len(entriesWithAggregations)).To(BeEquivalentTo(1)) + }) + }) }) Context("and and we have 10 elements in the database", func() { + BeforeEach(func() { + _ = seeder.SeedDbWithNFakeData(10) + }) It("returns the issues with aggregations", func() { entriesWithAggregations, err := db.GetIssuesWithAggregations(nil) @@ -433,6 +462,9 @@ var _ = Describe("Issue", Label("database", "Issue"), func() { BeEquivalentTo(entity.IssueAggregations{})) } }) + By("returning all ld constraints exclude all Go files inservices", func() { + Expect(len(entriesWithAggregations)).To(BeEquivalentTo(10)) + }) }) It("returns correct aggregation values", func() { //Should be filled with a check for each aggregation value, diff --git a/internal/database/mariadb/service.go b/internal/database/mariadb/service.go index 52b75349..b00eeb34 100644 --- a/internal/database/mariadb/service.go +++ b/internal/database/mariadb/service.go @@ -180,33 +180,6 @@ func (s *SqlDatabase) buildServiceStatement(baseQuery string, filter *entity.Ser return stmt, filterParameters, nil } -func (s *SqlDatabase) getServicesWithAggregations(query string, filter *entity.ServiceFilter) ([]entity.ServiceWithAggregations, error) { - l := logrus.WithFields(logrus.Fields{ - "filter": filter, - "event": "database.getServicesWithAggregation", - }) - stmt, filterParameters, err := s.buildServiceStatement(query, filter, true, l) - - if err != nil { - msg := ERROR_MSG_PREPARED_STMT - l.WithFields( - logrus.Fields{ - "error": err, - }).Error(msg) - return nil, fmt.Errorf("%s", msg) - } - defer stmt.Close() - - return performListScan( - stmt, - filterParameters, - l, - func(l []entity.ServiceWithAggregations, e GetServicesByRow) []entity.ServiceWithAggregations { - return append(l, e.AsServiceWithAggregations()) - }, - ) -} - func (s *SqlDatabase) CountServices(filter *entity.ServiceFilter) (int64, error) { l := logrus.WithFields(logrus.Fields{ "event": "database.CountServices", diff --git a/internal/e2e/issue_query_test.go b/internal/e2e/issue_query_test.go index 5198876d..f4846d7b 100644 --- a/internal/e2e/issue_query_test.go +++ b/internal/e2e/issue_query_test.go @@ -199,6 +199,46 @@ var _ = Describe("Getting Issues via API", Label("e2e", "Issues"), func() { Expect(*respData.Issues.PageInfo.PageNumber).To(Equal(1), "Correct page number") }) }) + Context("and we request metadata", Label("withMetadata.graphql"), func() { + It("returns correct metadata counts", func() { + // create a queryCollection (safe to share across requests) + client := graphql.NewClient(fmt.Sprintf("http://localhost:%s/query", cfg.Port)) + + //@todo may need to make this more fault proof?! What if the test is executed from the root dir? does it still work? + b, err := os.ReadFile("../api/graphql/graph/queryCollection/issue/withMetadata.graphql") + + Expect(err).To(BeNil()) + str := string(b) + req := graphql.NewRequest(str) + + req.Var("filter", map[string]string{}) + req.Var("first", 5) + req.Var("after", "0") + + req.Header.Set("Cache-Control", "no-cache") + ctx := context.Background() + + var respData struct { + Issues model.IssueConnection `json:"Issues"` + } + if err := util2.RequestWithBackoff(func() error { return client.Run(ctx, req, &respData) }); err != nil { + logrus.WithError(err).WithField("request", req).Fatalln("Error while unmarshaling") + } + + for _, issueEdge := range respData.Issues.Edges { + ciCount := 0 + serviceIdSet := map[string]bool{} + for _, imEdge := range issueEdge.Node.IssueMatches.Edges { + ciCount += *imEdge.Node.ComponentInstance.Count + serviceIdSet[imEdge.Node.ComponentInstance.Service.ID] = true + } + Expect(issueEdge.Node.Metadata.IssueMatchCount).To(Equal(issueEdge.Node.IssueMatches.TotalCount), "IssueMatchCount is correct") + Expect(issueEdge.Node.Metadata.ComponentInstanceCount).To(Equal(ciCount), "ComponentInstanceCount is correct") + Expect(issueEdge.Node.Metadata.ActivityCount).To(Equal(issueEdge.Node.Activities.TotalCount), "ActivityCount is correct") + Expect(issueEdge.Node.Metadata.ServiceCount).To(Equal(len(serviceIdSet)), "ServiceCount is correct") + } + }) + }) }) }) }) diff --git a/internal/entity/issue.go b/internal/entity/issue.go index 6b114872..84478240 100644 --- a/internal/entity/issue.go +++ b/internal/entity/issue.go @@ -66,7 +66,7 @@ type IssueFilter struct { } type IssueAggregations struct { - Activites int64 + Activities int64 IssueMatches int64 AffectedServices int64 AffectedComponentInstances int64 diff --git a/internal/entity/test/issue.go b/internal/entity/test/issue.go index 705fd9ff..d233e39a 100644 --- a/internal/entity/test/issue.go +++ b/internal/entity/test/issue.go @@ -31,7 +31,7 @@ func NewFakeIssueEntity() entity.Issue { func NewFakeIssueWithAggregationsEntity() entity.IssueWithAggregations { return entity.IssueWithAggregations{ IssueAggregations: entity.IssueAggregations{ - Activites: int64(gofakeit.Number(1, 10000000)), + Activities: int64(gofakeit.Number(1, 10000000)), IssueMatches: int64(gofakeit.Number(1, 10000000)), AffectedServices: int64(gofakeit.Number(1, 10000000)), ComponentVersions: int64(gofakeit.Number(1, 10000000)),