Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,8 @@ public Response removeTag(@PathParam("itemName") @Parameter(description = "item
@ApiResponse(responseCode = "200", description = "OK"), //
@ApiResponse(responseCode = "201", description = "Created"), //
@ApiResponse(responseCode = "404", description = "Item not found."), //
@ApiResponse(responseCode = "405", description = "Metadata not editable.") })
@ApiResponse(responseCode = "405", description = "Metadata not editable."),
@ApiResponse(responseCode = "503", description = "Managed provider not available.") })
public Response addMetadata(@PathParam("itemName") @Parameter(description = "item name") String itemName,
@PathParam("namespace") @Parameter(description = "namespace") String namespace,
@Parameter(description = "metadata", required = true) MetadataDTO metadata) {
Expand All @@ -765,14 +766,22 @@ 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 {
metadataRegistry.update(md);
Comment thread
mherwege marked this conversation as resolved.
Outdated
return Response.ok(null, MediaType.TEXT_PLAIN).build();
Comment thread
mherwege marked this conversation as resolved.
Outdated
}
} catch (UnsupportedOperationException e) {
// Trying to add to a reserved namespace that is in an unmanaged provider
return JSONResponse.createErrorResponse(Status.METHOD_NOT_ALLOWED, e.getMessage());
} catch (IllegalStateException e) {
// There is no managed provider available
return Response.status(Status.SERVICE_UNAVAILABLE).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.

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 @@ -782,8 +791,9 @@ public Response addMetadata(@PathParam("itemName") @Parameter(description = "ite
@Operation(operationId = "removeMetadataFromItem", summary = "Removes metadata from an item.", security = {
@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 = "404", description = "Item or namespace not found."),
@ApiResponse(responseCode = "405", description = "Metadata not editable."),
@ApiResponse(responseCode = "503", description = "Managed provider not available.") })
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 +805,22 @@ 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) {
// Exists, but not managed
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 (UnsupportedOperationException e) {
// Trying to remove from a reserved namespace that is in an unmanaged provider
return JSONResponse.createErrorResponse(Status.METHOD_NOT_ALLOWED, e.getMessage());
} catch (IllegalStateException e) {
// There is no managed provider available
return Response.status(Status.SERVICE_UNAVAILABLE).build();
}
}

Expand Down
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
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,52 @@ public void removeItemMetadata(String itemName) {
getManagedProvider()
.ifPresent(managedProvider -> ((ManagedMetadataProvider) managedProvider).removeItemMetadata(itemName));
}

@Override
public Metadata add(Metadata element) {
String namespace = element.getUID().getNamespace();
MetadataProvider provider = reservedNamespaces.get(namespace);
if (provider == null || provider instanceof ManagedProvider) {
return super.add(element);
Comment thread
mherwege marked this conversation as resolved.
}
throw new UnsupportedOperationException("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.

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();
MetadataProvider provider = reservedNamespaces.get(namespace);
if (provider == null || provider instanceof ManagedProvider) {
return super.update(element);
}
throw new UnsupportedOperationException("Cannot update metadata in '" + namespace + "' namespace");
}

@Override
public @Nullable Metadata remove(MetadataKey key) {
String namespace = key.getNamespace();
MetadataProvider provider = reservedNamespaces.get(namespace);
if (provider == null || provider instanceof ManagedProvider) {
return super.remove(key);
}
throw new UnsupportedOperationException("Cannot remove metadata from '" + namespace + "' namespace");
}

@Override
protected void addProvider(Provider<Metadata> provider) {
if (provider instanceof MetadataProvider metadataProvider) {
metadataProvider.getReservedNamespaces().stream()
.forEach(namespace -> reservedNamespaces.put(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);
}
}
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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import java.util.Collection;

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

/**
Expand Down Expand Up @@ -51,4 +53,46 @@ public interface MetadataRegistry extends Registry<Metadata, MetadataKey> {
* @param itemname the name of the item for which the metadata is to be removed.
*/
void removeItemMetadata(String itemname);

/**
* Add element to metadata
*
* @param element the element to add (must not be null)
* @return the added element or newly created object of the same type
*
* @throws UnsupportedOperationException if the metadata namespace has a reserved {@link MetadataProvider} that is
* not a {@link ManagedProvider}
* @throws IllegalStateException if no ManagedProvider is available
*/
@Override
Metadata add(Metadata element);

/**
* Update element in metadata
*
* @param element the element to update (must not be null)
* @return returns the old element or null if no element with the same key
* exists
*
* @throws UnsupportedOperationException if the metadata namespace has a reserved {@link MetadataProvider} that is
* not a {@link ManagedProvider}
* @throws IllegalStateException if no ManagedProvider is available
*/
@Override
@Nullable
Metadata update(Metadata element);

/**
* Remove element from metadata
*
* @param key the key of the element to remove (must not be null)
* @return the removed element, or null if no element with the given key exists
*
* @throws UnsupportedOperationException if the metadata namespace has a reserved {@link MetadataProvider} that is
* not a {@link ManagedProvider}
* @throws IllegalStateException if no ManagedProvider is available
*/
@Override
@Nullable
Metadata remove(MetadataKey key);
Comment thread
mherwege marked this conversation as resolved.
}
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 @@ -327,6 +328,18 @@ public void testAddMetadataUpdate() {
assertEquals(200, itemResource.addMetadata(ITEM_NAME1, "namespace", dto2).getStatus());
}

@Test
public void testAddMetadataUnmanagedReservedNamespace() {
MetadataDTO dto = new MetadataDTO();
dto.value = "some value";

MetadataProvider provider = mock(MetadataProvider.class);
when(provider.getReservedNamespaces()).thenReturn(Set.of("semantics"));
Comment thread
mherwege marked this conversation as resolved.
registerService(provider);

assertEquals(405, itemResource.addMetadata(ITEM_NAME1, "semantics", dto).getStatus());
}

@Test
public void testRemoveMetadataNonExistingItem() {
Response response = itemResource.removeMetadata("nonExisting", "anything");
Expand All @@ -347,7 +360,18 @@ 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.
}

@Test
public void testRemoveMetadataUnmanagedReservedNamespace() {
MetadataProvider provider = mock(MetadataProvider.class);
when(provider.getReservedNamespaces()).thenReturn(Set.of("semantics"));
when(provider.getAll())
.thenReturn(Set.of(new Metadata(new MetadataKey("semantics", ITEM_NAME1), "some value", null)));
registerService(provider);

assertEquals(405, itemResource.removeMetadata(ITEM_NAME1, "semantics").getStatus());
}

@SuppressWarnings("unused")
Expand Down
Loading