[REST] API /file-format/parse: do not extend thing/channel config by lolodomo · Pull Request #5295 · openhab/openhab-core

lolodomo · 2026-01-21T18:43:39Z

The /file-format/parse API does no more extend the thing and channel configuration with parameters having a default value. The output will contain hte thing/channel configuration parameters provided in the input.

It is fixed for YAML and DSL formats.

Related to #5238

Unit tests added for YAML models containing Thing
Done for usual models populating the thing registry and for isolated models used by hte /file-format/parse REST API.

These tests are also a way to test the YamlThingProvider.

Copilot

Pull request overview

Adjusts YAML and DSL Thing parsing so isolated models (used by /file-format/parse) do not get their Thing/Channel configurations implicitly extended with default-valued parameters.

Changes:

Propagates “isolated model” context through Thing creation/merge to preserve only user-provided config.
Skips applying default configuration from config descriptions for channels when parsing isolated models.
Uses defensive Configuration copies when calling ThingHandlerFactory.createThing(...) to avoid side effects.

Reviewed changes

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

File	Description
bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/things/YamlThingProvider.java	Adds isolated-model-aware Thing/channel creation and merge behavior (and adjusts handler-factory refresh logic accordingly).
bundles/org.openhab.core.model.thing/src/org/openhab/core/model/thing/internal/GenericThingProvider.xtend	Adds isolated-model-aware merge/config handling and prevents applying default channel config for isolated models.

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

Copilot · 2026-01-22T13:07:41Z

+        thing.getChannels().forEach(channel -> {
+            for (String name : channel.getConfiguration().keySet()) {
+                Object value = channel.getConfiguration().get(name);
+                logger.info("initial channel {}: {} => {} ({})", channel.getUID().getId(), name, value,
+                        value.getClass().getSimpleName());
+            }
+        });


The added INFO logs for channel configuration values have the same issues: they may expose secrets and value.getClass() will NPE if a channel config contains a null value. Please remove or make them null-safe and ensure sensitive values are not logged.

Copilot · 2026-01-22T13:07:41Z

+                    logger.info("before merge thing: {} => {} ({})", name, value, value.getClass().getSimpleName());
+                }
+                thingFromHandler.getChannels().forEach(channel -> {
+                    for (String name : channel.getConfiguration().keySet()) {
+                        Object value = channel.getConfiguration().get(name);
+                        logger.info("before merge channel {}: {} => {} ({})", channel.getUID().getId(), name, value,
+                                value.getClass().getSimpleName());
+                    }
+                });
+                mergeThing(thingFromHandler, thing, isolatedModel);
+                for (String name : thingFromHandler.getConfiguration().keySet()) {
+                    Object value = thingFromHandler.getConfiguration().get(name);
+                    logger.info("after merge thing: {} => {} ({})", name, value, value.getClass().getSimpleName());
+                }
+                thingFromHandler.getChannels().forEach(channel -> {
+                    for (String name : channel.getConfiguration().keySet()) {
+                        Object value = channel.getConfiguration().get(name);
+                        logger.info("after merge channel {}: {} => {} ({})", channel.getUID().getId(), name, value,
+                                value.getClass().getSimpleName());


The additional INFO logging around the merge (before merge... / after merge...) prints all configuration entries and types. This is high-volume and risks leaking sensitive config into logs. Also, the same null-value NPE risk exists with value.getClass(). Please remove these logs or make them trace-level and redact values.

Suggested change

logger.info("before merge thing: {} => {} ({})", name, value, value.getClass().getSimpleName());

}

thingFromHandler.getChannels().forEach(channel -> {

for (String name : channel.getConfiguration().keySet()) {

Object value = channel.getConfiguration().get(name);

logger.info("before merge channel {}: {} => {} ({})", channel.getUID().getId(), name, value,

value.getClass().getSimpleName());

}

});

mergeThing(thingFromHandler, thing, isolatedModel);

for (String name : thingFromHandler.getConfiguration().keySet()) {

Object value = thingFromHandler.getConfiguration().get(name);

logger.info("after merge thing: {} => {} ({})", name, value, value.getClass().getSimpleName());

}

thingFromHandler.getChannels().forEach(channel -> {

for (String name : channel.getConfiguration().keySet()) {

Object value = channel.getConfiguration().get(name);

logger.info("after merge channel {}: {} => {} ({})", channel.getUID().getId(), name, value,

value.getClass().getSimpleName());

if (logger.isTraceEnabled()) {

String valueType = value != null ? value.getClass().getSimpleName() : "null";

logger.trace("before merge thing config: {} (type={})", name, valueType);

}

}

thingFromHandler.getChannels().forEach(channel -> {

for (String name : channel.getConfiguration().keySet()) {

Object value = channel.getConfiguration().get(name);

if (logger.isTraceEnabled()) {

String valueType = value != null ? value.getClass().getSimpleName() : "null";

logger.trace("before merge channel config {}: {} (type={})",

channel.getUID().getId(), name, valueType);

}

}

});

mergeThing(thingFromHandler, thing, isolatedModel);

for (String name : thingFromHandler.getConfiguration().keySet()) {

Object value = thingFromHandler.getConfiguration().get(name);

if (logger.isTraceEnabled()) {

String valueType = value != null ? value.getClass().getSimpleName() : "null";

logger.trace("after merge thing config: {} (type={})", name, valueType);

}

}

thingFromHandler.getChannels().forEach(channel -> {

for (String name : channel.getConfiguration().keySet()) {

Object value = channel.getConfiguration().get(name);

if (logger.isTraceEnabled()) {

String valueType = value != null ? value.getClass().getSimpleName() : "null";

logger.trace("after merge channel config {}: {} (type={})",

channel.getUID().getId(), name, valueType);

}

Copilot · 2026-01-22T13:07:42Z

+                for (String name : thingFromHandler.getConfiguration().keySet()) {
+                    Object value = thingFromHandler.getConfiguration().get(name);
+                    logger.info("after merge thing: {} => {} ({})", name, value, value.getClass().getSimpleName());
+                }
+                thingFromHandler.getChannels().forEach(channel -> {
+                    for (String name : channel.getConfiguration().keySet()) {
+                        Object value = channel.getConfiguration().get(name);
+                        logger.info("after merge channel {}: {} => {} ({})", channel.getUID().getId(), name, value,
+                                value.getClass().getSimpleName());
+                    }
+                });


Same concern for the after merge INFO logs: they expose full configuration content (potential secrets), add significant log noise, and will NPE when a config value is null. Please remove them or convert to trace-level with redaction and null-safe type handling.

Copilot · 2026-01-22T13:07:42Z

+        for (String name : thing.getConfiguration().keySet()) {
+            Object value = thing.getConfiguration().get(name);
+            logger.info("initial thing: {} => {} ({})", name, value, value.getClass().getSimpleName());
+        }
+        thing.getChannels().forEach(channel -> {
+            for (String name : channel.getConfiguration().keySet()) {
+                Object value = channel.getConfiguration().get(name);
+                logger.info("initial channel {}: {} => {} ({})", channel.getUID().getId(), name, value,
+                        value.getClass().getSimpleName());
+            }
+        });


The new INFO-level logging of thing configuration values can leak sensitive data (e.g., passwords/tokens) into logs, and value.getClass() will throw NPE when a config value is null (Configuration allows null values). Please remove these logs or downgrade to TRACE/DEBUG with null-safe formatting and without printing raw config values.

Suggested change

for (String name : thing.getConfiguration().keySet()) {

Object value = thing.getConfiguration().get(name);

logger.info("initial thing: {} => {} ({})", name, value, value.getClass().getSimpleName());

}

thing.getChannels().forEach(channel -> {

for (String name : channel.getConfiguration().keySet()) {

Object value = channel.getConfiguration().get(name);

logger.info("initial channel {}: {} => {} ({})", channel.getUID().getId(), name, value,

value.getClass().getSimpleName());

}

});

if (logger.isDebugEnabled()) {

for (String name : thing.getConfiguration().keySet()) {

Object value = thing.getConfiguration().get(name);

String valueType = (value != null) ? value.getClass().getSimpleName() : "null";

logger.debug("Initial thing configuration parameter '{}' of thing '{}' has type {}", name,

thing.getUID(), valueType);

}

thing.getChannels().forEach(channel -> {

for (String name : channel.getConfiguration().keySet()) {

Object value = channel.getConfiguration().get(name);

String valueType = (value != null) ? value.getClass().getSimpleName() : "null";

logger.debug("Initial channel configuration parameter '{}' of channel '{}' has type {}", name,

channel.getUID(), valueType);

}

});

}

lolodomo · 2026-01-22T15:15:28Z

This is a draft! I plan of course to remove all the new INFO logs. They are here just for my tests.

The /file-format/parse API does no more extend the thing and channel configuration with parameters having a default value. The output will contain hte thing/channel configuration parameters provided in the input. It is fixed for YAML and DSL formats. Related to openhab#5238 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2026-01-23T07:47:48Z

@wborn : please restart a copilot review, the PR is now fully tested and ready for a review.

Copilot

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.

Copilot · 2026-01-23T10:21:47Z

+    def dispatch void merge(Configuration target, Configuration source, boolean keepSourceConfig) {
+        if (keepSourceConfig) {
+            target.setProperties(Map.of())
+        }
        source.keySet.forEach [


Please add/extend unit tests around the isolated-model parsing flow to cover the new keepSourceConfig merge behavior (clearing the target config before copying source keys). This should verify that /file-format/parse for DSL does not materialize default-valued config parameters that were omitted in the input.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2026-01-23T20:25:00Z

I will try to add unit tests at least for YAML.

Done for usual models populating the thing registry and for isolated models used by hte /file-format/parse REST API. These tests are also a way to test the YamlThingOrovider. Signed-off-by: Laurent Garnier <lg.hc@free.fr>

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2026-01-25T09:29:31Z

Note that build failure in integration tests is not linked to my PR.

lolodomo · 2026-01-25T10:34:36Z

Note that build failure in integration tests is not linked to my PR.

@holgerfriedrich : can you please retrigger a build, it looks like the build is working for few other PRs. The integration test is probably unstable.

holgerfriedrich · 2026-01-25T12:06:07Z

@lolodomo yes, the integration tests are running again and DiscoveryServiceRegistryOSGiTest is unstable. I have asked the AI to create a fix, but it is not yet merged (#5307).

I have retriggered the build, but no luck yet. You should be able to re-trigger (all builds) as well by adding the label "rebuild". Or is setting labels restricted to core-maintainers?

lolodomo · 2026-01-25T12:11:59Z

You should be able to re-trigger (all builds) as well by adding the label "rebuild". Or is setting labels restricted to core-maintainers?

Not possible to change a label if you are not maintainer.

Nadahar · 2026-01-25T17:45:05Z

Not possible to change a label if you are not maintainer.

That has something to do with the configuration of the repo. In the addons repo I can change labels after I became a "member" of the GitHub organization, in Core I cannot. So, that must be possible to change if desirable also for Core.

edit: Maybe it depends on some group membership that gives access to labels in addons but not in core.

lolodomo · 2026-01-25T18:02:20Z

@openhab/core-maintainers : please review that PR. It is confirmed to be an important fix for our current discussion about editing in Main UI code tab.
Backporti to 5.1 branch is wished.

jimtng · 2026-01-26T06:55:52Z

I haven't looked at the code, but I did test this PR and it solved the issue, i.e. the defaults are no longer being generated, which is what we want.

holgerfriedrich

LGTM, let's give it a try!

lolodomo · 2026-01-28T07:52:48Z

@holgerfriedrich : we need a backporting to the 5.1 branch. I hope it will work without any problem.

holgerfriedrich · 2026-01-28T07:57:54Z

@lolodomo I will have a look later today.

@kaikreuzer maybe we should shift our 5.1 patch release to the weekend after this has been properly done.

holgerfriedrich · 2026-01-29T07:46:09Z

@lolodomo It took a bit more effort to backport and get the tests working again:

commit 628b7bbfbc4aed52c2ead68d2fe489d9dd2dab64 (HEAD -> 5.1.x)
Author: Holger Friedrich <mail@holger-friedrich.de>
Date:   Thu Jan 29 08:21:46 2026 +0100

    Adapt test to 5.1

    Yaml provider returned BigDecimal instead of String before #5280.

    Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

commit 7b05252e7f9aba34d71607c223e6191245e8eab9
Author: lolodomo <lg.hc@free.fr>
Date:   Tue Jan 27 16:04:54 2026 +0100

    [REST] API /file-format/parse: do not extend thing/channel config (#5295)

    * [REST] API /file-format/parse: do not extend thing/channel config

    The /file-format/parse API does no more extend the thing and channel configuration with parameters having a default value. The output will contain hte thing/channel configuration parameters provided in the input.

    It is fixed for YAML and DSL formats.

    Unit tests added for YAML models containing Thing


    Signed-off-by: Laurent Garnier <lg.hc@free.fr>

commit 42eba09a446635920401bb680ee9009e5cfd8371
Author: jimtng <2554958+jimtng@users.noreply.github.com>
Date:   Fri Jan 23 02:16:20 2026 +1000

    [YAML Thing Provider] Log warning on Thing config errors (#5282)

    Catch IAE during Thing configuration loading to prevent
    the entire file from failing. Errors are now logged as warnings
    instead of throwing a full stack trace.

    Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>

commit 8a4c6b13933092bac8dde631c40d3c5240e17ac3
Author: jimtng <2554958+jimtng@users.noreply.github.com>
Date:   Sat Jan 24 04:53:12 2026 +1000

    Add support for short-form metadata namespace in Item Yaml config (#5250)

    * Support deserializing short-form metadata in YamlMetadataDTO

    So we can set
    `metadata: value`

    instead of
    ```
    metadata:
      value: value
    ```

    Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>

Do you see an issue backporting #5250 and #5282 as well?

I had the adapt 2 lines in the tests. I did not want to backport #5280, as it changes the data type of the returned value from BigDecimal to String.

I have not yet pushed this changes to 5.1.x.

lolodomo · 2026-01-30T06:05:01Z

Do you see an issue backporting #5250 and #5282 as well?

I had the adapt 2 lines in the tests. I did not want to backport #5280, as it changes the data type of the returned value from BigDecimal to String.

I am not at all in favor of backporting #5250 as it is a significant change only tested by one person and I am even not sure it will not be refactored in final 5.2 as @jimtng found a simpler approach.

I believe we need a new PR on branch 5.1.x (even without any unit tests if this is what adds "conflicts"). I can try to create that PR this evening.

) * [REST] API /file-format/parse: do not extend thing/channel config The /file-format/parse API does no more extend the thing and channel configuration with parameters having a default value. The output will contain hte thing/channel configuration parameters provided in the input. It is fixed for YAML and DSL formats. Unit tests added for YAML models containing Thing Signed-off-by: Laurent Garnier <lg.hc@free.fr>

holgerfriedrich · 2026-01-30T07:07:11Z

@lolodomo I did not use 5250. I have now pushed my proposal to a new branch on origin (could not get it to my personal repo, as I use a separate setup for the backports): https://github.com/openhab/openhab-core/tree/backports/5.1.x/5295

If you confirm, I can push this to 5.1.x.

lolodomo · 2026-01-30T07:41:14Z

@lolodomo I did not use 5250. I have now pushed my proposal to a new branch on origin (could not get it to my personal repo, as I use a separate setup for the backports): https://github.com/openhab/openhab-core/tree/backports/5.1.x/5295

If you confirm, I can push this to 5.1.x.

Sorry, I don't understand what you have done. I still see code from #5250 in your new branch.
Are there differences between your commit and mine?

) * [REST] API /file-format/parse: do not extend thing/channel config The /file-format/parse API does no more extend the thing and channel configuration with parameters having a default value. The output will contain hte thing/channel configuration parameters provided in the input. It is fixed for YAML and DSL formats. Unit tests added for YAML models containing Thing Signed-off-by: Laurent Garnier <lg.hc@free.fr>

holgerfriedrich · 2026-02-07T17:08:21Z

@lolodomo obviously I screwed it up.
Now it should be better, just picking 5282, a slightly modified version of this one, and finally adapting the test to 5.1.x.

) * [REST] API /file-format/parse: do not extend thing/channel config The /file-format/parse API does no more extend the thing and channel configuration with parameters having a default value. The output will contain hte thing/channel configuration parameters provided in the input. It is fixed for YAML and DSL formats. Unit tests added for YAML models containing Thing Signed-off-by: Laurent Garnier <lg.hc@free.fr>

holgerfriedrich · 2026-02-08T10:52:47Z

Backported to 5.1.x

lolodomo requested a review from a team as a code owner January 21, 2026 18:43

lolodomo marked this pull request as draft January 21, 2026 18:43

lolodomo force-pushed the do_not_extend_config branch 2 times, most recently from 2d87743 to 97b8d85 Compare January 22, 2026 00:04

wborn requested a review from Copilot January 22, 2026 12:58

Copilot started reviewing on behalf of wborn January 22, 2026 12:59 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

lolodomo force-pushed the do_not_extend_config branch from 97b8d85 to 0d5db33 Compare January 23, 2026 07:44

lolodomo marked this pull request as ready for review January 23, 2026 07:47

holgerfriedrich requested a review from Copilot January 23, 2026 10:09

Copilot started reviewing on behalf of holgerfriedrich January 23, 2026 10:10 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Copilot comment considered: rename method parameter name

ed33fc6

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo added 2 commits January 24, 2026 16:36

Unit tests added for YAML models containing Thing

5f1eb2e

Done for usual models populating the thing registry and for isolated models used by hte /file-format/parse REST API. These tests are also a way to test the YamlThingOrovider. Signed-off-by: Laurent Garnier <lg.hc@free.fr>

Merge branch 'main' into do_not_extend_config

9d9d560

lolodomo force-pushed the do_not_extend_config branch 5 times, most recently from e82d71e to d92110b Compare January 24, 2026 21:56

Remove one channel from test thing to simplify unit tests

7b06bbe

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo force-pushed the do_not_extend_config branch from d92110b to 7b06bbe Compare January 24, 2026 22:09

lolodomo added 2 commits January 24, 2026 23:30

Merge 2x2 unit tests to simplify

341270c

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

Enhanced unit tests + 1 new test

0976ae8

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo mentioned this pull request Jan 25, 2026

Round-tripping configuration via Code tab materializes default values #5238

Open

holgerfriedrich approved these changes Jan 27, 2026

View reviewed changes

holgerfriedrich merged commit 1a41209 into openhab:main Jan 27, 2026
11 of 13 checks passed

holgerfriedrich added the enhancement An enhancement or new feature of the Core label Jan 27, 2026

holgerfriedrich added this to the 5.2 milestone Jan 27, 2026

holgerfriedrich added the backported A PR that has been cherry-picked to a patch release branch label Feb 8, 2026

Uh oh!

Conversation

lolodomo commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

lolodomo commented Jan 22, 2026

Uh oh!

lolodomo commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lolodomo commented Jan 23, 2026

Uh oh!

lolodomo commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolodomo commented Jan 25, 2026

Uh oh!

holgerfriedrich commented Jan 25, 2026

Uh oh!

lolodomo commented Jan 25, 2026

Uh oh!

Nadahar commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolodomo commented Jan 25, 2026

Uh oh!

jimtng commented Jan 26, 2026

Uh oh!

holgerfriedrich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lolodomo commented Jan 28, 2026

Uh oh!

holgerfriedrich commented Jan 28, 2026

Uh oh!

holgerfriedrich commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolodomo commented Jan 30, 2026

Uh oh!

holgerfriedrich commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolodomo commented Jan 30, 2026

Uh oh!

holgerfriedrich commented Feb 7, 2026

Uh oh!

holgerfriedrich commented Feb 8, 2026

Uh oh!

Reviewers

Assignees

Labels

lolodomo commented Jan 21, 2026 •

edited

Loading

lolodomo commented Jan 23, 2026 •

edited

Loading

lolodomo commented Jan 25, 2026 •

edited

Loading

Nadahar commented Jan 25, 2026 •

edited

Loading

holgerfriedrich commented Jan 29, 2026 •

edited

Loading

holgerfriedrich commented Jan 30, 2026 •

edited

Loading