Fix review issues in cookie pattern handling
- 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é <emile@getprobo.com>
This commit is contained in:
@@ -15,6 +15,7 @@
|
|||||||
import { Input } from "@probo/ui";
|
import { Input } from "@probo/ui";
|
||||||
|
|
||||||
const UNITS: { value: string; label: string; seconds: number }[] = [
|
const UNITS: { value: string; label: string; seconds: number }[] = [
|
||||||
|
{ value: "seconds", label: "seconds", seconds: 1 },
|
||||||
{ value: "minutes", label: "minutes", seconds: 60 },
|
{ value: "minutes", label: "minutes", seconds: 60 },
|
||||||
{ value: "hours", label: "hours", seconds: 3600 },
|
{ value: "hours", label: "hours", seconds: 3600 },
|
||||||
{ value: "days", label: "days", seconds: 86400 },
|
{ 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 {
|
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;
|
if (isNaN(num) || num <= 0) return null;
|
||||||
const u = UNITS.find(u => u.value === unit);
|
const u = UNITS.find(u => u.value === unit);
|
||||||
if (!u) return null;
|
if (!u) return null;
|
||||||
return num * u.seconds;
|
return Math.round(num * u.seconds);
|
||||||
}
|
}
|
||||||
|
|
||||||
export function fromMaxAgeSeconds(seconds: number | null): { value: string; unit: string } {
|
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 / u.seconds), unit: u.value };
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return { value: String(seconds), unit: "minutes" };
|
return { value: String(seconds), unit: "seconds" };
|
||||||
}
|
}
|
||||||
|
|
||||||
interface DurationInputProps {
|
interface DurationInputProps {
|
||||||
|
|||||||
@@ -29,5 +29,5 @@ export function humanizeSeconds(seconds: number | null): string {
|
|||||||
return `${count} ${count === 1 ? singular : plural}`;
|
return `${count} ${count === 1 ? singular : plural}`;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return `${seconds} seconds`;
|
return `${seconds} ${seconds === 1 ? "second" : "seconds"}`;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1375,7 +1375,7 @@ func (s *Service) GetCookiePattern(
|
|||||||
func(ctx context.Context, conn pg.Querier) error {
|
func(ctx context.Context, conn pg.Querier) error {
|
||||||
if err := pattern.LoadByID(ctx, conn, scope, cookiePatternID); err != nil {
|
if err := pattern.LoadByID(ctx, conn, scope, cookiePatternID); err != nil {
|
||||||
if errors.Is(err, coredata.ErrResourceNotFound) {
|
if errors.Is(err, coredata.ErrResourceNotFound) {
|
||||||
return ErrCookieNotFound
|
return ErrCookiePatternNotFound
|
||||||
}
|
}
|
||||||
return fmt.Errorf("cannot load cookie pattern: %w", err)
|
return fmt.Errorf("cannot load cookie pattern: %w", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -92,7 +92,7 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook
|
|||||||
mergeGroups := findMergeGroups(patterns, patternMergeThreshold)
|
mergeGroups := findMergeGroups(patterns, patternMergeThreshold)
|
||||||
|
|
||||||
merged := false
|
merged := false
|
||||||
for prefix, group := range mergeGroups {
|
for key, group := range mergeGroups {
|
||||||
|
|
||||||
maxAge := mostCommonMaxAge(group)
|
maxAge := mostCommonMaxAge(group)
|
||||||
source := bestSource(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),
|
ID: gid.New(task.TenantID, coredata.CookiePatternEntityType),
|
||||||
OrganizationID: group[0].OrganizationID,
|
OrganizationID: group[0].OrganizationID,
|
||||||
CookieBannerID: task.BannerID,
|
CookieBannerID: task.BannerID,
|
||||||
CookieCategoryID: group[0].CookieCategoryID,
|
CookieCategoryID: key.categoryID,
|
||||||
Pattern: prefix,
|
Pattern: key.prefix,
|
||||||
MatchType: coredata.CookiePatternMatchTypePrefix,
|
MatchType: coredata.CookiePatternMatchTypePrefix,
|
||||||
DisplayName: prefix + "*",
|
DisplayName: key.prefix + "*",
|
||||||
MaxAgeSeconds: maxAge,
|
MaxAgeSeconds: maxAge,
|
||||||
Description: "",
|
Description: "",
|
||||||
Source: source,
|
Source: source,
|
||||||
@@ -114,10 +114,12 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook
|
|||||||
|
|
||||||
inserted, err := prefixPattern.InsertIfNotExists(ctx, tx, scope)
|
inserted, err := prefixPattern.InsertIfNotExists(ctx, tx, scope)
|
||||||
if err != nil {
|
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 {
|
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 {
|
for _, exactPattern := range group {
|
||||||
@@ -135,7 +137,7 @@ func (h *patternAnalysisHandler) Process(ctx context.Context, task coredata.Cook
|
|||||||
h.logger.InfoCtx(
|
h.logger.InfoCtx(
|
||||||
ctx,
|
ctx,
|
||||||
"merged exact patterns into prefix pattern",
|
"merged exact patterns into prefix pattern",
|
||||||
log.String("prefix", prefix),
|
log.String("prefix", key.prefix),
|
||||||
log.Int("count", len(group)),
|
log.Int("count", len(group)),
|
||||||
log.String("banner_id", task.BannerID.String()),
|
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(
|
func findMergeGroups(
|
||||||
patterns coredata.CookiePatterns,
|
patterns coredata.CookiePatterns,
|
||||||
threshold int,
|
threshold int,
|
||||||
) map[string][]*coredata.CookiePattern {
|
) map[mergeGroupKey][]*coredata.CookiePattern {
|
||||||
var exact []*coredata.CookiePattern
|
var exact []*coredata.CookiePattern
|
||||||
for _, p := range patterns {
|
for _, p := range patterns {
|
||||||
if p.MatchType == coredata.CookiePatternMatchTypeExact {
|
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 _, p := range exact {
|
||||||
for _, pfx := range separatorPrefixes(p.Pattern) {
|
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 {
|
type candidate struct {
|
||||||
prefix string
|
key mergeGroupKey
|
||||||
patterns []*coredata.CookiePattern
|
patterns []*coredata.CookiePattern
|
||||||
}
|
}
|
||||||
|
|
||||||
var candidates []candidate
|
var candidates []candidate
|
||||||
for pfx, pats := range prefixCounts {
|
for key, pats := range prefixCounts {
|
||||||
if len(pats) >= threshold {
|
if len(pats) >= threshold {
|
||||||
candidates = append(candidates, candidate{pfx, pats})
|
candidates = append(candidates, candidate{key, pats})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
sort.Slice(candidates, func(i, j int) bool {
|
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)
|
assigned := make(map[*coredata.CookiePattern]bool)
|
||||||
groups := make(map[string][]*coredata.CookiePattern)
|
groups := make(map[mergeGroupKey][]*coredata.CookiePattern)
|
||||||
|
|
||||||
for _, c := range candidates {
|
for _, c := range candidates {
|
||||||
var unassigned []*coredata.CookiePattern
|
var unassigned []*coredata.CookiePattern
|
||||||
@@ -201,7 +209,7 @@ func findMergeGroups(
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
groups[c.prefix] = unassigned
|
groups[c.key] = unassigned
|
||||||
for _, p := range unassigned {
|
for _, p := range unassigned {
|
||||||
assigned[p] = true
|
assigned[p] = true
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -122,6 +122,62 @@ LIMIT 1;
|
|||||||
return nil
|
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(
|
func (cp *CookiePattern) FindMatchingPattern(
|
||||||
ctx context.Context,
|
ctx context.Context,
|
||||||
conn pg.Querier,
|
conn pg.Querier,
|
||||||
@@ -153,7 +209,11 @@ WHERE
|
|||||||
OR (match_type = @match_type_exact AND pattern = @cookie_name)
|
OR (match_type = @match_type_exact AND pattern = @cookie_name)
|
||||||
)
|
)
|
||||||
ORDER BY
|
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;
|
LIMIT 1;
|
||||||
`
|
`
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user