MemberJunction / MemberJunction/MJ

CodeGen should warn when a field has ValueListType='List' but no EntityFieldValue rows

Open
#3,986 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

## Summary

CodeGen can set `EntityField.ValueListType='List'` on a field that has **no `EntityFieldValue` rows**. That combination describes a field where *no value is legal*, which should never exist — it means the metadata is broken. Nothing catches it at generation time, so it surfaces (if at all) at runtime.

Raised by @AN-BC in review of PR #3972:

> This is also something that CodeGen _could_ check for - a column that has ValueListType == List and has no values. Since the vast majority of the situations that drive the setting of ValueListType come from CodeGen parsing check constraints this is, again, extraordinarily unlikely.

- Status: OPEN
- Layer: `@memberjunction/codegen-lib` — `ManageMetadataBase.manageEntityFieldValuesAndValidatorFunctions` / `applyValueListConfig`

## Why it is worth a build-time check even though it is unlikely

The runtime half already landed: `EntityFieldInfo.ValueIsPermittedByValueList()` (#3969, PR #3972) permits everything for such a field and emits a `LogError` naming the entity and field, latched once per `EntityFieldInfo`. That is the right runtime behaviour — refusing every value would fail every save on the field — but it is a *late* and *conditional* report: it needs the field to be validated in a running process before anyone learns the metadata is wrong.

CodeGen already has everything needed to say it at generation time, where it is a build-time warning against the definitive metadata rather than a log line someone has to notice in production.

## Where it can arise

1. **`ValueListType` set, then the values removed.** `syncEntityFieldValues` deletes `EntityFieldValue` rows no longer present in the parsed list, but the `ValueListType='List'` update is a separate statement (`manage-metadata.ts` ~5270) — nothing re-evaluates the type when the list empties.
2. **A CHECK constraint edited into a shape the parser no longer matches.** `parseCheckConstraintValues` returns null for anything that is not exactly a disjunction of quoted equalities, and the constraint then routes to the AI-validator path — leaving an earlier `ValueListType='List'` in place with a list that is now stale or emptied.
3. **A hand-authored or soft (`applyValueListConfig`) list whose `PossibleValues` is empty.** `SoftFieldValueListConfig.PossibleValues` is not checked for emptiness before the type is applied.
4. **Metadata edited directly**, which is how it would most plausibly happen in a real deployment.

## Suggested fix

After `manageEntityFieldValuesAndValidatorFunctions` completes, one query over the metadata it just wrote:

```sql
SELECT e.Name AS EntityName, ef.Name AS FieldName
FROM __mj.EntityField ef
JOIN __mj.Entity e ON e.ID = ef.EntityID
WHERE ef.ValueListType = 'List'
AND NOT EXISTS (SELECT 1 FROM __mj.EntityFieldValue v WHERE v.EntityFieldID = ef.ID);
```

Report each hit as a CodeGen warning naming the entity and field, with the two remedies: re-derive the list, or set `ValueListType='None'` if the field is not meant to be constrained. A warning rather than an error, since the runtime fails open and this should not break a build.

Two decisions for whoever picks it up:

1. **Warn or auto-correct?** CodeGen could set `ValueListType='None'` itself, since `List`-with-no-values is never meaningful. That is tempting but it silently discards the *intent* that someone marked the field as constrained — a warning keeps the human in the loop, which matches how the runtime handles it (permit, but say so).
2. **Should the same pass check the inverse** — `EntityFieldValue` rows present while `ValueListType='None'`? That is the more common drift direction and equally cheap to detect in the same query.

## Related

- #3969 / PR #3972 — the runtime `LogError` this would pre-empt.
- #3978 — numeric/bit `IN (…)` lists produce no value list at all (a different parser gap in the same function).
- #3985 — eight MJ core `List` fields whose SQL default is not in their own value list; a related "the metadata disagrees with itself" check that CodeGen is also well placed to catch.

Contributor guide

Open the contributing guide

Research direction

Start in @memberjunction/codegen-lib's manage-metadata.ts, especially manageEntityFieldValuesAndValidatorFunctions, syncEntityFieldValues, and applyValueListConfig. Trace the metadata writes, then add the post-generation check so each List field without EntityFieldValue rows produces a warning naming its entity and field; preserve the suggested remedies and do not fail the build.

Written by the indexing model from the issue text.

Assessment

Tech stack
sql, typescript
Domain
databases, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.