aglinxinyuan opened a new pull request, #7827:
URL: https://github.com/apache/texera/pull/7827
### What changes were proposed in this PR?
`getAttrNames` decided whether an autofilled property was required by
consulting the **root** schema, while `DynamicSchemaService.mutateProperty`
recurses into nested `properties` / `items` / `definitions`. Its callbacks only
received `(propertyName, propertyValue)`, so the owning object schema was never
available — and a property nested inside a list element was tested against a
`required` array that does not describe it.
**This has a real production consequence, not just a synthetic one.**
`SortOpDesc` + `SortCriteriaUnit`: the root requires `attributes`, and each
element declares `@JsonProperty(value = "attribute", required = true)` with
`@AutofillAttributeName`. Root `required` is `["attributes"]`, so `"attribute"`
was not found there and an empty option was appended — offering a blank sort
key that `SortOpDesc.generatePythonCode` then rejects at code-generation time
(`require(attributes.forall(c => c.attributeName != null &&
c.attributeName.trim.nonEmpty))`) instead of at form validation.
The mirror case is the `aggregations[].attribute` shape, where `attribute`
is legitimately optional per element (COUNT(*) needs no column): a root-level
required property sharing that name would silently strip its empty option.
### The fix
Two files, minimally:
- `dynamic-schema.service.ts` — `mutationFunc` gains a third parameter: the
schema owning the `properties`/`definitions` map the property was found in.
Inside `mutateObjectProperty` that is just the enclosing `jsonSchema`, already
in scope. `matchFunc` is untouched, since it only inspects `autofill`.
- `workflow-compiling.service.ts` — `getAttrNames(attrName, v,
owningSchema)` now tests `owningSchema.required?.includes(attrName)`; both
`setOperatorInputAttrs` call sites forward it.
**On the signature change:** this does widen `mutateProperty`'s public
callback type, but additively — TypeScript permits callbacks that declare fewer
parameters, so both `restoreOperatorInputAttrs` call sites and all six spec
call sites compile untouched. The alternative, pre-collecting required-ness in
a second walk inside `setOperatorInputAttrs`, cannot work: required-ness would
still be keyed by name, and the collision case is precisely where name-based
lookup is ambiguous. Plumbing the owner is the only correct fix.
`mutateProperty` is not otherwise refactored.
### Failing before, passing after — both directions
| | production reverted | with fix |
|---|---|---|
| `workflow-compiling.service.spec.ts` | **3 failed, 47 passed** | **50
passed** |
| with `dynamic-schema.service.spec.ts` | — | **62 passed** |
The three before-state failures are the three cases that matter:
| case | actual | expected |
|---|---|---|
| nested required via `items` | `['col_a','col_b','']` | `['col_a','col_b']`
|
| nested optional colliding with a root-level required name |
`['col_a','col_b']` | `['col_a','col_b','']` |
| nested required via `$ref` + `definitions` | `['col_a','col_b','']` |
`['col_a','col_b']` |
The third is worth calling out: `definitions` is the shape the metadata
generator actually emits
(`OperatorMetadataGenerator.texeraSchemaGeneratorConfig` on
mbknor-jackson-jsonSchema places nested types there), so it would have
regressed independently of the `items` path.
### Root-level behaviour is unchanged
#7727's root-level tests are the regression net here and all pass: `injects
the input attribute names as an enum on an optional attributeName property`,
`does not append an empty option for required properties`, `appends the
property's string default instead of "" for optional properties`, and `includes
additionalEnumValue before the optional empty option`.
### Verification
- `workflow-compiling.service.spec.ts` 50/50;
`dynamic-schema.service.spec.ts` 12/12 (direct `mutateProperty` coverage,
including the `definitions` recursion).
- Combined with `operator-property-edit-frame.component.spec.ts`, the only
other autofill consumer: **258 passed | 1 skipped**.
- Because the builder typechecks every spec on each run, those green runs
also confirm no typecheck drift from the signature change.
- `yarn format:ci` exits 0; the spec diff is 94 insertions / 0 deletions, so
no untouched lines were reformatted. No `junit.xml` left behind.
### Any related issues, documentation, discussions?
Closes #7826
### How was this PR tested?
```
npx ng test --watch=false --include="**/workflow-compiling.service.spec.ts"
--include="**/dynamic-schema.service.spec.ts"
```
```
Test Files 2 passed (2)
Tests 62 passed (62)
```
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
