Item metadata REST endpoints: don't allow adding or removing semantics - #5390
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
florian-h05
left a comment
There was a problem hiding this comment.
LGTM, thanks!
(No core maintainer though, so I cannot merge this.)
There was a problem hiding this comment.
Pull request overview
Adds a “reserved metadata namespace” mechanism to prevent REST writes to computed/non-managed metadata (notably the semantics namespace), avoiding persistence into the managed metadata store and the resulting inconsistencies.
Changes:
- Introduce
MetadataProvider#getReservedNamespaces()to declare reserved metadata namespaces. - Enforce reserved-namespace write restrictions in
MetadataRegistryImpl(block add/update/remove when the reserving provider is not managed). - Update item metadata REST endpoints to return
405 Method Not Allowedwhen metadata is not editable, and adjust integration test expectations accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| itests/.../ItemResourceOSGiTest.java | Updates expected HTTP status for removing metadata provided by an unmanaged provider. |
| bundles/org.openhab.core/.../MetadataProvider.java | Adds API for providers to declare reserved namespaces. |
| bundles/org.openhab.core/.../MetadataRegistryImpl.java | Implements reserved-namespace tracking and blocks write operations for reserved namespaces owned by non-managed providers. |
| bundles/org.openhab.core.semantics/.../SemanticsMetadataProvider.java | Declares semantics as a reserved namespace. |
| bundles/org.openhab.core.io.rest.core/.../ItemResource.java | Maps registry write failures to 405 Method Not Allowed for metadata endpoints (PUT/DELETE). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) { | ||
| 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(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -795,13 +805,22 @@ public Response removeMetadata(@PathParam("itemName") @Parameter(description = " | |||
| if (namespace == null) { | |||
| metadataRegistry.removeItemMetadata(itemName); | |||
| } else { | |||
There was a problem hiding this comment.
This method is bound to @Path(.../metadata/{namespace}), so @PathParam("namespace") will never be null for real HTTP requests and the if (namespace == null) branch is effectively dead code. Consider removing @Nullable and the null-branch (or adding a separate endpoint without {namespace} if you intend to support deleting all metadata).
There was a problem hiding this comment.
This is a bug that was already there, confirmed.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| metadataRegistry.add(md); | ||
| return Response.status(Status.CREATED).type(MediaType.TEXT_PLAIN).build(); | ||
| } else { | ||
| if (metadataRegistry.update(md) == null) { |
There was a problem hiding this comment.
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.
| 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(); | |
| } |
There was a problem hiding this comment.
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.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Metadata add(Metadata element) { | ||
| String namespace = element.getUID().getNamespace(); | ||
| Set<MetadataProvider> providers = reservedNamespaces.get(namespace); | ||
| MetadataProvider managedProvider = (MetadataProvider) getManagedProvider().orElse(null); | ||
| if (providers == null || providers.isEmpty() || providers.stream().anyMatch(p -> p.equals(managedProvider))) { | ||
| return super.add(element); | ||
| } | ||
| throw new UnsupportedOperationException("Cannot add metadata to '" + namespace + "' namespace"); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I was throwing IllegalStateException before, which was also not handled.
I handle it now, and also updated the console commands to handle this properly.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
copilot is fully happy now. |
Closes #5377
The PUT and DELETE /item/{itemname}/metadata/{namespace} endpoints allow adding/updating and removing metadata.
If you use this endpoint to add something in the semantics namespace when the item did not already have semantics defined, it will be stored in the managed provider.
A user identified issues when trying to use an LLM to update the item semantics (#5363 (comment)). The LLM added semantics to the metadata managed provider. Semantics metadata is auto-generated from item tags in the SemanticsMetadataProvider, and this is not a managed provider, so the REST endpoint should not write in it. The only way to remove it was to edit the JSONDB.
To avoid such issues, the REST endpoint should not allow changing anything in the semantics namespace.
An easy fix would have been to just check for the namespace in the REST API. The disadvantage would be that it then assumes the SemanticsMetadataProvider is part of the installation. If someone would ever remove or replace the SemanticsMetadataProvider, the semantics namespace would still not be available.
I therefore opted for a more general fix. I have introduce the concept of reserved namespace(s) for a provider. If there is a reserved namespace and the provider is not a managed provider, it will refuse add, update or delete operations.
This also fixes the DELETE /item/{itemname}/metadata/{namespace}. The intention in the code is to allow a null namespace to be passed. JAX-RS does not allow null path parameters and this would never have worked. I created an extra endpoint which ommits the namespace path parameter to delete all managed metadata for an item.