From c90c781a07e33ef7ee3fc6b37994f32f1c230f1a Mon Sep 17 00:00:00 2001 From: Fabio Bonelli Date: Fri, 4 Sep 2026 06:14:27 +0200 Subject: [PATCH] fix: apply the from and to parameters where the spec declares them --- CHANGELOG.md | 2 + bundles_test.go | 11 +++++ catalogs_test.go | 25 ++++++++--- internal/handlers/catalogs.go | 5 +-- internal/handlers/crud.go | 28 ++++++------- internal/handlers/crud_test.go | 27 +++++++++++- internal/handlers/events.go | 2 +- internal/handlers/general/query.go | 41 +++++++++++++------ .../handlers/general/query_search_test.go | 2 +- internal/handlers/logs.go | 7 ++-- internal/handlers/publishers.go | 1 + internal/handlers/software.go | 11 ++--- internal/handlers/webhooks.go | 4 +- publishers_test.go | 12 ++++++ software_test.go | 24 +++++++++++ webhooks_test.go | 26 ++++++++++++ 16 files changed, 176 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47269f9..96325c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 delete event and notified the webhooks. - The event recorded by an analysis `PATCH` had no actor. - `?from` and `?to` refused RFC 3339 values with a zone offset. +- `?from` and `?to` were ignored on the per entity log lists and on the + webhook lists, although the OpenAPI spec declares them there. - Catalogs, bundles and webhooks wrote no event to the audit trail. - A webhook `POST` with the url of an existing one answered `500` instead of `409`. diff --git a/bundles_test.go b/bundles_test.go index 503779b..55f23e4 100644 --- a/bundles_test.go +++ b/bundles_test.go @@ -38,6 +38,17 @@ func TestBundleEndpoints(t *testing.T) { assertOnlyKeys(t, first, "id", "name", "description", "active", "softwareIds", "createdAt", "updatedAt") }, }, + { + description: "GET bundles ignores a from param the operation does not declare", + query: "GET /v1/bundles?from=not-a-date", + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]interface{}) { + data := assertListResponse(t, response) + + assert.Equal(t, 2, len(data)) + }, + }, { description: "GET bundles with all=true returns inactive too", query: "GET /v1/bundles?all=true", diff --git a/catalogs_test.go b/catalogs_test.go index 8fac2bd..a7460c3 100644 --- a/catalogs_test.go +++ b/catalogs_test.go @@ -74,6 +74,18 @@ func TestCatalogEndpoints(t *testing.T) { }, }, + { + description: "GET catalogs ignores a from param the operation does not declare", + query: "GET /v1/catalogs?from=not-a-date", + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]any) { + data := assertListResponse(t, response) + + assert.Equal(t, 2, len(data)) + }, + }, + // GET /catalogs/:id { description: "GET catalog by id", @@ -705,13 +717,14 @@ func TestCatalogEndpoints(t *testing.T) { }, }, { - description: "GET catalog software with unknown url and an invalid from param still validates from", - query: "GET /v1/catalogs/" + italiaID + "/software?url=https://example.org/unknown&from=not-a-date", - expectedCode: 422, - expectedContentType: "application/problem+json", + description: "GET catalog software ignores a from param the operation does not declare", + query: "GET /v1/catalogs/" + italiaID + "/software?from=not-a-date", + expectedCode: 200, + expectedContentType: "application/json", validateFunc: func(t *testing.T, response map[string]any) { - assert.Equal(t, `can't get Software`, response["title"]) - assert.Equal(t, "invalid date time format (RFC 3339 needed)", response["detail"]) + data := assertListResponse(t, response) + + assert.Equal(t, 1, len(data)) }, }, { diff --git a/internal/handlers/catalogs.go b/internal/handlers/catalogs.go index ac92378..8f1b7de 100644 --- a/internal/handlers/catalogs.go +++ b/internal/handlers/catalogs.go @@ -378,10 +378,7 @@ func (c *Catalog) GetCatalogSoftware(ctx *fiber.Ctx) error { return err } - stmt, err := general.Clauses(ctx, c.db.Preload("Aliases").Scopes(catalogScope(catalog)), "", "") - if err != nil { - return common.Error(fiber.StatusUnprocessableEntity, "can't get Software", general.QueryErrorDetail(err)) - } + stmt := c.db.Preload("Aliases").Scopes(catalogScope(catalog)) stmt, found, err := softwareURLFilter(ctx, c.db, stmt, "can't get Software") if err != nil { diff --git a/internal/handlers/crud.go b/internal/handlers/crud.go index 428a393..6b23ab5 100644 --- a/internal/handlers/crud.go +++ b/internal/handlers/crud.go @@ -28,11 +28,11 @@ type listOptions struct { order paginator.Order // activeOnly applies models.Active unless ?all=true is given. activeOnly bool - // skipClauses leaves ?filter, ?from, ?to and ?search untouched, - // either because the list never supported them (webhooks, per - // resource logs) or because the handler applied them itself before - // a short circuit (the software lists). - skipClauses bool + // dateWindow narrows the list to the ?from and ?to window on + // created_at. Only the lists whose OpenAPI operation declares the two + // parameters read them, elsewhere they are ignored rather than + // filtering a page or answering 422 on a value nobody documented. + dateWindow bool } // paginate applies opts to stmt and returns one page of T plus the cursor @@ -41,17 +41,13 @@ type listOptions struct { func paginate[T any](ctx *fiber.Ctx, stmt *gorm.DB, opts listOptions) ([]T, paginator.Cursor, error) { var items []T - if !opts.skipClauses { - var err error - - stmt, err = general.Clauses(ctx, stmt, opts.filterField, opts.searchField) - if err != nil { - return nil, paginator.Cursor{}, common.Error( - fiber.StatusUnprocessableEntity, - opts.title, - general.QueryErrorDetail(err), - ) - } + stmt, err := general.Clauses(ctx, stmt, opts.filterField, opts.searchField, opts.dateWindow) + if err != nil { + return nil, paginator.Cursor{}, common.Error( + fiber.StatusUnprocessableEntity, + opts.title, + general.QueryErrorDetail(err), + ) } if opts.activeOnly && !ctx.QueryBool("all", false) { diff --git a/internal/handlers/crud_test.go b/internal/handlers/crud_test.go index 9f4a08b..16af595 100644 --- a/internal/handlers/crud_test.go +++ b/internal/handlers/crud_test.go @@ -110,7 +110,7 @@ func TestPaginateFilterField(t *testing.T) { require.Len(t, items, 1) assert.Equal(t, "two", items[0].ID) - items, _, err = paginate[crudItem](newCrudCtx(t, "filter=second"), db, listOptions{title: "t", skipClauses: true}) + items, _, err = paginate[crudItem](newCrudCtx(t, "filter=second"), db, listOptions{title: "t"}) require.NoError(t, err) assert.Len(t, items, 3) } @@ -131,11 +131,34 @@ func TestPaginateSearchField(t *testing.T) { assert.Len(t, items, 3) } +func TestPaginateDateWindow(t *testing.T) { + db := newCrudDB(t) + seedCrudItems(t, db) + + items, _, err := paginate[crudItem]( + newCrudCtx(t, "from=2024-01-01T00:30:00Z"), db, listOptions{title: "t", dateWindow: true}, + ) + require.NoError(t, err) + assert.Len(t, items, 2) + + // Without the window the two parameters are ignored, a bad value + // included: the list does not document them. + items, _, err = paginate[crudItem](newCrudCtx(t, "from=2024-01-01T00:30:00Z"), db, listOptions{title: "t"}) + require.NoError(t, err) + assert.Len(t, items, 3) + + items, _, err = paginate[crudItem](newCrudCtx(t, "from=not-a-date"), db, listOptions{title: "t"}) + require.NoError(t, err) + assert.Len(t, items, 3) +} + func TestPaginateErrors(t *testing.T) { db := newCrudDB(t) seedCrudItems(t, db) - _, _, err := paginate[crudItem](newCrudCtx(t, "from=not-a-date"), db, listOptions{title: "can't get Items"}) + _, _, err := paginate[crudItem]( + newCrudCtx(t, "from=not-a-date"), db, listOptions{title: "can't get Items", dateWindow: true}, + ) assert.Equal(t, fiber.StatusUnprocessableEntity, problemStatus(t, err)) _, _, err = paginate[crudItem](newCrudCtx(t, "page[after]=garbage"), db, listOptions{title: "can't get Items"}) diff --git a/internal/handlers/events.go b/internal/handlers/events.go index 3392244..1e95367 100644 --- a/internal/handlers/events.go +++ b/internal/handlers/events.go @@ -46,7 +46,7 @@ func (e *Event) GetEvents(ctx *fiber.Ctx) error { stmt = stmt.Where(map[string]any{"type": eventType}) } - return list[models.Event](ctx, stmt, listOptions{title: errMsg, order: paginator.DESC}) + return list[models.Event](ctx, stmt, listOptions{title: errMsg, order: paginator.DESC, dateWindow: true}) } // GetEvent gets the event with the given ID and returns any error encountered. diff --git a/internal/handlers/general/query.go b/internal/handlers/general/query.go index aed1169..9b584fa 100644 --- a/internal/handlers/general/query.go +++ b/internal/handlers/general/query.go @@ -36,9 +36,10 @@ func QueryErrorDetail(err error) string { // Clauses applies the ?filter, ?from, ?to and ?search query parameters // to stmt. filterField is the column ?filter matches exactly and // searchField the column ?search matches as a case insensitive -// substring. An empty column name leaves its parameter unread: the -// lists without such a column must not pass it on to the database. -func Clauses(ctx *fiber.Ctx, stmt *gorm.DB, filterField, searchField string) (*gorm.DB, error) { +// substring, dateWindow turns ?from and ?to on. An empty column name or +// a false dateWindow leaves the parameter unread: a list must not act +// on a parameter its OpenAPI operation does not declare. +func Clauses(ctx *fiber.Ctx, stmt *gorm.DB, filterField, searchField string, dateWindow bool) (*gorm.DB, error) { ret := stmt if filterField != "" { @@ -49,6 +50,31 @@ func Clauses(ctx *fiber.Ctx, stmt *gorm.DB, filterField, searchField string) (*g } } + if dateWindow { + var err error + + ret, err = createdAtWindow(ctx, ret) + if err != nil { + return nil, err + } + } + + if searchField != "" { + if search := ctx.Query("search", ""); search != "" { + ret = ret.Where(clause.Expr{ + SQL: "LOWER(?) LIKE ?", + Vars: []any{clause.Column{Name: searchField}, "%" + utils.ToLower(search) + "%"}, + }) + } + } + + return ret, nil +} + +// createdAtWindow narrows stmt to the ?from and ?to window. +func createdAtWindow(ctx *fiber.Ctx, stmt *gorm.DB) (*gorm.DB, error) { + ret := stmt + if from := ctx.Query("from", ""); from != "" { at, err := time.Parse(time.RFC3339, from) if err != nil { @@ -67,14 +93,5 @@ func Clauses(ctx *fiber.Ctx, stmt *gorm.DB, filterField, searchField string) (*g ret = ret.Where("created_at < ?", at) } - if searchField != "" { - if search := ctx.Query("search", ""); search != "" { - ret = ret.Where(clause.Expr{ - SQL: "LOWER(?) LIKE ?", - Vars: []any{clause.Column{Name: searchField}, "%" + utils.ToLower(search) + "%"}, - }) - } - } - return ret, nil } diff --git a/internal/handlers/general/query_search_test.go b/internal/handlers/general/query_search_test.go index cfe8c09..01ced81 100644 --- a/internal/handlers/general/query_search_test.go +++ b/internal/handlers/general/query_search_test.go @@ -20,7 +20,7 @@ func TestClausesQuotesSearchColumn(t *testing.T) { stmt := db.Session(&gorm.Session{DryRun: true}).Table("logs") - ret, err := Clauses(ctx, stmt, "", "name") + ret, err := Clauses(ctx, stmt, "", "name", false) require.NoError(t, err) sql := ret.Find(&[]struct{}{}).Statement.SQL.String() diff --git a/internal/handlers/logs.go b/internal/handlers/logs.go index 80a5041..1f9839e 100644 --- a/internal/handlers/logs.go +++ b/internal/handlers/logs.go @@ -26,6 +26,7 @@ func (p *Log) GetLogs(ctx *fiber.Ctx) error { filterField: "message", searchField: "message", order: paginator.DESC, + dateWindow: true, }) } @@ -179,9 +180,9 @@ func (p *Log) getEntityLogs(ctx *fiber.Ctx, entity models.Model, entityName stri Where("entity_id = ?", entity.UUID()) return list[models.Log](ctx, stmt, listOptions{ - title: "can't get Logs", - order: paginator.DESC, - skipClauses: true, + title: "can't get Logs", + order: paginator.DESC, + dateWindow: true, }) } diff --git a/internal/handlers/publishers.go b/internal/handlers/publishers.go index 1684b96..5fb91ad 100644 --- a/internal/handlers/publishers.go +++ b/internal/handlers/publishers.go @@ -27,6 +27,7 @@ func (p *Publisher) GetPublishers(ctx *fiber.Ctx) error { return list[models.Publisher](ctx, p.db.Preload(codeHostingAssociation), listOptions{ title: "can't get Publishers", activeOnly: true, + dateWindow: true, }) } diff --git a/internal/handlers/software.go b/internal/handlers/software.go index 6185cf9..6d6382d 100644 --- a/internal/handlers/software.go +++ b/internal/handlers/software.go @@ -30,7 +30,7 @@ func NewSoftware(db *gorm.DB) *Software { // GetAllSoftware gets the list of all software and returns any error encountered. func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error { - stmt, err := general.Clauses(ctx, p.db.Preload("Aliases"), "", "") + stmt, err := general.Clauses(ctx, p.db.Preload("Aliases"), "", "", true) if err != nil { return common.Error(fiber.StatusUnprocessableEntity, "can't get Software", general.QueryErrorDetail(err)) } @@ -375,12 +375,13 @@ func softwareURLFilter(ctx *fiber.Ctx, gormdb, stmt *gorm.DB, title string) (*go // listSoftware writes one page of the software matched by stmt. stmt must // Preload("Aliases") and nothing else: Preload("URL") makes gorm guess a -// has one through the software_id column and load an arbitrary url. +// has one through the software_id column and load an arbitrary url. The +// ?from and ?to window is the caller's, GET /software applies it before +// its ?url short circuit and the catalog software list does not declare it. func listSoftware(ctx *fiber.Ctx, stmt *gorm.DB) error { software, cursor, err := paginate[models.Software](ctx, stmt, listOptions{ - title: "can't get Software", - activeOnly: true, - skipClauses: true, + title: "can't get Software", + activeOnly: true, }) if err != nil { return err diff --git a/internal/handlers/webhooks.go b/internal/handlers/webhooks.go index fb85465..049e380 100644 --- a/internal/handlers/webhooks.go +++ b/internal/handlers/webhooks.go @@ -44,7 +44,7 @@ func (p *Webhook[T]) GetResourceWebhooks(ctx *fiber.Ctx) error { stmt := p.db.Where(map[string]any{"entity_type": resource.TableName()}) - return list[models.Webhook](ctx, stmt, listOptions{title: "can't get Webhooks", skipClauses: true}) + return list[models.Webhook](ctx, stmt, listOptions{title: "can't get Webhooks", dateWindow: true}) } // GetSingleResourceWebhooks gets the webhooks associated to a resource @@ -65,7 +65,7 @@ func (p *Webhook[T]) GetSingleResourceWebhooks(ctx *fiber.Ctx) error { Where(map[string]any{"entity_type": resource.TableName()}). Where("entity_id = ?", resource.UUID()) - return list[models.Webhook](ctx, stmt, listOptions{title: "can't get Webhooks", skipClauses: true}) + return list[models.Webhook](ctx, stmt, listOptions{title: "can't get Webhooks", dateWindow: true}) } // PostResourceWebhook creates a new webhook associated to resources diff --git a/publishers_test.go b/publishers_test.go index 7f0dbb2..18df97e 100644 --- a/publishers_test.go +++ b/publishers_test.go @@ -1284,6 +1284,18 @@ func TestPublishersLogsEndpoints(t *testing.T) { assertOnlyKeys(t, log, "id", "createdAt", "updatedAt", "message", "entity") }, }, + { + description: `GET logs with a "from" query param later than every log`, + query: "GET /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde/logs?from=2020-01-01T00:00:00Z", + + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]interface{}) { + data := assertListResponse(t, response) + + assert.Empty(t, data) + }, + }, { description: "GET logs for non existing publisher", query: "GET /v1/publishers/NO_SUCH_PUBLISHER/logs", diff --git a/software_test.go b/software_test.go index 135a8bb..167fc5d 100644 --- a/software_test.go +++ b/software_test.go @@ -1175,6 +1175,30 @@ func TestSoftwareEndpoints(t *testing.T) { } }, }, + { + description: `GET logs with a "from" query param later than every log`, + query: "GET /v1/software/c353756e-8597-4e46-a99b-7da2e141603b/logs?from=2020-01-01T00:00:00Z", + + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]any) { + data := assertListResponse(t, response) + + assert.Empty(t, data) + }, + }, + { + description: `GET logs with a "to" query param earlier than every log`, + query: "GET /v1/software/c353756e-8597-4e46-a99b-7da2e141603b/logs?to=2000-01-01T00:00:00Z", + + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]any) { + data := assertListResponse(t, response) + + assert.Empty(t, data) + }, + }, { description: "GET logs for non existing software", query: "GET /v1/software/NO_SUCH_SOFTWARE/logs", diff --git a/webhooks_test.go b/webhooks_test.go index 14eabe1..fa0cc5c 100644 --- a/webhooks_test.go +++ b/webhooks_test.go @@ -101,6 +101,19 @@ func TestWebhooksEndpoints(t *testing.T) { }, }, + { + description: `GET webhooks with a "from" query param later than every webhook`, + query: "GET /v1/software/webhooks?from=2020-01-01T00:00:00Z", + headers: map[string][]string{"Authorization": {goodToken}}, + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]interface{}) { + data := assertListResponse(t, response) + + assert.Empty(t, data) + }, + }, + // GET /software/:id/webhooks { description: "GET webhooks of a software with invalid format for page[size] query param", @@ -114,6 +127,19 @@ func TestWebhooksEndpoints(t *testing.T) { }, }, + { + description: `GET webhooks of a software with a "from" query param later than every webhook`, + query: "GET /v1/software/9f135268-a37e-4ead-96ec-e4a24bb9344a/webhooks?from=2020-01-01T00:00:00Z", + headers: map[string][]string{"Authorization": {goodToken}}, + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]interface{}) { + data := assertListResponse(t, response) + + assert.Empty(t, data) + }, + }, + // PATCH /webhooks/:id { description: "PATCH webhook is not supported",