Skip to content

feat(data-access): auto-normalize enum values in PostgREST query filters#1392

Merged
ekremney merged 6 commits intomainfrom
feat/data-access-enum-case-normalization
Mar 2, 2026
Merged

feat(data-access): auto-normalize enum values in PostgREST query filters#1392
ekremney merged 6 commits intomainfrom
feat/data-access-enum-case-normalization

Conversation

@ekremney
Copy link
Copy Markdown
Member

@ekremney ekremney commented Mar 2, 2026

Summary

  • Auto-normalize enum attribute values in PostgREST query filters so that case mismatches (e.g. 'new' vs 'NEW') no longer silently return empty results
  • Adds a #normalizeEnumValue helper to BaseCollection that matches input strings case-insensitively against the known enum values array (attribute.type)
  • Applied in #applyKeyFilters (covers all synthetic accessors, allByIndexKeys, findByIndexKeys, updateByKeys, removeByIndexKeys) and the batchGetByKeys bulk .in() path
  • No schema changes required — automatic for any attribute with type: Object.values(...) (array)

Test plan

  • Unit tests: 6 new tests covering lowercase, mixed-case, non-enum passthrough, non-string passthrough, unrecognized value passthrough, and batch normalization
  • All 1522 existing unit tests pass
  • Integration tests: 2 new tests (Suggestion + Opportunity) verifying case-insensitive enum queries against real Postgres — run with npm run test:it

🤖 Generated with Claude Code

ekremney and others added 2 commits March 2, 2026 08:42
Postgres enum comparisons are case-sensitive, so passing 'new' when the
DB stores 'NEW' silently returns no results. This adds automatic
case-insensitive matching for all array-typed (enum) attributes in
#applyKeyFilters and the batchGetByKeys bulk .in() path. When the
attribute type is an array of known values, the input is matched
case-insensitively and replaced with the correctly-cased enum value.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 2, 2026

This PR will trigger a minor release when merged.

@ekremney ekremney self-assigned this Mar 2, 2026
@ekremney ekremney added the enhancement New feature or request label Mar 2, 2026
@ekremney ekremney requested a review from solaris007 March 2, 2026 07:54
Copy link
Copy Markdown
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ekremney

Verdict: Approve (conditional) - right approach, needs a rebase before merge.

Clean, well-scoped fix at the right layer. The core implementation is sound - 11 lines that solve a real class of bugs. One critical branch divergence issue to resolve.

Must fix before merge

# Issue Severity
1 caseInsensitive flag regression - The branch predates the caseInsensitive / ilike feature in #applyKeyFilters. The PR's version of #applyKeyFilters drops the if (attr?.caseInsensitive) check entirely, breaking Site.baseURL lookups (which rely on ilike). Rebase on main and merge both behaviors: caseInsensitive -> ilike, else -> eq with #normalizeEnumValue. Critical
2 Remove incorrect eslint-disable - // eslint-disable-next-line class-methods-use-this on #normalizeEnumValue is wrong. The method uses this.schema.getAttribute(key). Remove it. Minor (quick fix)
3 Revert package-lock.json noise - 186 lines of unrelated "peer": true additions and sibling package version bumps. Split out or drop. Minor (hygiene)

Should track as follow-ups

# Issue Severity
4 applyWhere gap - The where clause path (postgrest.utils.js) does op.eq() without normalization. where: (attr, op) => op.eq(attr.status, 'new') silently returns empty results. No callers currently use where with enum fields, so this is latent. Document or fix in follow-up. Important (latent)
5 Write-side asymmetry - Reads now accept 'new' for 'NEW', but #validateItem on the write path uses strict includes() and will reject 'new'. Not a bug per se - reads are forgiving, writes enforce canonical values - but should be documented to avoid confusion. Low
6 removeByIndexKeys bulk .in() path - Not normalized (unlike batchGetByKeys which is). Low severity since bulk deletes typically use UUIDs, not enum values. Low
7 Direct PostgREST consumers - Mystique queries PostgREST directly and won't benefit from this fix. If Mystique hits the same case regression, a DB-level solution (citext) will still be needed. Informational

No concerns

  • Security - No vulnerabilities. Bounded match set from schema definitions, safe fallback, parameterized queries. No injection or authorization risk.
  • Performance - .find() with .toLowerCase() over 2-8 enum values per key is negligible against PostgREST round-trip latency.
  • Schema alignment - All model constants are consistently uppercase and match the schema type arrays.
  • FK / ID resolution - #normalizeEnumValue correctly skips non-enum attributes, so UUIDs and FK attributes are untouched.

Recommended merged #applyKeyFilters

After rebase, the method should look like:

