Item metadata REST endpoints: don't allow adding or removing semantics #5390
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,7 +110,6 @@ | |
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import com.google.gson.JsonObject; | ||
|
|
||
| import io.swagger.v3.oas.annotations.Operation; | ||
|
|
@@ -182,8 +181,6 @@ private static void respectForwarded(final UriBuilder uriBuilder, final @Context | |
| } | ||
|
|
||
| private final Logger logger = LoggerFactory.getLogger(ItemResource.class); | ||
| private final DTOMapper dtoMapper; | ||
| private final EventPublisher eventPublisher; | ||
| private final ItemBuilderFactory itemBuilderFactory; | ||
|
|
@@ -500,7 +497,7 @@ private Response sendItemStateInternal(@Nullable String language, String itemNam | |
| final Locale locale = localeService.getLocale(language); | ||
| final ZoneId zoneId = timeZoneProvider.getTimeZone(); | ||
|
|
||
| String eventSource = buildSource(source, securityContext); | ||
|
|
||
| // get Item | ||
| Item item = getItem(itemName); | ||
|
|
@@ -512,7 +509,7 @@ private Response sendItemStateInternal(@Nullable String language, String itemNam | |
|
|
||
| if (state != null) { | ||
| // set State and report OK | ||
| eventPublisher.post(ItemEventFactory.createStateEvent(itemName, state, eventSource)); | ||
| return getItemResponse(null, Status.ACCEPTED, null, locale, zoneId, null); | ||
| } else { | ||
| // State could not be parsed | ||
|
|
@@ -566,7 +563,7 @@ private Response sendItemCommandInternal(String itemName, String value, @Nullabl | |
| SecurityContext securityContext) { | ||
| Item item = getItem(itemName); | ||
| Command command = null; | ||
| String eventSource = buildSource(source, securityContext); | ||
| if (item != null) { | ||
| if ("toggle".equalsIgnoreCase(value) && (item instanceof SwitchItem || item instanceof RollershutterItem)) { | ||
| if (OnOffType.ON.equals(item.getStateAs(OnOffType.class))) { | ||
|
|
@@ -585,7 +582,7 @@ private Response sendItemCommandInternal(String itemName, String value, @Nullabl | |
| command = TypeParser.parseCommand(item.getAcceptedCommandTypes(), value); | ||
| } | ||
| if (command != null) { | ||
| eventPublisher.post(ItemEventFactory.createCommandEvent(itemName, command, eventSource)); | ||
| ResponseBuilder resbuilder = Response.ok(); | ||
| resbuilder.type(MediaType.TEXT_PLAIN); | ||
| return resbuilder.build(); | ||
|
|
@@ -750,7 +747,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 = "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) { | ||
|
|
@@ -765,47 +763,83 @@ public Response addMetadata(@PathParam("itemName") @Parameter(description = "ite | |
| value = ""; | ||
| } | ||
|
|
||
| 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) { | ||
| // Exists, but not managed | ||
| return Response.status(Status.METHOD_NOT_ALLOWED).build(); | ||
| } | ||
| return Response.ok(null, MediaType.TEXT_PLAIN).build(); | ||
| } | ||
| } 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
There was a problem hiding this comment. Catching all
Contributor
Author
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.
mherwege marked this conversation as resolved.
|
||
| } | ||
|
|
||
| @DELETE | ||
| @RolesAllowed({ Role.ADMIN }) | ||
| @Path("/{itemName: [a-zA-Z_0-9]+}/metadata") | ||
| @Operation(operationId = "removeAllMetadataFromItem", summary = "Removes all managed metadata from an item.", security = { | ||
| @SecurityRequirement(name = "oauth2", scopes = { "admin" }) }, responses = { | ||
| @ApiResponse(responseCode = "200", description = "OK"), | ||
| @ApiResponse(responseCode = "404", description = "Item not found.") }) | ||
| public Response removeAllMetadata(@PathParam("itemName") @Parameter(description = "item name") String itemName) { | ||
| Item item = getItem(itemName); | ||
|
|
||
| if (item == null) { | ||
| return Response.status(Status.NOT_FOUND).build(); | ||
| } | ||
|
|
||
| metadataRegistry.removeItemMetadata(itemName); | ||
| return Response.ok(null, MediaType.TEXT_PLAIN).build(); | ||
| } | ||
|
|
||
| @DELETE | ||
| @RolesAllowed({ Role.ADMIN }) | ||
| @Path("/{itemName: [a-zA-Z_0-9]+}/metadata/{namespace}") | ||
| @Operation(operationId = "removeMetadataFromItem", summary = "Removes metadata in a specific namespace from an item.", security = { | ||
| @SecurityRequirement(name = "oauth2", scopes = { "admin" }) }, responses = { | ||
| @ApiResponse(responseCode = "200", description = "OK"), | ||
| @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, | ||
| @PathParam("namespace") @Parameter(description = "namespace") String namespace) { | ||
| Item item = getItem(itemName); | ||
|
|
||
| if (item == null) { | ||
| return Response.status(Status.NOT_FOUND).build(); | ||
| } | ||
|
|
||
| try { | ||
| MetadataKey key = new MetadataKey(namespace, itemName); | ||
| if (metadataRegistry.get(key) != null) { | ||
| Metadata removedMetadata = metadataRegistry.remove(key); | ||
| if (removedMetadata != null) { | ||
| return Response.ok(null, MediaType.TEXT_PLAIN).build(); | ||
| } | ||
| if (metadataRegistry.get(key) != null) { | ||
| // Exists, but not managed, and not removed in the mean time | ||
| return Response.status(Status.METHOD_NOT_ALLOWED).build(); | ||
| } | ||
| } | ||
| 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(); | ||
| } | ||
| } | ||
|
|
||
| @POST | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,16 @@ | |
| package org.openhab.core.internal.items; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| 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.Provider; | ||
| import org.openhab.core.events.EventPublisher; | ||
| import org.openhab.core.items.ManagedMetadataProvider; | ||
| import org.openhab.core.items.Metadata; | ||
|
|
@@ -31,18 +37,24 @@ | |
| import org.osgi.service.component.annotations.Reference; | ||
| import org.osgi.service.component.annotations.ReferenceCardinality; | ||
| import org.osgi.service.component.annotations.ReferencePolicy; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * This is the main implementing class of the {@link MetadataRegistry} interface. It | ||
| * 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 final Logger logger = LoggerFactory.getLogger(MetadataRegistryImpl.class); | ||
| private final Map<String, Set<MetadataProvider>> reservedNamespaces = new ConcurrentHashMap<>(); | ||
|
|
||
| @Activate | ||
| public MetadataRegistryImpl(final @Reference ReadyService readyService) { | ||
| super(MetadataProvider.class); | ||
|
|
@@ -114,4 +126,71 @@ public void removeItemMetadata(String itemName) { | |
| getManagedProvider() | ||
| .ifPresent(managedProvider -> ((ManagedMetadataProvider) managedProvider).removeItemMetadata(itemName)); | ||
| } | ||
|
|
||
| @Override | ||
| 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); | ||
|
mherwege marked this conversation as resolved.
|
||
| } | ||
| throw new UnsupportedOperationException("Cannot add metadata to '" + namespace + "' namespace"); | ||
| } | ||
|
Comment on lines
+131
to
+139
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. I was throwing IllegalStateException before, which was also not handled. |
||
|
|
||
| @Override | ||
| public @Nullable Metadata update(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.update(element); | ||
| } | ||
| throw new UnsupportedOperationException("Cannot update metadata in '" + namespace + "' namespace"); | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable Metadata remove(MetadataKey key) { | ||
| String namespace = key.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.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 -> { | ||
| Set<MetadataProvider> currentProviders = reservedNamespaces.getOrDefault(namespace, Set.of()); | ||
| if (!currentProviders.isEmpty()) { | ||
| logger.debug("Multiple metadata providers are reserving namespace '{}', there should only be one.", | ||
| namespace); | ||
| } | ||
| Set<MetadataProvider> providers = Stream | ||
| .concat(currentProviders.stream(), Set.of(metadataProvider).stream()) | ||
| .collect(Collectors.toSet()); | ||
| reservedNamespaces.put(namespace, providers); | ||
| }); | ||
| } | ||
|
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 -> { | ||
| Set<MetadataProvider> providers = reservedNamespaces.getOrDefault(namespace, Set.of()).stream() | ||
| .filter(p -> !provider.equals(p)).collect(Collectors.toSet()); | ||
| if (providers.isEmpty()) { | ||
| reservedNamespaces.remove(namespace); | ||
| } else { | ||
| reservedNamespaces.put(namespace, providers); | ||
| } | ||
| }); | ||
| } | ||
|
mherwege marked this conversation as resolved.
mherwege marked this conversation as resolved.
|
||
| super.removeProvider(provider); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metadataRegistry.get(key)andmetadataRegistry.update(md)are not atomic. If the metadata existed at the time ofgetbut is removed concurrently beforeupdate,updatecan returnnulland this code will return 405 (“not editable”), which is misleading (it’s actually gone). A safer approach is to re-checkmetadataRegistry.get(key)whenupdatereturnsnulland return 404 if it’s now absent, reserving 405 for the “exists but not managed/editable” case.There was a problem hiding this comment.
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.