Clarin9/Port per-field type binding (submit.type-bind.field "A=>B") (#876)

[milanmajchrak]

Problem description

Fixes A3 of dataquest-dev/dspace-customers#876 (analysed in dataquest-dev/dspace-customers#871).

Admin → new submission into LINDAT / CLARIAH-CZ DH Data → set input#edm_type = TEXT. The dependent field is
rendered in the DOM (label#label_27_array = "Pick the languages of the TEXT *") but its container keeps the
Bootstrap hide class (div.mb-2.d-none, ds-dynamic-form-control-container.d-none), so the field never appears and
the item cannot be filled in. On 7.6.5 selecting TEXT reveals it.

The backend is fine — the submission-form payload is byte-identical between 7.6.5 and 9.3 and does contain
"typeBind": ["TEXT", …], "typeBindField": "edm.type".

Analysis

Upstream type-bind supports exactly one global controlling field. LINDAT needs per-field control, expressed as
submit.type-bind.field = dc.type, dc.language.iso=>edm.type ("dc.language.iso is controlled by edm.type,
everything else by dc.type"). None of that was ported to the v9 branch:

what	dtq-dev (7.x)	dtq-dev-9-base
FormFieldModel.typeBindField	@autoserialize	absent → cerialize drops the REST value
field-parser relation id	per-field	always parserOptions.typeField
FormBuilderService.typeField	Map + TYPE_BIND_DEFAULT_KEY	single string
setTypeBindFieldFromConfig	parses A=>B	takes values[0], drops the rest
getTypeBindModel()	keyed by field	single model

Net effect: dc.language.iso gets a relation whose condition id is dc_type, matchesCondition reads the (empty)
dc.type model, 'TEXT' !== '', the hidden matcher fires and the container keeps d-none.

What this PR does

1. FormFieldModel.typeBindField — add the @autoserialize property so <type-bind field="edm.type"> survives
   deserialization.
2. FormBuilderService — typeFields: Map<string,string> (TYPE_BIND_DEFAULT_KEY → default model id, plus one
   entry per A=>B override) and typeBindModel: Map<string,DynamicFormControlModel> keyed by model id, with a
   typeBindModelUpdates subject. getTypeBindModel(ref?) takes an optional ref, so every existing zero-arg call
   site and mock keeps working. The registry is scoped to one submission: sections of the same submission share it
   (a controlling field may live in another section), but it is dropped when the submission changes so a model from
   the previously opened collection's form cannot answer lookups. Registration is re-run when
   submit.type-bind.field arrives, so an override that exists only in the property still resolves if the config
   lands after the form was parsed.
3. field-parser.getTypeBindFieldRef() — the relation condition id is now the controlling model id when
   <type-bind field="…"> is declared, and otherwise the field's own metadata name, resolved later against the
   submit.type-bind.field map (that property arrives over REST asynchronously, which is exactly why 7.x defers it).
4. DsDynamicTypeBindRelationService — pass the relation id through to getTypeBindModel(…), guard the two
   places that dereferenced the bind model unconditionally, and keep listening for registrations so the real
   controlling model is attached even when the field was temporarily bound to the default one. subscribeRelations
   returns a single owning Subscription: the caller spreads the returned array into its own, so a child created
   after that snapshot would otherwise never be torn down.
5. form-builder-service.mock — getTypeBindModelUpdates: EMPTY.

Deliberate deviations from a 1:1 7.x port (please read before diffing against dtq-dev)

• findById is not widened to accept string[]. 7.x had to, and even then it registered only one model per
  call (later matches overwrote the result). Instead modelFromConfiguration loops the candidate ids and calls the
  unchanged findById(id, rows) once per id — findById has ~40 call sites and its own spec coverage, and this also
  fixes the one-model-per-call limitation.
• row-parser and parserOptions are untouched. 7.x repurposes parserOptions.typeField inside the row loop to
  mean "this field's own metadata name", mutating a shared object that is handed to an Injector by reference. On v9
  typeField is a real parameter of RowParser.parse() with 10 assertions in its spec; FieldParser already knows
  the field id, so the same information is available one level down with zero changes there.
• controlModel.typeBindField is not carried on control models. Its only reader on 7.x is the guard below; the
  parser reads configData.typeBindField directly.
• Clause 2 of the 7.x getTypeBindModel guard is dropped. 7.x has
  if (isUndefined(candidate) || isNotUndefined(candidate.typeBindField)) return default;. The second clause is
  inverted with respect to its own comment, and the candidate here is always a controlling model whereas
  typeBindField marks a bound field — for the LINDAT config it is a no-op, and if it ever did fire it would fall
  back to dc_type, i.e. silently reproduce this very bug. Clause 1 (fall back to the default when the configured
  controlling field is not part of this collection's form) is kept.
• The A=>B parsing is order-independent, duplicate-safe and trimmed. The deployment currently reports
  ['dc.type', 'dc.type', 'dc.language.iso=>edm.type']; 7.x sets the default key on every iteration and would set it
  to '' if the => entry came first, and does not trim, so ", dc.language.iso=>…" would produce a key that never
  matches. Blank and malformed values ('', '=>', 'a=>b=>c', a null entry) are skipped with a warning.
• A self-referencing type bind warns and skips instead of throwing. Upstream throws
  FormControl X cannot depend on itself, but with per-field ids that comparison had gone inert; restoring it as a
  throw would mean one misconfigured <type-bind field="…"> takes down the whole submission section, and it can now
  be reached from the registration callback. One field degrades instead.

Tests

• form-builder.service.spec.ts — new sibling suite: duplicated property still yields dc_type; per-field model
  resolution (dc.language.iso → edm_type, direct model id, unmapped field → default, no-arg → default); fallback
  when the controlling model is not in the form; getTypeBindModelUpdates() emits every registration; and an
  end-to-end modelFromConfiguration test that reproduces the LINDAT shape and asserts
  typeBindRelations[0].when[0].id === 'edm_type'. The existing suite is unchanged (shared helpers were hoisted to
  module scope so a sibling describe can reuse them).
• form-builder.service.spec.ts — also: cross-submission isolation, a delayed configuration response (the
  property-only override still resolves), and blank/malformed values.
• ds-dynamic-type-bind-relation.service.spec.ts — 4 new specs: the relation id is passed to the form builder; a
  missing controlling model keeps a MATCH_VISIBLE field hidden instead of throwing; a late registration really
  re-evaluates the relation and stops when the caller unsubscribes; the same when the field was first attached to
  the default model.
• form-field.model.spec.ts (new) — a cerialize round-trip proving typeBindField survives deserialization, which
  is the root cause and was otherwise only exercised through hand-built object literals.
• onebox-field-parser.spec.ts — 2 new specs for the two getTypeBindFieldRef() branches.
• Verified locally: full npm run test:headless 5662/5662, ng lint clean, check-circ-deps clean.

Problems

Two things worth one minute against the live REST payload of the DH Data form, because they change what a reviewer
should expect:

1. Does that form contain a dc.type field at all? If not, the default type bind model is never registered and every
   ordinary type-bound field in it falls into the "no controlling model → stay hidden" branch. That is correct by
   config, not a regression.
2. Does the edm.type field itself carry a <type-bind>? If it did, the self-dependency guard would throw
   (FormControl edm_type cannot depend on itself) — it did on 7.x too, but with dotted ids the comparison had gone
   inert, so this PR restores the check.

Remaining known hole (narrow): if the controlling model is registered only after the bound field's container has
already attached to something, the field re-attaches via getTypeBindModelUpdates(); but a form section that is
destroyed and rebuilt between those two events re-runs the whole attach anyway, so the only uncovered case is a
controlling model that is never registered at all — which correctly leaves the field hidden.

Out of scope: the dead submission.typeBind.field key in src/config/default-app-config.ts (zero consumers on v9,
superseded by the REST-exposed submit.type-bind.field) — worth a separate cleanup PR.

Manual Testing (if applicable)

• Admin → new submission into LINDAT / CLARIAH-CZ DH Data → pick edm.type = TEXT → "Pick the languages of the
  TEXT" becomes visible; pick another type → it hides again.
• A vanilla collection whose fields are bound to dc.type still shows/hides correctly (no regression on the
  default path).

Copilot review

• Requested review from Copilot

[Copilot]

Pull request overview

This PR ports and extends DSpace submission “type-bind” behavior to support per-field controlling type fields (e.g. dc.language.iso=>edm.type) in the Angular UI, ensuring dependent fields show/hide correctly when their configured controlling field changes.

Changes:

• Adds typeBindField to FormFieldModel so <type-bind field="..."> survives REST deserialization.
• Refactors FormBuilderService to support multiple controlling fields via maps, plus a registration-updates observable.
• Updates field parsing and type-bind relation evaluation to resolve the correct controlling model and handle late availability; adds targeted unit tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/app/shared/mocks/form-builder-service.mock.ts	Extends mock to support getTypeBindModelUpdates() for updated type-bind relation logic.
src/app/shared/form/builder/parsers/onebox-field-parser.spec.ts	Adds tests for deriving the type-bind relation id from <type-bind field="..."> vs fallback behavior.
src/app/shared/form/builder/parsers/field-parser.ts	Uses a new resolver for type-bind controlling field refs (explicit <type-bind field> vs metadata-name fallback).
src/app/shared/form/builder/models/form-field.model.ts	Introduces autoserialized typeBindField property.
src/app/shared/form/builder/form-builder.service.ts	Replaces single type-bind field/model with maps + update stream; parses submit.type-bind.field including A=>B entries.
src/app/shared/form/builder/form-builder.service.spec.ts	Adds a dedicated suite covering per-field model resolution, fallbacks, updates emission, and end-to-end parsing behavior.
src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts	Passes relation ids through to model resolution; guards missing controlling models; supports late attachment.
src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.spec.ts	Adds tests for passing relation id, missing-model behavior, and late attachment.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/app/shared/form/builder/form-builder.service.ts:390

• modelFromConfiguration() appends every parsed rows array to typeBindParsedRows, but that cache is only cleared when the submission id changes. If the same submission section is re-parsed (e.g. section form rebuilds during updates), this can grow without bound and retain large model graphs for the lifetime of the submission, increasing memory usage and making later registerTypeBindModels re-walk more data than needed.

Consider caching rows only until submit.type-bind.field has been processed, then clearing and disabling further caching (e.g. a typeBindConfigLoaded flag set in setTypeBindFieldFromConfig(), and only push() when it’s false).

    if (hasValue(typeBindModel)) {
      this.setTypeBindModel(typeBindModel);
    } else {
      this.typeBindParsedRows.push(rows);
      this.registerTypeBindModels(this.getTypeBindModelIds(rawData), rows);
    }

src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts:117

• The comment above the hasNoValue(bindModel) guard implies the controlling model might be absent from the form, but FormBuilderService.getTypeBindModel() can fall back to the default model in that case. This makes the comment misleading for future maintenance: the undefined case here primarily means the controlling model hasn’t been registered yet (e.g. parsed later / config not arrived), not that it is permanently absent.

      // The controlling model may not be part of this form at all (e.g. `dc.language.iso=>edm.type`
      // is configured deployment-wide but this collection's form has no `edm.type` field), or it may
      // not be registered yet. Keep MATCH_VISIBLE fields hidden until it becomes available.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/app/shared/form/builder/form-builder.service.ts:450

• getTypeBindModelIds() adds field.typeBindField directly after an isNotEmpty check, but isNotEmpty(' ') is true in this codebase. If the REST/XML ever contains whitespace around/inside typeBindField, the collected id can become e.g. ' edm_type ' and the controlling model will never be registered (even though getTypeBindFieldRef() correctly trims). Trim (and re-check) before replacing dots so the pre-registration path is consistent with relation ids.

      (formRows || []).forEach((formRow: FormRowModel) => (formRow?.fields || []).forEach((field: FormFieldModel) => {
        if (isNotEmpty(field?.typeBindField)) {
          ids.add(field.typeBindField.replace(/\./g, '_'));
        }

[milanmajchrak]

Copilot suppressed comment on form-builder.service.ts:450 (review of ed4b0fa) applied in 79f94f1: getTypeBindModelIds() now trims typeBindField exactly like FieldParser.getTypeBindFieldRef() does, so a padded <type-bind field=" edm.type "> can no longer register the controlling model as ' edm_type ' while the relations point at edm_type. The end-to-end spec now uses a padded value, so it fails without the trim.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts:262

• Self-referencing type-bind relations are warned about and skipped in getRelatedFormModel(), but evaluateRelations() still evaluates the relation via matchesCondition(). If the controlling model is the same as the bound model, this can still hide/disable the control during the initial evaluation (and can make the field permanently unreachable). Consider explicitly skipping evaluation of self-bound relations here as well.

      this.dynamicMatchers.forEach((matcher) => {
        // Find the relation
        const relation = this.dynamicFormRelationService.findRelationByMatcher((model as any).typeBindRelations, matcher);
        // If the relation is defined, get matchesCondition result and pass it to the onChange event listener
        if (relation !== undefined) {
          const hasMatch = this.matchesCondition(relation, matcher);
          matcher.onChange(hasMatch, model, control, this.injector);
        }

[milanmajchrak]

Copilot suppressed comment on ds-dynamic-type-bind-relation.service.ts:262 (review of 79f94f1) applied in f25829c. You were right that skipping the self-reference in getRelatedFormModel() was not enough: evaluateRelations() still ran matchesCondition() against the field's own empty value on the initial pass, hid it, and - with nothing attached - never re-evaluated it, so the field became permanently unreachable. subscribeRelations() now detects the self-reference up front, warns once and returns without evaluating or attaching anything. Covered by a new spec asserting the field stays visible and gets no related models.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[milanmajchrak]

Correction to my earlier comment on the getTypeBindModelIds trim: the claim that the existing end-to-end spec fails without the trim was wrong. That suite configures dc.language.iso=>edm.type, so typeFields already contributes edm_type to the id set and the padded value was merely an extra miss. Fixed in c0891cf with a spec that configures only dc.type, making the padded <type-bind field=" edm.type "> the sole source of the controlling id — verified to fail with the trim removed and pass with it.

Same commit also fixes a defect in the self-reference guard added in f25829c: it used the runtime lookup, which falls back to the default model, so for a field whose own id is the default model id a relation whose real target had not been parsed yet looked like a misconfiguration and the field never picked up its controlling model. The guard now resolves from configuration only (FormBuilderService.resolveTypeBindModelId), and the transient case just skips the initial evaluation while still attaching later.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/app/shared/form/builder/form-builder.service.ts:182

• resolveTypeBindModelId() is declared to return string, but the current expression can return undefined if the default entry is missing (and it also relies on Map.get() which returns string | undefined). Add an explicit final fallback so callers always receive a concrete model id.

  resolveTypeBindModelId(typeBindFieldRef?: string): string {
    return this.typeFields.get(typeBindFieldRef) ?? typeBindFieldRef ?? this.typeFields.get(TYPE_BIND_DEFAULT_KEY);
  }

src/app/shared/form/builder/form-builder.service.ts:173

• getTypeBindModel() assumes TYPE_BIND_DEFAULT_KEY is always set, but Map.get() returns string | undefined, which can flow into Map.get(defaultModelId) and make the fallback path type-unsafe (and potentially undefined at runtime if the map is ever cleared/mutated). Make the fallback explicit so this method always queries with a concrete default id.

This issue also appears on line 180 of the same file.

  getTypeBindModel(typeBindFieldRef?: string): DynamicFormControlModel | undefined {
    const defaultModelId = this.typeFields.get(TYPE_BIND_DEFAULT_KEY);
    return this.typeBindModel.get(this.resolveTypeBindModelId(typeBindFieldRef)) ?? this.typeBindModel.get(defaultModelId);
  }

src/app/shared/form/builder/models/form-field.model.ts:141

• typeBindField is optional in the REST payload (and the new spec asserts it remains undefined when absent), but the model declares it as a required string. Mark it optional to reflect the actual deserialization behavior and prevent unsafe assumptions in callers.

  @autoserialize
  typeBindField: string;

src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-type-bind-relation.service.ts:82

• The inline comment references dependsOnItself(), but that method doesn’t exist in this service (the relevant guard is isConfiguredToDependOnItself). Updating the reference avoids confusion while debugging type-bind evaluation.

      if (bindModel.id === model.id) {
        // A misconfigured <type-bind field="..."> pointing at the field itself - see
        // dependsOnItself(), which stops the relation from being evaluated at all.
        return;
      }

[milanmajchrak]

Copilot suppressed comments on the review of c0891cf — 3 of 4 applied in 7312c5b:

• form-builder.service.ts:182 / :173 — resolveTypeBindModelId() and getTypeBindModel() now both go through a new private getDefaultTypeBindModelId(), which falls back to a TYPE_BIND_DEFAULT_MODEL_ID constant, so neither can hand a undefined model id to Map.get. The constant also replaces the four scattered 'dc_type' literals.
• ds-dynamic-type-bind-relation.service.ts:82 — comment updated to name isConfiguredToDependOnItself().
• form-field.model.ts:141 (typeBindField?: string) — not applied, on purpose. The observation is correct that the attribute is optional in the payload, but so are typeBind, scope, style, hints, languageCodes, value, visibility and every other @autoserialize member of FormFieldModel, and none of them is declared optional. Marking exactly one of them ? would be inconsistent without changing anything: tsconfig.json sets strictNullChecks: false, so the annotation is not enforced either way. The new form-field.model.spec.ts pins the actual runtime contract (typeBindField is undefined when the attribute is absent), which is what callers rely on.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/app/shared/form/builder/form-builder.service.ts:733

• getTypeField() is declared to return string, but Map.get() can return undefined, so the current return this.typeFields.get(TYPE_BIND_DEFAULT_KEY); violates the signature and can propagate an unexpected undefined to call sites. Use the existing getDefaultTypeBindModelId() helper (which guarantees a fallback) for the return value.

    return this.typeFields.get(TYPE_BIND_DEFAULT_KEY);

[milanmajchrak]

Copilot suppressed comment on form-builder.service.ts:733 applied in 9d9c747: getTypeField() was the one remaining place returning a bare Map.get for a method declared string; it now goes through getDefaultTypeBindModelId() like the other two.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/app/shared/mocks/form-builder-service.mock.ts:50

• In the mock, resolveTypeBindModelId is currently configured to return undefined. The real FormBuilderService.resolveTypeBindModelId() returns a string model id, and callers (e.g. DsDynamicTypeBindRelationService.isConfiguredToDependOnItself) may rely on that. Returning undefined in the shared mock can lead to test/runtime errors if any code starts using the return value as a string.

    getTypeBindModelUpdates: EMPTY,
    resolveTypeBindModelId: undefined,

src/app/shared/form/builder/models/form-field.model.ts:141

• typeBindField is optional in the REST payload (and the new spec asserts it can be undefined), but it’s typed as a required string. This makes it easy for future call sites to dereference it unsafely (TypeScript will assume it’s always present). Mark it optional to match runtime behavior and to keep the ?.trim() usage in parsers type-correct.

  @autoserialize
  typeBindField: string;

[milanmajchrak]

Both Copilot suppressed comments from the 9d9c747 review applied in 4cde913:

• form-builder-service.mock.ts:50 — the mock now mirrors the real resolveTypeBindModelId() for a reference the type field map does not remap, instead of returning undefined.
• form-field.model.ts:141 — changing my earlier answer: typeBindField is now typeBindField?: string. I previously argued for consistency with the other non-optional @autoserialize members, but this is the one member the code actually null-checks (?.trim() in both FieldParser.getTypeBindFieldRef() and FormBuilderService.getTypeBindModelIds(), plus a spec asserting it stays undefined when the attribute is absent), so accuracy beats consistency here.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/app/shared/mocks/form-builder-service.mock.ts:55

• The mock’s resolveTypeBindModelId callFake takes typeBindFieldRef: string, but the real method accepts an optional ref (typeBindFieldRef?: string). Keeping the mock signature aligned avoids type drift and matches the existing ?? fallback (which only makes sense for undefined/null).

  // mirror the real implementation for a reference that the type field map does not remap
  formBuilderService.resolveTypeBindModelId.and.callFake((typeBindFieldRef: string) => typeBindFieldRef ?? 'dc_type');

[milanmajchrak]

Copilot suppressed comment on form-builder-service.mock.ts:55 applied in b86c0d6: the fake now declares typeBindFieldRef?: string, matching the real optional parameter that the ?? fallback exists for.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.