Refactor domain-match lookup to idiomatic Load + filter
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é <emile@probo.com>
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
45
pkg/coredata/common_third_party_domain_filter.go
Normal file
45
pkg/coredata/common_third_party_domain_filter.go
Normal file
@@ -0,0 +1,45 @@
|
||||
// Copyright (c) 2026 Probo Inc <hello@getprobo.com>.
|
||||
//
|
||||
// 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
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user