Some minor improvements in the tagging implementation

- Improve (De-)serialization of tag model in DynamoTagConfigClient - Make Ref model immutable (again) - Remove new usages of apache-commons lib (which is to be removed from dependencies soon) - Rename abstract dataset VersionRefSource to VersionedSource - Rename HubWebClient#getTag() to #loadTag() to match the style of the other entity loading methods (and also to indicate that the method call could be a longer running one as it's a query to remote system) - Add missing method HubWebClientAsync#loadTagAsync() Signed-off-by: Benjamin Rögner <[email protected]>
qGYdXbY2 · Mar 28, 2024 · 571f803 · 571f803
1 parent 7b32ba4
commit 571f803
Show file tree

Hide file tree

Showing 13 changed files with 299 additions and 288 deletions.
diff --git a/xyz-hub-service/src/main/java/com/here/xyz/httpconnector/util/jobs/Export.java b/xyz-hub-service/src/main/java/com/here/xyz/httpconnector/util/jobs/Export.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2023 HERE Europe B.V.
+ * Copyright (C) 2017-2024 HERE Europe B.V.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -69,7 +69,7 @@
 import com.here.xyz.jobs.datasets.FileBasedTarget;
 import com.here.xyz.jobs.datasets.FileOutputSettings;
 import com.here.xyz.jobs.datasets.Identifiable;
-import com.here.xyz.jobs.datasets.VersionRefSource;
+import com.here.xyz.jobs.datasets.VersionedSource;
 import com.here.xyz.jobs.datasets.files.Csv;
 import com.here.xyz.jobs.datasets.files.FileFormat;
 import com.here.xyz.jobs.datasets.files.GeoJson;
