Skip to content

Fix config deserialization for records#5458

Open
jlaur wants to merge 10 commits intoopenhab:mainfrom
jlaur:config-parser-record
Open

Fix config deserialization for records#5458
jlaur wants to merge 10 commits intoopenhab:mainfrom
jlaur:config-parser-record

Conversation

@jlaur
Copy link
Copy Markdown
Contributor

@jlaur jlaur commented Mar 29, 2026

This addresses several issues with the record support:

  • Map configuration properties to record components by name and order
  • Missing or invalid values for primitive record components now fall back to Java default values (0, false, etc.)
  • Private records are now supported.
  • Nested Lists and Sets are now ignored rather than causing ClassCastException.
  • Enhanced logging to provide better diagnostics similar to existing class support.

General:

  • Let valueAs consistently return null rather than throwing RuntimeExceptions.

See openhab/openhab-addons#20469 (comment)

Regression of #4508

jlaur added 3 commits March 28, 2026 23:25
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur requested a review from a team as a code owner March 29, 2026 12:12
@jlaur jlaur force-pushed the config-parser-record branch from 8b5a53e to 24d691b Compare March 29, 2026 12:27
@wborn wborn requested a review from Copilot March 29, 2026 13:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves ConfigParser’s handling of Java record configuration DTOs to address regressions introduced by earlier record support work, focusing on safer/clearer deserialization and better diagnostics.

Changes:

  • Split configuration deserialization into dedicated paths for classes vs. records, mapping record components by name and declared order.
  • Add primitive-default handling for missing record component values and make collection conversion more robust (e.g., skip unsupported nested generics).
  • Extend test coverage for record deserialization scenarios (missing fields, out-of-order properties, primitive defaults, collection conversion, nested lists).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
bundles/org.openhab.core.config.core/src/main/java/org/openhab/core/config/core/ConfigParser.java Refactors config-to-DTO mapping with dedicated record construction, primitive defaults, collection conversion refactor, and enhanced logging.
bundles/org.openhab.core.config.core/src/test/java/org/openhab/core/config/core/ConfigParserTest.java Adds unit tests covering new/updated record deserialization behaviors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jlaur jlaur marked this pull request as draft March 29, 2026 14:35
@jlaur jlaur force-pushed the config-parser-record branch from 24d691b to 1723be0 Compare March 29, 2026 15:44
@jlaur jlaur marked this pull request as ready for review March 29, 2026 16:29
@wborn wborn requested a review from Copilot March 29, 2026 16:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +316 to 320
} else if (Set.class.isAssignableFrom(typeClass)) {
result = Set.of(value);
} else if (Collection.class.isAssignableFrom(typeClass)) {
result = List.of(value);
}
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

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

In valueAs, the Set/Collection wrapping logic is currently inside the value instanceof String conversion block. This means non-String scalar inputs (e.g., Integer -> Set.class) will no longer be wrapped into a single-element collection and will now fail conversion (returning null). Move the Set/Collection handling back to the top-level (outside the String-only branch) so it applies to any input type.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@jlaur jlaur Mar 29, 2026

Choose a reason for hiding this comment

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

This was not changed by this PR. I'm not sure exactly what is meant here, but I'm also not sure it should be within the scope of this PR to change this behavior.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 87 to +91
* @param configurationClass The configuration holder class. An instance of this will be created so make sure that
* a default constructor is available.
* @return The configuration holder object. All fields that matched a configuration option are set. If a required
* field is not set, null is returned.
* field is not set, null is returned. For record classes, missing primitives are automatically set to
* their default value.
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The Javadoc for configurationAs says "If a required field is not set, null is returned", but neither constructClass nor constructRecord currently enforce any concept of "required" fields/components (missing entries are skipped/defaulted and an instance is still returned). Either implement required-field handling (and define how it's declared, e.g., an annotation) or adjust the Javadoc to match the actual behavior.

Copilot uses AI. Check for mistakes.
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.

I'm not sure what was meant by this. If I read the Javadoc correctly (which I didn't change), "All fields that matched a configuration option are set." should be sufficent, i.e., unmatched configuration options are not set? In this case the sentence "If a required field is not set, null is returned." can simply be removed.

What I'm confused about is "null is returned". That could indicate that the entire method returns null, but this isn't related to "required fields", but rather something like not being able to call the constructor to create the object (through reflection). Please advise.

Comment on lines +326 to +329
} else if (Set.class.isAssignableFrom(typeClass)) {
result = Set.of(value);
} else if (Collection.class.isAssignableFrom(typeClass)) {
result = List.of(value);
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

In valueAs, the singleton wrapping for Set/Collection targets is now only applied when the input is a String (inside the value instanceof String branch). This changes behavior for non-String scalar inputs (e.g., 1 -> Set.class), which will now fail conversion and return null. Move the Set/Collection wrapping logic outside the String-only branch so it applies consistently regardless of input type.

Suggested change
} else if (Set.class.isAssignableFrom(typeClass)) {
result = Set.of(value);
} else if (Collection.class.isAssignableFrom(typeClass)) {
result = List.of(value);
}
}
// Apply singleton wrapping for Set/Collection targets for non-collection scalar inputs,
// regardless of the original scalar type (String, Number, etc.).
if (result != null && !typeClass.isAssignableFrom(result.getClass())
&& !(value instanceof Collection<?>)) {
if (Set.class.isAssignableFrom(typeClass)) {
result = Set.of(result);
} else if (Collection.class.isAssignableFrom(typeClass)) {
result = List.of(result);

Copilot uses AI. Check for mistakes.
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.

See #5458 (comment)

I would prefer human intervention here before doing further changes in this area.

jlaur added 7 commits April 1, 2026 18:55
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
…ptions

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the config-parser-record branch from 5c1821e to dc12abc Compare April 1, 2026 16:57
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.

2 participants