Skip to content

fix(QTDI-2489): Apply default value before required validation for absent keys#1254

Open
thboileau wants to merge 8 commits into
masterfrom
copilot/QTDI-2489_required_default_value_fix
Open

fix(QTDI-2489): Apply default value before required validation for absent keys#1254
thboileau wants to merge 8 commits into
masterfrom
copilot/QTDI-2489_required_default_value_fix

Conversation

@thboileau

@thboileau thboileau commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Requirements

  • Any code change adding any logic MUST be tested through a unit test executed with the default build
  • Any API addition MUST be done with a documentation update if relevant

Why this PR is needed?

Fixes QTDI-2489.

When a connector author adds a new @Required + @DefaultValue field to an existing connector, the TCK framework was throwing IllegalArgumentException because @Required validation fired on the raw config map before @DefaultValue was applied to absent keys. Existing serialized configurations (missing the new key) caused a false-positive validation failure.

Root cause: In ReflectionService.PayloadValidator.onParameter, the required-field check compared the raw value against JsonValue.NULL without first checking whether a default was declared. For newly added fields absent from a stored config, PayloadMapper delivers JsonValue.NULL — the validation fired even though the declared default would have been applied moments later.

What does this PR add (design/code thoughts)?

One-line guard in PayloadValidator.onParameter:

// BEFORE
if (Boolean.parseBoolean(meta.getMetadata().get("tcomp::validation::required"))
        && value == JsonValue.NULL) {
    errors.add(MESSAGES.required(meta.getPath()));
}

// AFTER
if (Boolean.parseBoolean(meta.getMetadata().get("tcomp::validation::required"))
        && value == JsonValue.NULL
        && meta.getMetadata().get("tcomp::ui::defaultvalue::value") == null) {
    errors.add(MESSAGES.required(meta.getPath()));
}

The tcomp::ui::defaultvalue::value metadata key is set by UiParameterEnricher when @DefaultValue is present. When it is non-null, we suppress the required-field error — the default will be applied later in the object factory chain. When it is absent, the existing behaviour is preserved: a genuinely missing required field still throws.

Tests added (5 new tests in ReflectionServiceTest):

Test What it verifies
validationRequiredWithDefaultValueMissingKeyPasses Missing key + @DefaultValue → no exception
validationRequiredWithDefaultValueResolvedValue Missing key + @DefaultValue("DEFAULT") → resolved value equals "DEFAULT"
validationRequiredNoDefaultValueMissingKeyFails Missing key + no @DefaultValueIllegalArgumentException (regression)
validationRequiredWithEmptyDefaultValuePasses Missing key + @DefaultValue("") → no exception
validationRequiredMultipleAbsentDefaultsPasses Multiple missing keys all with defaults → no exception

All 50 tests in ReflectionServiceTest pass (45 pre-existing + 5 new).

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

…sent keys

When a new @required + @DefaultValue field is added to an existing
connector, the framework was throwing IllegalArgumentException because
@required validation fired on the raw config map before @DefaultValue
was applied to absent keys.

Add a guard in PayloadValidator.onParameter to skip the required-field
error when a default value is declared (tcomp::ui::defaultvalue::value
metadata present). Existing @Required-without-default behaviour is
unchanged.

Co-Authored-By: GitHub Copilot <copilot@github.com>
@thboileau

Copy link
Copy Markdown
Contributor Author

🔍 Scope & Design Review — QTDI-2489 (Round 1)

Verdict: APPROVED — zero Blocker/Major findings.

Finding Severity Detail
Metadata key tcomp::ui::defaultvalue::value None Verified from UiParameterEnricher and StreamingLongParamBuilder
Explicit-null vs absent-key None PayloadMapper delivers JsonValue.NULL only for absent map keys; explicitly null entries don't occur in Map<String,String> configs
AC1 — default suppresses required error None Guard correctly fires when required AND absent AND defaultvalue declared
AC2 — present key path unchanged None value != JsonValue.NULL bypasses guard entirely
AC3 — genuinely missing required field still fails None @DefaultValue absent → get(...) == null is true → error fires as before
AC4 — no migration handler required None Fix confined to validation layer; no connector change needed
@DefaultValue on complex types Info Annotation is primitive-only per Javadoc; TCK maven plugin enforces at build time — no runtime risk
Test coverage None 5 new tests cover all ACs plus empty-string and multi-absent edge cases

Fixes applied in Round 1: None.


Review performed by AI (GitHub Copilot). Human reviewer should verify the guard condition and test coverage.

@thboileau

Copy link
Copy Markdown
Contributor Author

✅ Compliance Check — QTDI-2489

Verdict: PASS — no compliance violations.

Files reviewed

  1. component-runtime-manager/src/main/java/org/talend/sdk/component/runtime/manager/reflect/ReflectionService.java
  2. component-runtime-manager/src/test/java/org/talend/sdk/component/runtime/manager/ReflectionServiceTest.java

Findings

Rule File Severity Status
Spotless formatting Both PASS — auto-applied before commit
Checkstyle Both PASS — 0 violations
No wildcard imports Test PASS — specific import added
JUnit 5 — no public on @Test Test PASS — default visibility on all 5 new methods
Scope discipline Both PASS — all changes traceable to approved plan
Test level (UT) Test PASS — pure framework logic, UT correct
Test scope coverage Test PASS — all 5 items from test scope covered
No deprecated API usage Both PASS — no deprecated APIs called
No opportunistic refactoring Both PASS — minimal required change only

Severity summary

Severity Count
Critical fixed 0
Warnings 0
Info 0

Replace anonymous HashMap double-brace initializers with Map.of()
calls (S3599) and add assertDoesNotThrow() to test methods that
lacked assertions (S2699).
Extract isRequiredAndMissingWithoutDefault helper in PayloadValidator to
reduce onParameter cognitive complexity (S3776).

Replace three identical assertDoesNotThrow test methods with a single
@ParameterizedTest using @MethodSource with three distinct input maps
(S5785, S4144 x2). Extract Map.of() outside assertThrows lambda in
validationRequiredNoDefaultValueMissingKeyFails to avoid multiple
potentially-throwing invocations in one lambda (S5778).
&& ((JsonNumber) value).doubleValue() < bound) {
errors.add(MESSAGES.min(meta.getPath(), bound, ((JsonNumber) value).doubleValue()));
}
validateRuleMin(meta, value, metadata);

@thboileau thboileau Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following Sonar comment: reduce method complexity. Each block has been extracted as a method

Sonar R4: Replace 10 individual test methods with 2 @ParameterizedTest methods
and remove 2 S4144 duplicate methods in ReflectionServiceTest.

- S5785: replace 6 Ko + 4 Ok individual tests with @ParameterizedTest + @MethodSource
- S4144: remove validationMaxNumberOk and validationMaxStringLengthOk (identical to Min variants)
- Net: 12 @test removed, 2 @ParameterizedTest added (10 invocations preserved)
- Build: component-runtime-manager 64/64 GREEN
@sonar-rnd

sonar-rnd Bot commented Jul 16, 2026

Copy link
Copy Markdown

Failed Quality Gate failed

  • 0.00% Coverage on New Code (is less than 80.00%)

Project ID: org.talend.sdk.component:component-runtime

View in SonarQube

@thboileau

Copy link
Copy Markdown
Contributor Author

Issue with Sonar -> https://qlik-dev.atlassian.net/browse/QTDI-2489?focusedCommentId=1172248
let's wait for the next release of TCK (should reset the base reference of Sonar), and rerun Sonar CI then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant