From 10adf2bd4bdb8044233d9d4de0ff4f341dbed3dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89mile=20R=C3=A9?= Date: Tue, 19 May 2026 11:52:57 +0400 Subject: [PATCH] Refactor domain-match lookup to idiomatic Load + filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the cross-entity JOIN in DetectedTrackers.LoadCommonThirdPartyIDByDomainMatch with two idiomatic coredata calls: LoadInitiatorDomainsByTrackerPatternID on DetectedTrackers, then a new CommonThirdPartyDomains.Load with a CommonThirdPartyDomainFilter. Each entity now queries only its own table, and the caller orchestrates the lookup. Document the Load vs LoadAll naming convention and the no cross-entity JOINs rule in contrib/claude/coredata.md. Signed-off-by: Émile Ré --- contrib/claude/coredata.md | 19 +++++++- pkg/cookiebanner/tracker_mapping_worker.go | 22 ++++++--- pkg/coredata/common_third_party_domain.go | 42 +++++++++++++++++ .../common_third_party_domain_filter.go | 45 +++++++++++++++++++ pkg/coredata/detected_tracker.go | 36 +++------------ 5 files changed, 128 insertions(+), 36 deletions(-) create mode 100644 pkg/coredata/common_third_party_domain_filter.go diff --git a/contrib/claude/coredata.md b/contrib/claude/coredata.md index eaae866bc..a11976768 100644 --- a/contrib/claude/coredata.md +++ b/contrib/claude/coredata.md @@ -100,7 +100,10 @@ This ensures the compiler catches renamed or removed enum values instead of sile | -------------------------------------------------------- | ----------- | ---------------------------- | ------------------------------------ | | `LoadByID(ctx, conn, scope, id)` | `*Entity` | `error` | Single entity by ID | | `LoadBy*(ctx, conn, scope, key)` | `*Entity` | `error` | Single entity by unique key | -| `LoadAllBy*(ctx, conn, scope, parentID, cursor, filter)` | `*Entities` | `error` | Paginated list | +| `LoadBy*(ctx, conn, scope, parentID, cursor, filter)` | `*Entities` | `error` | Paginated list (cursor provides limit) | +| `Load(ctx, conn, limit, filter)` | `*Entities` | `error` | Filtered list with explicit limit | +| `LoadAllBy*(ctx, conn, scope, parentID)` | `*Entities` | `error` | All matching rows (never cursor/limit) | +| `LoadAll(ctx, conn, filter)` | `*Entities` | `error` | All matching rows with filter (never cursor/limit) | | `CountBy*(ctx, conn, scope, parentID, filter)` | `*Entities` | `(int, error)` | Count matching rows | | `Insert(ctx, conn, scope)` | `*Entity` | `error` | Insert, uses `scope.GetTenantID()` | | `Update(ctx, conn, scope)` | `*Entity` | `error` | Update via `Exec` (no `RETURNING`) | @@ -108,6 +111,20 @@ This ensures the compiler catches renamed or removed enum values instead of sile | `CursorKey(orderField)` | `*Entity` | `page.CursorKey` | Cursor for pagination | | `AuthorizationAttributes(ctx, conn)` | `*Entity` | `(map[string]string, error)` | Attributes for IAM policy evaluation | +### Load vs LoadAll naming + +The method name signals whether the result set is bounded: + +- **`LoadBy*` with a `cursor` param** — paginated list tied to a GraphQL connection; the cursor provides the limit and ordering. Example: `Assets.LoadByOrganizationID(ctx, conn, scope, orgID, cursor)`. +- **`Load` / `LoadBy*` with a `limit int` param** — filtered list with an explicit limit, used when cursor pagination is not needed but the caller controls the result count. Example: `CommonThirdPartyDomains.Load(ctx, conn, 1, filter)`. +- **`LoadAllBy*`** — returns all matching rows with no limit or cursor. Use only when the full set is needed (e.g. all categories for a banner). +- **`LoadAll`** — same as `LoadAllBy*` but without a parent key; returns all rows matching a filter. Example: `CommonThirdParties.LoadAll(ctx, conn, filter)`. + +`LoadAll*` methods must **never** accept a cursor or limit parameter — the `All` suffix means the entire matching set is returned. If a bounded result is needed, use `Load*` or `LoadBy*` with a `cursor` or `limit int` instead. The codebase has some legacy `LoadAllBy*` methods that accept a cursor; do not follow that pattern — new code must use `LoadBy*` for paginated queries. + +## No cross-entity JOINs + +Each entity file queries only its own table. When data from multiple entities is needed, the caller orchestrates separate calls. Never write a JOIN between two entity tables inside an entity method, and never return a raw ID belonging to a different entity — return the full entity and let the caller read the foreign key field. ## Row collection diff --git a/pkg/cookiebanner/tracker_mapping_worker.go b/pkg/cookiebanner/tracker_mapping_worker.go index 6bac20c2a..140579fdb 100644 --- a/pkg/cookiebanner/tracker_mapping_worker.go +++ b/pkg/cookiebanner/tracker_mapping_worker.go @@ -229,19 +229,31 @@ func (h *trackerMappingHandler) matchByDomain( tp coredata.TrackerPattern, ) (*gid.GID, *gid.GID, error) { var trackers coredata.DetectedTrackers - commonThirdPartyID, err := trackers.LoadCommonThirdPartyIDByDomainMatch(ctx, tx, tp.ID) + domains, err := trackers.LoadInitiatorDomainsByTrackerPatternID(ctx, tx, tp.ID, 10) if err != nil { - return nil, nil, fmt.Errorf("cannot load common third party ID from domain: %w", err) + return nil, nil, fmt.Errorf("cannot load initiator domains: %w", err) } - if commonThirdPartyID == nil { + if len(domains) == 0 { return nil, nil, nil } + filter := coredata.NewCommonThirdPartyDomainFilter(domains) + var matchedDomains coredata.CommonThirdPartyDomains + if err := matchedDomains.Load(ctx, tx, 1, filter); err != nil { + return nil, nil, fmt.Errorf("cannot load common third party domain by domain match: %w", err) + } + + if len(matchedDomains) == 0 { + return nil, nil, nil + } + + commonThirdPartyID := matchedDomains[0].CommonThirdPartyID + now := time.Now() commonPattern := coredata.CommonTrackerPattern{ ID: gid.New(gid.NilTenant, coredata.CommonTrackerPatternEntityType), - CommonThirdPartyID: commonThirdPartyID, + CommonThirdPartyID: &commonThirdPartyID, TrackerType: tp.TrackerType, Pattern: tp.Pattern, MatchType: tp.MatchType, @@ -270,7 +282,7 @@ func (h *trackerMappingHandler) identifyWithAgent( tp coredata.TrackerPattern, ) (*gid.GID, *gid.GID, error) { var trackers coredata.DetectedTrackers - domains, err := trackers.LoadInitiatorDomainsByTrackerPatternID(ctx, tx, tp.ID) + domains, err := trackers.LoadInitiatorDomainsByTrackerPatternID(ctx, tx, tp.ID, 5) if err != nil { h.logger.WarnCtx(ctx, "cannot load initiator domains for agent", log.Error(err)) } diff --git a/pkg/coredata/common_third_party_domain.go b/pkg/coredata/common_third_party_domain.go index 6e4952784..dfd0b1ff8 100644 --- a/pkg/coredata/common_third_party_domain.go +++ b/pkg/coredata/common_third_party_domain.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "maps" "time" "github.com/jackc/pgx/v5" @@ -189,6 +190,47 @@ func (d CommonThirdPartyDomain) Delete( return nil } +func (ds *CommonThirdPartyDomains) Load( + ctx context.Context, + conn pg.Querier, + limit int, + filter *CommonThirdPartyDomainFilter, +) error { + q := ` +SELECT + id, + common_third_party_id, + domain, + created_at, + updated_at +FROM + common_third_party_domains +WHERE + %s +ORDER BY domain ASC +LIMIT @limit; +` + + q = fmt.Sprintf(q, filter.SQLFragment()) + + args := pgx.StrictNamedArgs{"limit": limit} + maps.Copy(args, filter.SQLArguments()) + + rows, err := conn.Query(ctx, q, args) + if err != nil { + return fmt.Errorf("cannot query common third party domains: %w", err) + } + + domains, err := pgx.CollectRows(rows, pgx.RowToAddrOfStructByName[CommonThirdPartyDomain]) + if err != nil { + return fmt.Errorf("cannot collect common third party domains: %w", err) + } + + *ds = domains + + return nil +} + func (ds *CommonThirdPartyDomains) LoadByCommonThirdPartyID( ctx context.Context, conn pg.Querier, diff --git a/pkg/coredata/common_third_party_domain_filter.go b/pkg/coredata/common_third_party_domain_filter.go new file mode 100644 index 000000000..cb7a71a8e --- /dev/null +++ b/pkg/coredata/common_third_party_domain_filter.go @@ -0,0 +1,45 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package coredata + +import ( + "github.com/jackc/pgx/v5" +) + +type CommonThirdPartyDomainFilter struct { + domains []string +} + +func NewCommonThirdPartyDomainFilter(domains []string) *CommonThirdPartyDomainFilter { + return &CommonThirdPartyDomainFilter{domains: domains} +} + +func (f *CommonThirdPartyDomainFilter) SQLFragment() string { + return `( + CASE + WHEN @filter_domains::text[] IS NOT NULL THEN + domain = ANY(@filter_domains::text[]) + ELSE TRUE + END +)` +} + +func (f *CommonThirdPartyDomainFilter) SQLArguments() pgx.StrictNamedArgs { + args := pgx.StrictNamedArgs{"filter_domains": nil} + if len(f.domains) > 0 { + args["filter_domains"] = f.domains + } + return args +} diff --git a/pkg/coredata/detected_tracker.go b/pkg/coredata/detected_tracker.go index 98f713e7f..3982555ed 100644 --- a/pkg/coredata/detected_tracker.go +++ b/pkg/coredata/detected_tracker.go @@ -16,7 +16,6 @@ package coredata import ( "context" - "errors" "fmt" "maps" "time" @@ -152,47 +151,24 @@ WHERE return count, nil } -func (dts *DetectedTrackers) LoadCommonThirdPartyIDByDomainMatch( - ctx context.Context, - conn pg.Querier, - trackerPatternID gid.GID, -) (*gid.GID, error) { - q := ` -SELECT DISTINCT ctpd.common_third_party_id -FROM detected_trackers dt -JOIN common_third_party_domains ctpd ON ctpd.domain = dt.initiator_domain -WHERE dt.tracker_pattern_id = @tracker_pattern_id - AND dt.initiator_domain IS NOT NULL -LIMIT 1; -` - - args := pgx.StrictNamedArgs{"tracker_pattern_id": trackerPatternID} - - var commonThirdPartyID gid.GID - if err := conn.QueryRow(ctx, q, args).Scan(&commonThirdPartyID); err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, nil - } - return nil, fmt.Errorf("cannot load common third party ID by tracker pattern: %w", err) - } - - return &commonThirdPartyID, nil -} - func (dts *DetectedTrackers) LoadInitiatorDomainsByTrackerPatternID( ctx context.Context, conn pg.Querier, trackerPatternID gid.GID, + limit int, ) ([]string, error) { q := ` SELECT DISTINCT initiator_domain FROM detected_trackers WHERE tracker_pattern_id = @tracker_pattern_id AND initiator_domain IS NOT NULL -LIMIT 5; +LIMIT @limit; ` - args := pgx.StrictNamedArgs{"tracker_pattern_id": trackerPatternID} + args := pgx.StrictNamedArgs{ + "tracker_pattern_id": trackerPatternID, + "limit": limit, + } rows, err := conn.Query(ctx, q, args) if err != nil {