From 2fe77d9ddcb8e41346c8544414c34cfab9515561 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89mile=20R=C3=A9?= Date: Thu, 30 Apr 2026 10:26:25 +0400 Subject: [PATCH] Fix review issues in cookie pattern handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix DurationInput fallback unit from "minutes" to "seconds" and add seconds as a selectable unit to prevent silent duration inflation - Use parseFloat instead of parseInt for duration input to preserve fractional values - Scope prefix merge groups by category ID to prevent cross-category merging - Relink cookies and delete exact patterns even when prefix pattern already exists - Prefer exact matches and longest prefix in pattern selection query - Fix wrong error type in GetCookiePattern (ErrCookiePatternNotFound) - Handle singular/plural in humanizeSeconds fallback branch Signed-off-by: Émile Ré --- .../cookies/_components/DurationInput.tsx | 7 ++- packages/helpers/src/duration.ts | 2 +- pkg/cookiebanner/service.go | 2 +- pkg/cookiebanner/worker.go | 40 +++++++----- pkg/coredata/cookie_pattern.go | 62 ++++++++++++++++++- 5 files changed, 91 insertions(+), 22 deletions(-) diff --git a/apps/console/src/pages/organizations/cookie-banners/configuration/cookies/_components/DurationInput.tsx b/apps/console/src/pages/organizations/cookie-banners/configuration/cookies/_components/DurationInput.tsx index 02ee97152..fe0a43a95 100644 --- a/apps/console/src/pages/organizations/cookie-banners/configuration/cookies/_components/DurationInput.tsx +++ b/apps/console/src/pages/organizations/cookie-banners/configuration/cookies/_components/DurationInput.tsx @@ -15,6 +15,7 @@ import { Input } from "@probo/ui"; const UNITS: { value: string; label: string; seconds: number }[] = [ + { value: "seconds", label: "seconds", seconds: 1 }, { value: "minutes", label: "minutes", seconds: 60 }, { value: "hours", label: "hours", seconds: 3600 }, { value: "days", label: "days", seconds: 86400 }, @@ -24,11 +25,11 @@ const UNITS: { value: string; label: string; seconds: number }[] = [ ]; export function toMaxAgeSeconds(value: string, unit: string): number | null { - const num = parseInt(value, 10); + const num = parseFloat(value); if (isNaN(num) || num <= 0) return null; const u = UNITS.find(u => u.value === unit); if (!u) return null; - return num * u.seconds; + return Math.round(num * u.seconds); } export function fromMaxAgeSeconds(seconds: number | null): { value: string; unit: string } { @@ -38,7 +39,7 @@ export function fromMaxAgeSeconds(seconds: number | null): { value: string; unit return { value: String(seconds / u.seconds), unit: u.value }; } } - return { value: String(seconds), unit: "minutes" }; + return { value: String(seconds), unit: "seconds" }; } interface DurationInputProps { diff --git a/packages/helpers/src/duration.ts b/packages/helpers/src/duration.ts index 969e04eea..6fdf18a36 100644 --- a/packages/helpers/src/duration.ts +++ b/packages/helpers/src/duration.ts @@ -29,5 +29,5 @@ export function humanizeSeconds(seconds: number | null): string { return `${count} ${count === 1 ? singular : plural}`; } } - return `${seconds} seconds`; + return `${seconds} ${seconds === 1 ? "second" : "seconds"}`; } diff --git a/pkg/cookiebanner/service.go b/pkg/cookiebanner/service.go index 6613a3610..ac38d25db 100644 --- a/pkg/cookiebanner/service.go +++ b/pkg/cookiebanner/service.go @@ -1375,7 +1375,7 @@ func (s *Service) GetCookiePattern( func(ctx context.Context, conn pg.Querier) error { if err := pattern.LoadByID(ctx, conn, scope, cookiePatternID); err != nil { if errors.Is(err, coredata.ErrResourceNotFound) { - return ErrCookieNotFound + return ErrCookiePatternNotFound } return fmt.Errorf("cannot load cookie pattern: %w", err) } diff --git a/pkg/cookiebanner/worker.go b/pkg/cookiebanner/worker.go index 0c07b3d30..710b534eb 100644 --- a/pkg/cookiebanner/worker.go +++ b/pkg/cookiebanner/worker.go @@ -92,7 +92,7 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook mergeGroups := findMergeGroups(patterns, patternMergeThreshold) merged := false - for prefix, group := range mergeGroups { + for key, group := range mergeGroups { maxAge := mostCommonMaxAge(group) source := bestSource(group) @@ -101,10 +101,10 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook ID: gid.New(task.TenantID, coredata.CookiePatternEntityType), OrganizationID: group[0].OrganizationID, CookieBannerID: task.BannerID, - CookieCategoryID: group[0].CookieCategoryID, - Pattern: prefix, + CookieCategoryID: key.categoryID, + Pattern: key.prefix, MatchType: coredata.CookiePatternMatchTypePrefix, - DisplayName: prefix + "*", + DisplayName: key.prefix + "*", MaxAgeSeconds: maxAge, Description: "", Source: source, @@ -114,10 +114,12 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook inserted, err := prefixPattern.InsertIfNotExists(ctx, tx, scope) if err != nil { - return fmt.Errorf("cannot insert prefix pattern %q: %w", prefix, err) + return fmt.Errorf("cannot insert prefix pattern %q: %w", key.prefix, err) } if !inserted { - continue + if err := prefixPattern.LoadByBannerIDAndPattern(ctx, tx, scope, task.BannerID, key.prefix); err != nil { + return fmt.Errorf("cannot load existing prefix pattern %q: %w", key.prefix, err) + } } for _, exactPattern := range group { @@ -135,7 +137,7 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook h.logger.InfoCtx( ctx, "merged exact patterns into prefix pattern", - log.String("prefix", prefix), + log.String("prefix", key.prefix), log.Int("count", len(group)), log.String("banner_id", task.BannerID.String()), ) @@ -152,10 +154,15 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook ) } +type mergeGroupKey struct { + categoryID gid.GID + prefix string +} + func findMergeGroups( patterns coredata.CookiePatterns, threshold int, -) map[string][]*coredata.CookiePattern { +) map[mergeGroupKey][]*coredata.CookiePattern { var exact []*coredata.CookiePattern for _, p := range patterns { if p.MatchType == coredata.CookiePatternMatchTypeExact { @@ -163,31 +170,32 @@ func findMergeGroups( } } - prefixCounts := make(map[string][]*coredata.CookiePattern) + prefixCounts := make(map[mergeGroupKey][]*coredata.CookiePattern) for _, p := range exact { for _, pfx := range separatorPrefixes(p.Pattern) { - prefixCounts[pfx] = append(prefixCounts[pfx], p) + key := mergeGroupKey{categoryID: p.CookieCategoryID, prefix: pfx} + prefixCounts[key] = append(prefixCounts[key], p) } } type candidate struct { - prefix string + key mergeGroupKey patterns []*coredata.CookiePattern } var candidates []candidate - for pfx, pats := range prefixCounts { + for key, pats := range prefixCounts { if len(pats) >= threshold { - candidates = append(candidates, candidate{pfx, pats}) + candidates = append(candidates, candidate{key, pats}) } } sort.Slice(candidates, func(i, j int) bool { - return len(candidates[i].prefix) > len(candidates[j].prefix) + return len(candidates[i].key.prefix) > len(candidates[j].key.prefix) }) assigned := make(map[*coredata.CookiePattern]bool) - groups := make(map[string][]*coredata.CookiePattern) + groups := make(map[mergeGroupKey][]*coredata.CookiePattern) for _, c := range candidates { var unassigned []*coredata.CookiePattern @@ -201,7 +209,7 @@ func findMergeGroups( continue } - groups[c.prefix] = unassigned + groups[c.key] = unassigned for _, p := range unassigned { assigned[p] = true } diff --git a/pkg/coredata/cookie_pattern.go b/pkg/coredata/cookie_pattern.go index 44dab3059..8bbb24dc2 100644 --- a/pkg/coredata/cookie_pattern.go +++ b/pkg/coredata/cookie_pattern.go @@ -122,6 +122,62 @@ LIMIT 1; return nil } +func (cp *CookiePattern) LoadByBannerIDAndPattern( + ctx context.Context, + conn pg.Querier, + scope Scoper, + cookieBannerID gid.GID, + pattern string, +) error { + q := ` +SELECT + id, + organization_id, + cookie_banner_id, + cookie_category_id, + pattern, + match_type, + display_name, + max_age_seconds, + description, + source, + created_at, + updated_at +FROM + cookie_patterns +WHERE + %s + AND cookie_banner_id = @cookie_banner_id + AND pattern = @pattern +LIMIT 1; +` + + q = fmt.Sprintf(q, scope.SQLFragment()) + + args := pgx.StrictNamedArgs{ + "cookie_banner_id": cookieBannerID, + "pattern": pattern, + } + maps.Copy(args, scope.SQLArguments()) + + rows, err := conn.Query(ctx, q, args) + if err != nil { + return fmt.Errorf("cannot query cookie patterns: %w", err) + } + + p, err := pgx.CollectExactlyOneRow(rows, pgx.RowToStructByName[CookiePattern]) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrResourceNotFound + } + return fmt.Errorf("cannot collect cookie pattern: %w", err) + } + + *cp = p + + return nil +} + func (cp *CookiePattern) FindMatchingPattern( ctx context.Context, conn pg.Querier, @@ -153,7 +209,11 @@ WHERE OR (match_type = @match_type_exact AND pattern = @cookie_name) ) ORDER BY - CASE WHEN match_type = @match_type_prefix THEN 0 ELSE 1 END + CASE WHEN match_type = @match_type_exact AND pattern = @cookie_name THEN 0 + WHEN match_type = @match_type_prefix THEN 1 + ELSE 2 + END, + LENGTH(pattern) DESC LIMIT 1; `