fix(QTDI-2489): Apply default value before required validation for absent keys#1254
Open
thboileau wants to merge 8 commits into
Open
fix(QTDI-2489): Apply default value before required validation for absent keys#1254thboileau wants to merge 8 commits into
thboileau wants to merge 8 commits into
Conversation
…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>
Contributor
Author
🔍 Scope & Design Review — QTDI-2489 (Round 1)Verdict: APPROVED — zero Blocker/Major findings.
Fixes applied in Round 1: None. Review performed by AI (GitHub Copilot). Human reviewer should verify the guard condition and test coverage. |
Contributor
Author
✅ Compliance Check — QTDI-2489Verdict: PASS — no compliance violations. Files reviewed
Findings
Severity summary
|
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).
thboileau
commented
Jul 15, 2026
| && ((JsonNumber) value).doubleValue() < bound) { | ||
| errors.add(MESSAGES.min(meta.getPath(), bound, ((JsonNumber) value).doubleValue())); | ||
| } | ||
| validateRuleMin(meta, value, metadata); |
Contributor
Author
There was a problem hiding this comment.
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
Contributor
Author
|
Issue with Sonar -> https://qlik-dev.atlassian.net/browse/QTDI-2489?focusedCommentId=1172248 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Requirements
Why this PR is needed?
Fixes QTDI-2489.
When a connector author adds a new
@Required + @DefaultValuefield to an existing connector, the TCK framework was throwingIllegalArgumentExceptionbecause@Requiredvalidation fired on the raw config map before@DefaultValuewas 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 againstJsonValue.NULLwithout first checking whether a default was declared. For newly added fields absent from a stored config,PayloadMapperdeliversJsonValue.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:The
tcomp::ui::defaultvalue::valuemetadata key is set byUiParameterEnricherwhen@DefaultValueis 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):validationRequiredWithDefaultValueMissingKeyPasses@DefaultValue→ no exceptionvalidationRequiredWithDefaultValueResolvedValue@DefaultValue("DEFAULT")→ resolved value equals"DEFAULT"validationRequiredNoDefaultValueMissingKeyFails@DefaultValue→IllegalArgumentException(regression)validationRequiredWithEmptyDefaultValuePasses@DefaultValue("")→ no exceptionvalidationRequiredMultipleAbsentDefaultsPassesAll 50 tests in
ReflectionServiceTestpass (45 pre-existing + 5 new).AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code