#applyKeyFilters(query, keys) {
  if (!isNonEmptyObject(keys)) return query;
  let filtered = query;
  Object.entries(keys).forEach(([key, value]) => {
    const dbField = this.#toDbField(key);
    const attr = this.schema.getAttribute(key);
    if (attr?.caseInsensitive && typeof value === 'string') {
      const escaped = value.replace(/[%_]/g, '\\$&');
      filtered = filtered.ilike(dbField, escaped);
    } else {
      filtered = filtered.eq(dbField, this.#normalizeEnumValue(key, value));
    }
  });
  return filtered;
}

@solaris007
Copy link
Copy Markdown
Member

Correction to review item #1

The caseInsensitive / ilike feature in #applyKeyFilters was from PR #1388, which is closed and never merged to main. So there is no regression - the PR branch is up-to-date with main as-is.

That said, the two features (caseInsensitive ilike for baseURL and enum normalization for status fields) are complementary and should eventually both land. When they do, the merged #applyKeyFilters shown in the review is the right target shape. But this is a coordination item, not a blocker for this PR.

Updated verdict: items #2 and #3 remain (eslint-disable fix, package-lock cleanup). Item #1 is not a blocker.

ekremney and others added 2 commits March 2, 2026 13:13
- Remove incorrect eslint-disable on #normalizeEnumValue (uses this)
- Normalize enum values in removeByIndexKeys bulk .in() path

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ekremney
Copy link
Copy Markdown
Member Author

ekremney commented Mar 2, 2026

Thanks for the thorough review @solaris007!

Addressed the feedback:

Fixed:

  • #2: Removed the incorrect eslint-disable-next-line class-methods-use-this — good catch, the method does use this.
  • #3: Reverted the package-lock.json noise.
  • #6: Added enum normalization to the removeByIndexKeys bulk .in() path as well — easy win.

Acknowledged follow-ups:

  • #4 (applyWhere gap): Agreed, no callers currently use where with enum fields so it's latent. Will track for a follow-up.
  • #5 (write-side asymmetry): Intentional — reads should be forgiving, writes should enforce canonical values. Worth documenting.
  • #7 (direct PostgREST consumers): Noted. If Mystique hits the same issue, a DB-level solution would be the right approach.

And confirmed — #1 is not a blocker since PR #1388 was never merged. When both land, the merged #applyKeyFilters shape you suggested is the right target.

@ekremney
Copy link
Copy Markdown
Member Author

ekremney commented Mar 2, 2026

Re #3 (package-lock.json): turns out the lockfile update is actually required — main's package.json references newer sibling packages (@adobe/spacecat-shared-ims-client@1.11.10, jose@6.1.2, etc.) that aren't reflected in main's lockfile. Without it, npm ci fails in CI. Reverted the revert to keep it in.

@ekremney ekremney merged commit f44d0d2 into main Mar 2, 2026
7 checks passed
@ekremney ekremney deleted the feat/data-access-enum-case-normalization branch March 2, 2026 12:34
solaris007 pushed a commit that referenced this pull request Mar 2, 2026
## [@adobe/spacecat-shared-data-access-v3.5.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v3.4.2...@adobe/spacecat-shared-data-access-v3.5.0) (2026-03-02)

### Features

* **data-access:** auto-normalize enum values in PostgREST query filters ([#1392](#1392)) ([f44d0d2](f44d0d2))
@solaris007
Copy link
Copy Markdown
Member

🎉 This PR is included in version @adobe/spacecat-shared-data-access-v3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ekremney added a commit to adobe/spacecat-api-service that referenced this pull request Mar 2, 2026
…ries (#1890)

## Summary

- Adds 9 integration tests verifying that `by-status` and
`by-delivery-type` API endpoints work with lowercase and mixed-case enum
values (e.g. `new`, `New` instead of `NEW`)
- Exercises the auto-normalize enum feature added to `BaseCollection` in
spacecat-shared-data-access 3.5.0
([spacecat-shared#1392](adobe/spacecat-shared#1392))
end-to-end through the full HTTP → controller → data-access → PostgREST
stack

### New tests by entity

| File | Tests | What they verify |
|---|---|---|
| `opportunities.js` | 2 | `/by-status/new` and `/by-status/New` return
same results as `/by-status/NEW` |
| `suggestions.js` | 3 | `/by-status/new`, `/by-status/New`, and
`/by-status/new/paged/10` |
| `fixes.js` | 2 | `/by-status/pending` and `/by-status/Pending` |
| `sites.js` | 2 | `/by-delivery-type/AEM_EDGE` and
`/by-delivery-type/Aem_Edge` |

## Test plan

- [ ] PostgreSQL IT suite passes: `npx mocha --require
test/it/postgres/harness.js --timeout 30000
'test/it/postgres/**/*.test.js'`
- [ ] DynamoDB IT suite passes: `npx mocha --require
test/it/dynamo/harness.js --timeout 30000 'test/it/dynamo/**/*.test.js'`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants