Skip to content

Item metadata REST endpoints: don't allow adding or removing semantics #5390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
holgerfriedrich merged 19 commits into from
Mar 21, 2026
Merged

Item metadata REST endpoints: don't allow adding or removing semantics #5390

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
da78b82
reserve semantics namespace
mherwege Feb 24, 2026
abab090
spotless
mherwege Feb 24, 2026
2648c75
fix test
mherwege Feb 24, 2026
8b392f7
fix javadoc
mherwege Feb 24, 2026
fcb7522
fix javadoc
mherwege Feb 24, 2026
0178239
copilot review adjustments
mherwege Mar 3, 2026
355e1d3
fix test
mherwege Mar 3, 2026
6f1ea7c
fix test
mherwege Mar 3, 2026
97da6e1
copilot feedback
mherwege Mar 3, 2026
1301f56
fix deleting all item metadata
mherwege Mar 3, 2026
5e3a4a0
add tests remove all metadata
mherwege Mar 3, 2026
5b25110
fix update for unmanaged and unreserved
mherwege Mar 3, 2026
6c4d37d
eliminate compiler warnings
mherwege Mar 3, 2026
9ba5b7f
handle multiple reservations for same namespace
mherwege Mar 4, 2026
f962ed2
Change log level to debug
mherwege Mar 4, 2026
10267ae
copilot feedback
mherwege Mar 4, 2026
de19943
handle exceptions in metadata console commands
mherwege Mar 4, 2026
39e195a
update javadoc
mherwege Mar 4, 2026
81b14fd
fix console remove return text
mherwege Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions ....io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/item/ItemResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,14 +765,20 @@ public Response addMetadata(@PathParam("itemName") @Parameter(description = "ite
value = "";
}

MetadataKey key = new MetadataKey(namespace, itemName);
Metadata md = new Metadata(key, value, metadata.config);
if (metadataRegistry.get(key) == null) {
metadataRegistry.add(md);
return Response.status(Status.CREATED).type(MediaType.TEXT_PLAIN).build();
} else {
metadataRegistry.update(md);
return Response.ok(null, MediaType.TEXT_PLAIN).build();
try {
MetadataKey key = new MetadataKey(namespace, itemName);
Metadata md = new Metadata(key, value, metadata.config);
if (metadataRegistry.get(key) == null) {
metadataRegistry.add(md);
return Response.status(Status.CREATED).type(MediaType.TEXT_PLAIN).build();
} else {
if (metadataRegistry.update(md) == null) {

Copilot AI Mar 3, 2026

Copy link

Choose a reason for hiding this comment

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

metadataRegistry.get(key) and metadataRegistry.update(md) are not atomic. If the metadata existed at the time of get but is removed concurrently before update, update can return null and this code will return 405 (“not editable”), which is misleading (it’s actually gone). A safer approach is to re-check metadataRegistry.get(key) when update returns null and return 404 if it’s now absent, reserving 405 for the “exists but not managed/editable” case.

Suggested change
if (metadataRegistry.update(md) == null) {
Metadata previous = metadataRegistry.update(md);
if (previous == null) {
// Metadata may have been removed concurrently; re-check existence
if (metadataRegistry.get(key) == null) {
return Response.status(Status.NOT_FOUND).build();
}

Copilot uses AI. Check for mistakes.

@mherwege mherwege Mar 3, 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.

I don't think this is a good idea. You try to add. Responding you are not adding because the metadata does not exist is not what you would expect. You could try to add again, but you get in a potential loop. I prefer the current not allowed.

return Response.status(Status.METHOD_NOT_ALLOWED).build();
}
return Response.ok(null, MediaType.TEXT_PLAIN).build();
}
} catch (IllegalStateException e) {
return Response.status(Status.METHOD_NOT_ALLOWED.getStatusCode(), e.getMessage()).build();
}
Comment on lines +768 to 785

Copilot AI Feb 28, 2026

Copy link

Choose a reason for hiding this comment

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

Catching all IllegalStateException here will also convert unrelated registry failures (e.g., ManagedProvider is not available thrown by AbstractRegistry.add/update/remove) into a 405, which can mask real server/configuration problems. It would be safer to throw/catch a dedicated exception type for “reserved namespace not editable” (e.g., UnsupportedOperationException or a custom runtime exception) and let other IllegalStateExceptions propagate/return a 500-style error.

Copilot uses AI. Check for mistakes.

@mherwege mherwege Mar 3, 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.

I am using UnsupportedOperationException now. But looking at the previous code, while the API documentation said it was returning 405 if the metadata was not editable, it did not do that. The IllegalStateException was also never catched and not returned by the REST API. Not having a managed provider should be an acceptable state. It just means nothing can be edited through the REST API or UI. But it should be handled in my opinion.

Comment thread
mherwege marked this conversation as resolved.
}

Expand All @@ -783,7 +789,7 @@ public Response addMetadata(@PathParam("itemName") @Parameter(description = "ite
@SecurityRequirement(name = "oauth2", scopes = { "admin" }) }, responses = {
@ApiResponse(responseCode = "200", description = "OK"),
@ApiResponse(responseCode = "404", description = "Item not found."),
@ApiResponse(responseCode = "405", description = "Meta data not editable.") })
@ApiResponse(responseCode = "405", description = "Metadata not editable.") })
public Response removeMetadata(@PathParam("itemName") @Parameter(description = "item name") String itemName,
@Nullable @PathParam("namespace") @Parameter(description = "namespace") String namespace) {
Item item = getItem(itemName);
Expand All @@ -795,13 +801,17 @@ public Response removeMetadata(@PathParam("itemName") @Parameter(description = "
if (namespace == null) {
metadataRegistry.removeItemMetadata(itemName);
} else {
MetadataKey key = new MetadataKey(namespace, itemName);
if (metadataRegistry.get(key) != null) {
if (metadataRegistry.remove(key) == null) {
return Response.status(Status.CONFLICT).build();
try {
MetadataKey key = new MetadataKey(namespace, itemName);
if (metadataRegistry.get(key) != null) {
if (metadataRegistry.remove(key) == null) {
return Response.status(Status.METHOD_NOT_ALLOWED).build();
}
} else {
return Response.status(Status.NOT_FOUND).build();
}
} else {
return Response.status(Status.NOT_FOUND).build();
} catch (IllegalStateException e) {
return Response.status(Status.METHOD_NOT_ALLOWED.getStatusCode(), e.getMessage()).build();
}
}

Expand Down
6 changes: 6 additions & 0 deletions ...emantics/src/main/java/org/openhab/core/semantics/internal/SemanticsMetadataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.TreeMap;

import org.eclipse.jdt.annotation.NonNullByDefault;
Expand Down Expand Up @@ -108,6 +109,11 @@ public Collection<Metadata> getAll() {
return semantics.values();
}

@Override
public Collection<String> getReservedNamespaces() {
return Set.of(NAMESPACE);
}

/**
* Updates the semantic metadata for an item and notifies all listeners about changes
*
Expand Down
53 changes: 53 additions & 0 deletions .../org.openhab.core/src/main/java/org/openhab/core/internal/items/MetadataRegistryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,15 @@
package org.openhab.core.internal.items;

import java.util.Collection;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.core.common.registry.AbstractRegistry;
import org.openhab.core.common.registry.ManagedProvider;
import org.openhab.core.common.registry.Provider;
import org.openhab.core.events.EventPublisher;
import org.openhab.core.items.ManagedMetadataProvider;
import org.openhab.core.items.Metadata;
Expand All @@ -37,12 +42,15 @@
* keeps track of all declared metadata of all metadata providers.
*
* @author Kai Kreuzer - Initial contribution
* @author Mark Herwege - semantics namespace not in managed provider
*/
@Component(immediate = true, service = MetadataRegistry.class)
@NonNullByDefault
public class MetadataRegistryImpl extends AbstractRegistry<Metadata, MetadataKey, MetadataProvider>
implements MetadataRegistry {

private Map<String, MetadataProvider> reservedNamespaces = new ConcurrentHashMap<>();
Comment thread
mherwege marked this conversation as resolved.
Outdated

@Activate
public MetadataRegistryImpl(final @Reference ReadyService readyService) {
super(MetadataProvider.class);
Expand Down Expand Up @@ -114,4 +122,49 @@ public void removeItemMetadata(String itemName) {
getManagedProvider()
.ifPresent(managedProvider -> ((ManagedMetadataProvider) managedProvider).removeItemMetadata(itemName));
}

@Override
public Metadata add(Metadata element) {
String namespace = element.getUID().getNamespace();
if (reservedNamespaces.get(namespace) == null || reservedNamespaces.get(namespace) instanceof ManagedProvider) {
return super.add(element);
Comment thread
mherwege marked this conversation as resolved.
}
throw new IllegalStateException("Cannot add metadata to '" + namespace + "' namespace");
}
Comment on lines +131 to +139

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

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

MetadataRegistryImpl.add/update/remove now throw UnsupportedOperationException for reserved namespaces. There are existing callers that invoke metadataRegistry.remove(...)/update(...) without handling this (e.g., the REST /items/metadata/purge implementation and the metadata console commands), which will now fail with an uncaught runtime exception whenever they touch reserved namespaces like semantics. Consider avoiding exceptions here (e.g., return null for update/remove and block add via a separate check) or update all internal callers to catch UnsupportedOperationException and handle it gracefully.

Copilot uses AI. Check for mistakes.

@mherwege mherwege Mar 4, 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.

I was throwing IllegalStateException before, which was also not handled.
I handle it now, and also updated the console commands to handle this properly.


@Override
public @Nullable Metadata update(Metadata element) {
String namespace = element.getUID().getNamespace();
if (reservedNamespaces.get(namespace) == null || reservedNamespaces.get(namespace) instanceof ManagedProvider) {
return super.update(element);
}
throw new IllegalStateException("Cannot update metadata in '" + namespace + "' namespace");
}

@Override
public @Nullable Metadata remove(MetadataKey key) {
String namespace = key.getNamespace();
if (reservedNamespaces.get(namespace) == null || reservedNamespaces.get(namespace) instanceof ManagedProvider) {
return super.remove(key);
}
throw new IllegalStateException("Cannot remove metadata from '" + namespace + "' namespace");
Comment thread
mherwege marked this conversation as resolved.
Outdated
}

@Override
protected void addProvider(Provider<Metadata> provider) {
if (provider instanceof MetadataProvider metadataProvider) {
metadataProvider.getReservedNamespaces().stream()
.forEach(namespace -> reservedNamespaces.putIfAbsent(namespace, metadataProvider));
}
Comment thread
mherwege marked this conversation as resolved.
super.addProvider(provider);
}

@Override
protected void removeProvider(Provider<Metadata> provider) {
if (provider instanceof MetadataProvider metadataProvider) {
metadataProvider.getReservedNamespaces().stream()
.forEach(namespace -> reservedNamespaces.remove(namespace, metadataProvider));
}
Comment thread
mherwege marked this conversation as resolved.
Comment thread
mherwege marked this conversation as resolved.
super.removeProvider(provider);
}
}
21 changes: 21 additions & 0 deletions bundles/org.openhab.core/src/main/java/org/openhab/core/items/MetadataProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,37 @@
*/
package org.openhab.core.items;

import java.util.Collection;
import java.util.Set;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.openhab.core.common.registry.ManagedProvider;
import org.openhab.core.common.registry.Provider;

/**
* This is a marker interface for metadata provider implementations that should be used to register those as an OSGi
* service.
*
* @author Kai Kreuzer - Initial contribution
* @author Mark Herwege - Added reserved namespaces
*/
@NonNullByDefault
public interface MetadataProvider extends Provider<Metadata> {

/**
* A {@link MetadataProvider} implementation can reserve a metadata namespace. Only a single provider for this
* namespace can provide metadata for this namespace. Updating metadata in this namespace will have to be with this
* provider, and is refused if the provider is not a {@link ManagedProvider}.
*
* This is useful if providers calculate metadata and this metadata is not meant to be persisted with a
* {@link ManagedProvider}. An example is semantics metadata provided by its own provider.
* Implementations are expected to return an immutable {@link Collection}.
*
* The default implementation returns an empty {@link Set}.
*
* @return collection reserved namespaces
*/
public default Collection<String> getReservedNamespaces() {
return Set.of();
}
}
3 changes: 2 additions & 1 deletion ...tests/src/main/java/org/openhab/core/io/rest/core/internal/item/ItemResourceOSGiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static org.hamcrest.collection.IsCollectionWithSize.hasSize;
import static org.hamcrest.core.IsIterableContaining.hasItems;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.Mockito.*;

import java.io.ByteArrayOutputStream;
Expand Down Expand Up @@ -347,7 +348,7 @@ public void testRemoveMetadataUnmanagedMetadata() {
registerService(provider);

Response response = itemResource.removeMetadata(ITEM_NAME1, "namespace");
assertEquals(409, response.getStatus());
assertEquals(405, response.getStatus());
Comment thread
mherwege marked this conversation as resolved.
}

@SuppressWarnings("unused")
Expand Down