feat(brands): persist and return URL type field in brand_sites (LLMO-4058)

[dzehnder]

Summary

• Updates syncBrandSites() to track and persist the type field (e.g. "base") from URL entries to the brand_sites table
• Updates mapDbBrandToV2() and BRAND_SELECT to return type on base-URL entries
• 3 new test cases covering type persistence, type in V2 mapping, and backward compatibility

Prerequisite: DB migration required before deploy:

ALTER TABLE brand_sites ADD COLUMN type TEXT DEFAULT NULL;

Companion PR: adobe-rnd/llmo-data-retrieval-service#1222 (DRS injects baseUrl with type: "base")

Jira: LLMO-4058

Test plan

• CI green (unit tests)
• DB migration applied to dev/stage
• Integration test: upsert a brand with a URL having type: "base", verify GET returns it
• Deploy before companion DRS PR

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[irenelagno]

Code Review

🟡 type only emitted on root path — consider a clarifying comment (Low)

In mapDbBrandToV2:

if (p === '/' && hasText(bs.type)) entry.type = bs.type;

This is correct behavior — subpaths under a base URL shouldn't carry type: "base". But it's non-obvious to future readers why the condition is p === '/'. A short inline comment would help, e.g.:

// Only the root entry (/) carries the base-URL type; subpaths are plain URLs
if (p === '/' && hasText(bs.type)) entry.type = bs.type;

🟢 typeByBase hardcoded to "base" (Nit)

if (typeof u === 'object' && u?.type === 'base') typeByBase.set(normalizedBase, 'base');

This only tracks type === 'base'. If another URL type is ever introduced it won't be persisted. Could future-proof with:

if (typeof u === 'object' && u?.type) typeByBase.set(normalizedBase, u.type);

Not blocking — just a consideration for extensibility.

---

Overall the changes look solid. CI is green. 👍

[alinarublea]

Staff/Distinguished Engineer Review

Verdict: Request changes (1 blocking, rest are suggestions)

The implementation is solid structurally. Review feedback from @irenelagno was addressed well. Test coverage is complete per Codecov. The backward-compatibility story is good (no type key emitted when absent).

What's good

• hasText(bs.type) guard means the response shape stays clean — no type: null leaking into the API contract.
• The clarifying comment on the root-path condition is helpful.
• Making typeByBase generic (storing any type, not just "base") was the right call.
• Test cases cover the three important scenarios: type present, type absent, and round-trip through upsert.

Blocking

1. Last-write-wins data loss on typeByBase. If a caller sends two URL objects with the same base URL but different types, the last one silently overwrites the first:

   urls: [
     { value: 'https://adobe.com/en', type: 'base' },
     { value: 'https://adobe.com/fr', type: 'localized' }
   ]

   Both normalize to https://adobe.com. The map ends up with type: 'localized', dropping 'base' with no warning. This is a silent data loss bug. Either:

   • Validate that conflicting types for the same base URL are rejected (throw), or
   • Store type per-path rather than per-base, if the model supports it, or
   • Document that type is strictly per-base-URL and the first/last-one-wins behavior is intentional (with a test proving it).

Non-blocking suggestions

2. No input validation on type values. Any string passes through to the database. If type has a defined vocabulary ('base', potentially others), validate at the API boundary. A const VALID_BRAND_SITE_TYPES = new Set(['base']) with a check is ~3 lines and prevents schema drift.

3. type is an extremely generic column/field name. In JavaScript, type isn't reserved but it collides with TypeScript discriminators, JSON-Schema keywords, and common destructuring patterns. url_role or site_type would be more self-documenting and grep-friendly. I realize this is likely settled at the API contract level already, but raising it — renaming is cheapest before any consumers depend on it.

4. Commit hygiene. 7 commits including 2 merge commits and a CI-retrigger empty commit. Squash-merge this to keep main history clean.

5. Test: verify the overwrite scenario. Add a test for the case in point #1 — two URL objects with the same base URL but different types. Whatever the intended behavior is, it should be documented by a test.

Cross-PR considerations

• Deploy ordering is correct and well-documented: migration first, then API, then DRS is already merged and will start sending type: "base".
• Rollback story: If the API PR needs to be rolled back, the column stays in the DB harmlessly (nullable, no consumers). Clean.
• Schema evolution: If more types are added later, the TEXT column + application-level validation approach will work, but consider the CHECK constraint (PR #326 feedback) to keep the DB as the source of truth for allowed values.

[alinarublea]

Staff/Distinguished Engineer Re-review

Verdict: Approve

The blocking issue from the first review has been addressed in commit 2e47cd2:

Blocking item — resolved

Last-write-wins data loss — now uses first-write-wins with !typeByBase.has(normalizedBase) guard. The comment explains the intent clearly. A test ('uses first type when multiple URLs share same base with different types') proves the behavior. The "first wins silently" approach is reasonable for this use case — the primary caller (DRS brandalf sync) sends type: "base" only on the canonical URL, so conflicts are unlikely in practice.

Non-blocking items — status

These remain unaddressed but are not blocking:

• Input validation on type: Partially mitigated by the CHECK constraint added in the companion migration PR (#326), which will reject anything other than 'base' at the DB level. Application-level validation would still be a nice-to-have for better error messages, but the DB constraint is the more important guard.
• Generic type field name: Acknowledged as likely settled at the API contract level.
• Commit hygiene: 8 commits including merge commits. Recommend squash-merge.

Overall a solid, well-tested change. The cross-PR story (migration → API → DRS) is clean and the rollback path is safe.

[solaris007]

🎉 This PR is included in version 1.431.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