@@ -268,16 +268,17 @@ public Future<Export> setDefaults() {
                 if (readParamExtends() != null && context == null)
                     addParam(PARAM_CONTEXT, DEFAULT);
 
-                if (getSource() instanceof VersionRefSource<?> versionRefSource
+                if (getSource() instanceof VersionedSource<?> versionRefSource
                     && getSource() instanceof Identifiable<?> identifiable
                     && versionRefSource.getVersionRef() != null
-                    && !versionRefSource.getVersionRef().isResolved()) {
+                    && versionRefSource.getVersionRef().isTag()) {
                   final Ref ref = versionRefSource.getVersionRef();
                   try {
-                    long version = CService.hubWebClient.getTag(identifiable.getId(), ref.getVersionRef()).getVersion();
-                    ref.setVersion(version);
+                    long version = CService.hubWebClient.loadTag(identifiable.getId(), ref.getTag()).getVersion();
+                    versionRefSource.setVersionRef(new Ref(version));
                     setTargetVersion(String.valueOf(version));
-                  } catch (HubWebClientException e) {
+                  }
+                  catch (HubWebClientException e) {
                     return Future.failedFuture(e);
                   }
                 }

diff --git a/xyz-hub-service/src/main/java/com/here/xyz/hub/config/dynamo/DynamoTagConfigClient.java b/xyz-hub-service/src/main/java/com/here/xyz/hub/config/dynamo/DynamoTagConfigClient.java
@@ -19,7 +19,6 @@
 
 package com.here.xyz.hub.config.dynamo;
 
-import com.amazonaws.services.dynamodbv2.document.DeleteItemOutcome;
 import com.amazonaws.services.dynamodbv2.document.Item;
 import com.amazonaws.services.dynamodbv2.document.Table;
 import com.amazonaws.services.dynamodbv2.document.TableKeysAndAttributes;
@@ -30,27 +29,20 @@
 import com.amazonaws.services.dynamodbv2.model.ExecuteTransactionRequest;
 import com.amazonaws.services.dynamodbv2.model.ParameterizedStatement;
 import com.amazonaws.services.dynamodbv2.model.ReturnValue;
-import com.amazonaws.util.CollectionUtils;
+import com.here.xyz.XyzSerializable;
 import com.here.xyz.hub.config.TagConfigClient;
 import com.here.xyz.models.hub.Tag;
 import com.here.xyz.util.service.aws.dynamo.DynamoClient;
 import com.here.xyz.util.service.aws.dynamo.IndexDefinition;
 import io.vertx.core.Future;
-import io.vertx.core.json.Json;
-import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
-import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.Marker;
-import software.amazon.awssdk.utils.AttributeMap;
 
 public class DynamoTagConfigClient extends TagConfigClient {
 
@@ -75,6 +67,7 @@ public Future<Void> init() {
   @Override
   public Future<Tag> getTag(Marker marker, String id, String spaceId) {
     try {
+      //TODO: Replace PartiQL query by actual query
       final ExecuteStatementRequest request = new ExecuteStatementRequest()
           .withStatement("SELECT * FROM \"" + tagTable.getTableName() + "\" WHERE \"id\" = ? and \"spaceId\" = ?")
           .withParameters(new AttributeValue(id), new AttributeValue(spaceId));
@@ -90,7 +83,7 @@ public Future<Tag> getTag(Marker marker, String id, String spaceId) {
 
   @Override
   public Future<List<Tag>> getTags(Marker marker, String tagId, List<String> spaceIds) {
-    if (CollectionUtils.isNullOrEmpty(spaceIds))
+    if (spaceIds == null || spaceIds.isEmpty())
       return Future.succeededFuture(Collections.emptyList());
 
     return dynamoClient.executeQueryAsync(() -> {
@@ -117,6 +110,7 @@ public Future<List<Tag>> getTags(Marker marker, String tagId, List<String> space
 
   public Future<List<Tag>> getTagsByTagId(Marker marker, String tagId) {
     try {
+      //TODO: Replace PartiQL query by actual query
       ExecuteStatementRequest request = new ExecuteStatementRequest()
           .withStatement("SELECT * FROM \"" + tagTable.getTableName() + "\" WHERE \"id\" = ?")
           .withParameters(new AttributeValue(tagId));
@@ -134,6 +128,7 @@ public Future<List<Tag>> getTags(Marker marker, String spaceId, boolean includeS
     try {
       final String includeSystemTagsQuery = includeSystemTags ? "" : " AND (\"system\" is MISSING OR \"system\" = false)";
 
+      //TODO: Replace PartiQL query by actual query
       final ExecuteStatementRequest request = new ExecuteStatementRequest()
           .withStatement("SELECT * FROM \"" + tagTable.getTableName() + "\".\"spaceId-index\" WHERE \"spaceId\" = ?" + includeSystemTagsQuery)
           .withParameters(new AttributeValue(spaceId));
@@ -149,9 +144,10 @@ public Future<List<Tag>> getTags(Marker marker, String spaceId, boolean includeS
   @Override
   public Future<List<Tag>> getTags(Marker marker, List<String> spaceIds) {
     try {
-      String spaceParamsSt = StringUtils.join(Collections.nCopies(spaceIds.size(), "?"), ",");
+      String spaceParamsSt = String.join(",", Collections.nCopies(spaceIds.size(), "?"));
       List<AttributeValue> params = spaceIds.stream().map(AttributeValue::new).collect(Collectors.toList());
 
+      //TODO: Replace PartiQL query by actual query
       final ExecuteStatementRequest request = new ExecuteStatementRequest()
           .withStatement("SELECT * FROM \"" + tagTable.getTableName() + "\" WHERE \"spaceId\" IN [" + spaceParamsSt + "]")
           .withParameters(params);
@@ -167,6 +163,7 @@ public Future<List<Tag>> getTags(Marker marker, List<String> spaceIds) {
   @Override
   public Future<List<Tag>> getAllTags(Marker marker) {
     try {
+      //TODO: Replace PartiQL query by actual query
       final ExecuteStatementRequest request = new ExecuteStatementRequest()
           .withStatement("SELECT * FROM \"" + tagTable.getTableName() + "\"");
 
@@ -181,11 +178,7 @@ public Future<List<Tag>> getAllTags(Marker marker) {
   @Override
   public Future<Void> storeTag(Marker marker, Tag tag) {
     return dynamoClient.executeQueryAsync(() -> {
-      tagTable.putItem(new Item()
-          .withString("id", tag.getId())
-          .withString("spaceId", tag.getSpaceId())
-          .withLong("version", tag.getVersion())
-          .withBoolean("system", tag.isSystem()));
+      tagTable.putItem(Item.fromMap(XyzSerializable.toMap(tag)));
       return null;
     });
   }
@@ -196,16 +189,8 @@ public Future<Tag> deleteTag(Marker marker, String id, String spaceId) {
       DeleteItemSpec deleteItemSpec = new DeleteItemSpec()
           .withPrimaryKey("id", id, "spaceId", spaceId)
           .withReturnValues(ReturnValue.ALL_OLD);
-      DeleteItemOutcome response = tagTable.deleteItem(deleteItemSpec);
-      if (response.getItem() != null) {
-        final Map<String, Object> tagData = response.getItem().asMap();
-        return new Tag()
-            .withId((String) tagData.get("id"))
-            .withSpaceId((String) tagData.get("spaceId"))
-            .withVersion(((BigDecimal) tagData.get("version")).intValue())
-            .withSystem((Boolean) tagData.get("system"));
-      }
-      return null;
+      Item tagItem = tagTable.deleteItem(deleteItemSpec).getItem();
+      return tagItem == null ? null : XyzSerializable.fromMap(tagItem.asMap(), Tag.class);
     });
   }
 
@@ -214,7 +199,6 @@ public Future<List<Tag>> deleteTagsForSpace(Marker marker, String spaceId) {
     return getTags(marker, spaceId, true)
         .compose(tags -> {
           try {
-
             if (tags.isEmpty())
               return Future.succeededFuture();
 
@@ -234,7 +218,7 @@ public Future<List<Tag>> deleteTagsForSpace(Marker marker, String spaceId) {
 
   private static List<Tag> tagDataToTags(List<Map<String, AttributeValue>> items) {
     if (items == null || items.isEmpty())
-      return new ArrayList<>();
+      return Collections.emptyList();
 
     return items.stream().map(tagData -> new Tag()
         .withId(tagData.get("id").getS())

diff --git a/xyz-hub-service/src/main/java/com/here/xyz/hub/rest/SpaceBasedApi.java b/xyz-hub-service/src/main/java/com/here/xyz/hub/rest/SpaceBasedApi.java
@@ -1,3 +1,22 @@
+/*
+ * Copyright (C) 2017-2024 HERE Europe B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * License-Filename: LICENSE
+ */
+
 package com.here.xyz.hub.rest;
 
 import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
@@ -76,7 +95,7 @@ protected Ref getRef(RoutingContext context) throws HttpException {
       return new Ref(versionRef);
     }
     catch (InvalidRef e) {
-      throw new HttpException(BAD_REQUEST, "Invalid value for version: " + version, e);
+      throw new HttpException(BAD_REQUEST, "Invalid value for versionRef: " + versionRef, e);
     }
   }
 }
diff --git a/xyz-hub-service/src/main/java/com/here/xyz/hub/rest/TagApi.java b/xyz-hub-service/src/main/java/com/here/xyz/hub/rest/TagApi.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017-2023 HERE Europe B.V.
+ * Copyright (C) 2017-2024 HERE Europe B.V.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -36,7 +36,6 @@
 import io.vertx.core.Future;
 import io.vertx.ext.web.RoutingContext;
 import io.vertx.ext.web.openapi.router.RouterBuilder;
-import java.util.regex.Pattern;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.Marker;
 
@@ -121,6 +120,7 @@ public static Future<Tag> updateTag(Marker marker, String spaceId, String tagId,
       return Future.failedFuture(new ValidationException("Invalid spaceId parameter"));
     }
 
+    //FIXME: Neither -2 nor -1 are valid versions
     if (version < -2) {
       return Future.failedFuture(new ValidationException("Invalid version parameter"));
     }
@@ -141,21 +141,19 @@ public static Future<Tag> createTag(Marker marker, String spaceId, String tagId)
   }
 
   // TODO auth
-  public static Future<Tag> createTag(Marker marker, String spaceId, String tagId, long
-      version, boolean system) {
+  public static Future<Tag> createTag(Marker marker, String spaceId, String tagId, long version, boolean system) {
     if (spaceId == null) {
       return Future.failedFuture(new ValidationException("Invalid spaceId parameter"));
     }
 
-    if (tagId == null || invalidTagId(tagId)) {
+    if (!Tag.isValidId(tagId)) {
       return Future.failedFuture(new ValidationException("Invalid tagId parameter"));
     }
 
     if (version < -2) {
       return Future.failedFuture(new ValidationException("Invalid version parameter"));
     }
 
-
     final Future<Space> spaceFuture = getSpace(marker, spaceId);
     final Future<ChangesetsStatisticsResponse> changesetFuture = ChangesetApi.getChangesetStatistics(marker, Future::succeededFuture, spaceId);
     final Future<Tag> tagFuture = Service.tagConfigClient.getTag(marker, tagId, spaceId)
@@ -172,10 +170,6 @@ public static Future<Tag> createTag(Marker marker, String spaceId, String tagId,
     });
   }
 
-  public static boolean invalidTagId(String tagId) {
-    return StringUtils.isBlank(tagId) || !Pattern.matches("^[a-zA-Z][a-zA-Z0-9_-]+$", tagId) || "HEAD".equals(tagId) || tagId.length() > 50;
-  }
-
   // TODO auth
   public static Future<Tag> deleteTag(Marker marker, String spaceId, String tagId) {
     if (spaceId == null || tagId == null) {

diff --git a/xyz-hub-service/src/main/java/com/here/xyz/hub/task/FeatureTaskHandler.java b/xyz-hub-service/src/main/java/com/here/xyz/hub/task/FeatureTaskHandler.java
@@ -776,25 +776,24 @@ public static <T extends FeatureTask> void checkSpaceIsActive(T task, Callback<T
   }
 
   static <X extends FeatureTask> void resolveVersionRef(final X task, final Callback<X> callback) {
-    if (!(task.getEvent() instanceof SelectiveEvent<?> event)) {
+    if (!(task.getEvent() instanceof SelectiveEvent event)) {
       callback.call(task);
       return;
     }
 
-    if (event.getRef() == null || event.getRef().isResolved()) {
+    if (event.getRef() == null || !event.getRef().isTag()) {
       callback.call(task);
       return;
     }
 
-    TagConfigClient.getInstance().getTag(task.getMarker(), event.getRef().getVersionRef(), task.space.getId())
+    TagConfigClient.getInstance().getTag(task.getMarker(), event.getRef().getTag(), task.space.getId())
         .onSuccess(tag -> {
           if (tag == null) {
-            callback.exception(new HttpException(BAD_REQUEST, "Version ref not found: " + event.getRef().getVersionRef()));
+            callback.exception(new HttpException(BAD_REQUEST, "Version ref not found: " + event.getRef().getTag()));
             return;
           }
 
-          event.getRef().setResolved(true);
-          event.getRef().setVersion(tag.getVersion());
+          event.setRef(new Ref(tag.getVersion()));
           callback.call(task);
         })
         .onFailure(t -> {