Skip to content

Commit db52abe

Browse files
mujammil10mujammil10
committed
DS-1747: Support tags on branches
- Allow creation of tags with branch as ref - Add versionRef field to Tag config - Use single method to correctly resolve ref as tag or branch during feature read Signed-off-by: Ansari, Mujammil <mujammil.ansari@here.com>
1 parent 83d320d commit db52abe

File tree

9 files changed

+139
-99
lines changed

9 files changed

+139
-99
lines changed

‎xyz-hub-service/src/main/java/com/here/xyz/hub/config/dynamo/DynamoTagConfigClient.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import com.amazonaws.services.dynamodbv2.model.ReturnValue;
3232
import com.here.xyz.XyzSerializable;
3333
import com.here.xyz.hub.config.TagConfigClient;
34+
import com.here.xyz.models.hub.Ref;
3435
import com.here.xyz.models.hub.Tag;
3536
import com.here.xyz.util.service.Core;
3637
import com.here.xyz.util.service.aws.dynamo.DynamoClient;
@@ -225,6 +226,7 @@ private static List<Tag> tagDataToTags(List<Map<String, AttributeValue>> items)
225226
.withId(tagData.get("id").getS())
226227
.withSpaceId(tagData.get("spaceId").getS())
227228
.withVersion(Long.parseLong(tagData.get("version").getN()))
229+
.withVersionRef(new Ref(tagData.get("versionRef").getS()))
228230
.withSystem( tagData.get("system") != null ? tagData.get("system").getBOOL() : false )
229231
.withDescription(tagData.get("description") != null ? tagData.get("description").getS() : "")
230232
.withAuthor(tagData.get("author") != null ? tagData.get("author").getS() : "system")

‎xyz-hub-service/src/main/java/com/here/xyz/hub/rest/TagApi.java‎

Lines changed: 41 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package com.here.xyz.hub.rest;
2121

2222
import static com.here.xyz.hub.rest.ApiParam.Path.INCLUDE_SYSTEM_TAGS;
23+
import static com.here.xyz.models.hub.Ref.HEAD;
2324
import static io.netty.handler.codec.http.HttpResponseStatus.NOT_FOUND;
2425

2526
import com.fasterxml.jackson.core.JsonProcessingException;
@@ -28,6 +29,9 @@
2829
import com.here.xyz.hub.connectors.models.Space;
2930
import com.here.xyz.hub.rest.ApiParam.Path;
3031
import com.here.xyz.hub.rest.ApiParam.Query;
32+
import com.here.xyz.hub.task.BranchHandler;
33+
import com.here.xyz.hub.task.FeatureTask;
34+
import com.here.xyz.models.hub.Ref;
3135
import com.here.xyz.models.hub.Tag;
3236
import com.here.xyz.responses.ChangesetsStatisticsResponse;
3337
import com.here.xyz.util.service.BaseHttpServerVerticle;
@@ -61,7 +65,7 @@ private void createTag(RoutingContext context) {
6165
getMarker(context),
6266
getSpaceId(context),
6367
tag.getId(),
64-
tag.getVersion(),
68+
tag.getVersionRef(),
6569
tag.isSystem(),
6670
tag.getDescription(),
6771
author,
@@ -119,19 +123,15 @@ private void updateTag(RoutingContext context) {
119123

120124
String author = BaseHttpServerVerticle.getAuthor(context);
121125
deserializeTag(context.body().asString())
122-
.compose(tag -> updateTag(marker, spaceId, tagId, tag.getVersion(), author, tag.getDescription()))
126+
.compose(tag -> updateTag(marker, spaceId, tagId, tag.getVersionRef(), author, tag.getDescription()))
123127
.onSuccess(tag -> sendResponse(context, HttpResponseStatus.OK.code(), tag))
124128
.onFailure(t -> sendErrorResponse(context, t));
125129
}
126130

127-
public static Future<Tag> updateTag(Marker marker, String spaceId, String tagId, long version, String author, String description) {
131+
public static Future<Tag> updateTag(Marker marker, String spaceId, String tagId, Ref versionRef, String author, String description) {
128132
if (spaceId == null)
129133
return Future.failedFuture(new ValidationException("Invalid parameter"));
130134

131-
//FIXME: Neither -2 nor -1 are valid versions
132-
if (version < -2)
133-
return Future.failedFuture(new ValidationException("Invalid version parameter"));
134-
135135
if (!Tag.isDescriptionValid(description))
136136
return Future.failedFuture(new ValidationException("Invalid description parameter, description must be less than 255 characters"));
137137

@@ -140,24 +140,24 @@ public static Future<Tag> updateTag(Marker marker, String spaceId, String tagId,
140140
.compose(loadedTag -> loadedTag == null
141141
? Future.failedFuture(new HttpException(NOT_FOUND, "Tag " + tagId + " not found"))
142142
: Future.succeededFuture(loadedTag))
143-
.compose(loadedTag -> Service.tagConfigClient.storeTag(marker, new Tag()
144-
.withId(tagId)
145-
.withSpaceId(spaceId)
146-
.withVersion(version)
147-
.withSystem(loadedTag.isSystem())
148-
.withDescription(description == null ? loadedTag.getDescription() : description))
149-
.map(loadedTag))
143+
.compose(loadedTag -> resolveVersionRef(marker, spaceId, versionRef)
144+
.compose(resolvedVersionRef -> Service.tagConfigClient.storeTag(marker, new Tag()
145+
.withId(tagId)
146+
.withSpaceId(spaceId)
147+
.withVersionRef(resolvedVersionRef)
148+
.withSystem(loadedTag.isSystem())
149+
.withDescription(description == null ? loadedTag.getDescription() : description))
150+
.map(loadedTag.withVersionRef(resolvedVersionRef))))
150151
.map(loadedTag -> loadedTag
151-
.withVersion(version)
152152
.withDescription(description == null ? loadedTag.getDescription() : description));
153153
}
154154

155155
public static Future<Tag> createTag(Marker marker, String spaceId, String tagId, String author) {
156-
return createTag(marker, spaceId, tagId, -2, true, "", author, Core.currentTimeMillis());
156+
return createTag(marker, spaceId, tagId, new Ref(HEAD), true, "", author, Core.currentTimeMillis());
157157
}
158158

159159
// TODO auth
160-
public static Future<Tag> createTag(Marker marker, String spaceId, String tagId, long version, boolean system,
160+
public static Future<Tag> createTag(Marker marker, String spaceId, String tagId, Ref versionRef, boolean system,
161161
String description, String author, long createdAt) {
162162
if (spaceId == null) {
163163
return Future.failedFuture(new ValidationException("Invalid parameter"));
@@ -173,19 +173,15 @@ public static Future<Tag> createTag(Marker marker, String spaceId, String tagId,
173173
);
174174
}
175175

176-
if (version < -2) {
177-
return Future.failedFuture(new ValidationException("Invalid version parameter"));
178-
}
179-
180176
final Future<Space> spaceFuture = getSpaceIfActive(marker, spaceId);
181-
final Future<ChangesetsStatisticsResponse> changesetFuture = ChangesetApi.getChangesetStatistics(marker, Future::succeededFuture, spaceId);
177+
final Future<Ref> resolveVersionRef = resolveVersionRef(marker, spaceId, versionRef);
182178
final Future<Void> checkExistingAlias = checkExistingAlias(marker, spaceId, tagId);
183179

184-
return Future.all(spaceFuture, changesetFuture, checkExistingAlias).compose(cf -> {
180+
return Future.all(spaceFuture, resolveVersionRef, checkExistingAlias).compose(cf -> {
185181
final Tag tag = new Tag()
186182
.withId(tagId)
187183
.withSpaceId(spaceId)
188-
.withVersion(version == -2 ? changesetFuture.result().getMaxVersion() : version)
184+
.withVersionRef(resolveVersionRef.result())
189185
.withSystem(system)
190186
.withDescription(description)
191187
.withAuthor(author)
@@ -204,13 +200,18 @@ public static Future<Tag> deleteTag(Marker marker, String spaceId, String tagId)
204200
.compose(none -> Service.tagConfigClient.deleteTag(marker, tagId, spaceId));
205201
}
206202

203+
//TODO: Check why does this return a Future ?
207204
private static Future<Tag> deserializeTag(String body) {
208205
if (StringUtils.isBlank(body)) {
209206
return Future.failedFuture(new ValidationException("Unable to parse body"));
210207
}
211208

212209
try {
213-
return Future.succeededFuture(XyzSerializable.deserialize(body, Tag.class));
210+
Tag tag = XyzSerializable.deserialize(body, Tag.class);
211+
if (tag.getVersionRef() == null) {
212+
tag.setVersionRef(tag.getVersion() == -2 ? new Ref(HEAD) : new Ref(tag.getVersion()));
213+
}
214+
return Future.succeededFuture(tag);
214215
} catch (JsonProcessingException e) {
215216
return Future.failedFuture(new ValidationException("Unable to parse body"));
216217
}
@@ -226,6 +227,21 @@ private static Future<Space> getSpaceIfActive(Marker marker, String spaceId) {
226227
: Future.succeededFuture(s));
227228
}
228229

230+
private static Future<Ref> resolveVersionRef(Marker marker, String spaceId, Ref versionRef) {
231+
if (versionRef.isTag())
232+
throw new IllegalArgumentException("A tag cannot be used as Ref");
233+
234+
Future<Ref> future = Future.succeededFuture(versionRef);
235+
if (!versionRef.isMainBranch()) {
236+
future = FeatureTask.getReferencedBranch(spaceId, versionRef).map(versionRef);
237+
}
238+
239+
if (versionRef.isHead()) {
240+
future = future.compose(v -> BranchHandler.resolveRefHeadVersion(marker, spaceId, versionRef));
241+
}
242+
return future;
243+
}
244+
229245
private static String getAuthor(Future<Tag> tagFuture, String author) {
230246
return tagFuture.result().getAuthor().isEmpty() ? author : tagFuture.result().getAuthor();
231247
}

‎xyz-hub-service/src/main/java/com/here/xyz/hub/task/BranchHandler.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ private static Future<Ref> resolveRef(Marker marker, String spaceId, Ref ref) {
183183
return resolveRefHeadVersion(marker, spaceId, ref);
184184
}
185185

186-
private static Future<Ref> resolveRefHeadVersion(Marker marker, String spaceId, Ref ref) {
186+
public static Future<Ref> resolveRefHeadVersion(Marker marker, String spaceId, Ref ref) {
187187
if (ref.isHead())
188188
return FeatureQueryApi.getStatistics(marker, spaceId, EXTENSION, ref, true, false)
189189
.map(statistics -> new Ref(ref.getBranch() + ":" + statistics.getMaxVersion().getValue()));

‎xyz-hub-service/src/main/java/com/here/xyz/hub/task/FeatureTask.java‎

Lines changed: 31 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import com.here.xyz.hub.auth.Authorization;
5050
import com.here.xyz.hub.auth.FeatureAuthorization;
5151
import com.here.xyz.hub.config.BranchConfigClient;
52+
import com.here.xyz.hub.config.TagConfigClient;
5253
import com.here.xyz.hub.connectors.RpcClient;
5354
import com.here.xyz.hub.connectors.models.Connector;
5455
import com.here.xyz.hub.connectors.models.Space;
@@ -147,13 +148,25 @@ private FeatureTask(T event, RoutingContext context, ApiResponseType responseTyp
147148
this.requestBodySize = requestBodySize;
148149
}
149150

150-
//TODO: Merge into resolveVersionRef
151-
void resolveBranch(final X task, final Callback<X> callback) {
151+
void resolveVersionRef(final X task, final Callback<X> callback) {
152152
try {
153153
if (task.getEvent() instanceof ContextAwareEvent<?> event) {
154-
resolveBranchFor(event, task.space)
155-
.onFailure(callback::exception)
156-
.onSuccess(v -> callback.call(task));
154+
Future<Void> future = Future.succeededFuture();
155+
if (event.getRef().isTag()) {
156+
future = TagConfigClient.getInstance().getTag(task.getMarker(), event.getRef().getTag(), task.space.getId())
157+
.compose(tag -> {
158+
if (tag == null) {
159+
//It could be a branch
160+
event.setRef(Ref.fromBranchId(event.getRef().getTag()));
161+
} else {
162+
event.setRef(tag.getVersionRef());
163+
}
164+
return Future.succeededFuture();
165+
});
166+
}
167+
future.compose(v -> resolveBranchFor(event, task.space))
168+
.onFailure(callback::exception)
169+
.onSuccess(v -> callback.call(task));
157170
} else
158171
callback.call(task);
159172
} catch (Exception e) {
@@ -162,27 +175,11 @@ void resolveBranch(final X task, final Callback<X> callback) {
162175
}
163176

164177
public static Future<Void> resolveBranchFor(ContextAwareEvent<?> event, Space space) {
165-
if (event.getRef() == null || (event.getRef().isMainBranch() && !event.getRef().isTag()))
178+
if (event.getRef() == null || event.getRef().isMainBranch())
166179
return Future.succeededFuture();
167180

168-
Ref branchRef = event.getRef().isTag() ? Ref.fromBranchId(event.getRef().getTag()) : event.getRef();
169-
Future<Branch> future = getReferencedBranch(space.getId(), branchRef);
170-
if (event.getRef().isTag()) {
171-
// Check if it's a branch or a tag
172-
future = future.compose(branch -> {
173-
if (branch != null)
174-
event.setRef(branchRef);
175-
return Future.succeededFuture(branch);
176-
})
177-
.recover(t -> {
178-
// If the branch does not exist, then it could be a normal tag
179-
if (t instanceof HttpException e && e.status == NOT_FOUND)
180-
return Future.succeededFuture();
181-
return Future.failedFuture(t);
182-
});
183-
}
184-
185-
return future.compose(referencedBranch -> {
181+
return getReferencedBranch(space.getId(), event.getRef())
182+
.compose(referencedBranch -> {
186183
if (referencedBranch != null) {
187184
LinkedList<Ref> branchPath = new LinkedList<>(referencedBranch.getBranchPath());
188185
event.withNodeId(referencedBranch.getNodeId())
@@ -211,7 +208,7 @@ public static Future<Branch> getReferencedBranch(String spaceId, Ref ref) {
211208
return BranchConfigClient.getInstance().load(spaceId, ref.getBranch())
212209
.compose(branch -> {
213210
if (branch == null)
214-
return Future.failedFuture(new HttpException(NOT_FOUND, "Branch \"" + ref.getBranch() + "\" was not found on resource \"" + spaceId + "\"."));
211+
return Future.failedFuture(new HttpException(NOT_FOUND, "Tag or Branch \"" + ref.getBranch() + "\" was not found on resource \"" + spaceId + "\"."));
215212
return Future.succeededFuture(branch);
216213
});
217214
}
@@ -342,8 +339,7 @@ public GeometryQuery(GetFeaturesByGeometryEvent event, RoutingContext context, A
342339
public TaskPipeline<GeometryQuery> createPipeline() {
343340
return TaskPipeline.create(this)
344341
.then(FeatureTaskHandler::resolveSpace)
345-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef and move back again after checkImmutability
346-
.then(FeatureTaskHandler::resolveVersionRef)
342+
.then(this::resolveVersionRef)
347343
.then(this::resolveRefSpace)
348344
.then(this::resolveRefConnector)
349345
.then(Authorization::authorizeComposite)
@@ -473,8 +469,7 @@ public BBoxQuery(GetFeaturesByBBoxEvent event, RoutingContext context, ApiRespon
473469
public TaskPipeline<BBoxQuery> createPipeline() {
474470
return TaskPipeline.create(this)
475471
.then(FeatureTaskHandler::resolveSpace)
476-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef and move back again after checkImmutability
477-
.then(FeatureTaskHandler::resolveVersionRef)
472+
.then(this::resolveVersionRef)
478473
.then(Authorization::authorizeComposite)
479474
.then(FeatureAuthorization::authorize)
480475
.then(FeatureTaskHandler::checkImmutability)
@@ -504,8 +499,7 @@ public TileQuery(GetFeaturesByTileEvent event, RoutingContext context, ApiRespon
504499
public TaskPipeline<TileQuery> createPipeline() {
505500
return TaskPipeline.create(this)
506501
.then(FeatureTaskHandler::resolveSpace)
507-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef and move back again after checkImmutability
508-
.then(FeatureTaskHandler::resolveVersionRef)
502+
.then(this::resolveVersionRef)
509503
.then(Authorization::authorizeComposite)
510504
.then(FeatureAuthorization::authorize)
511505
.then(FeatureTaskHandler::checkImmutability)
@@ -540,8 +534,7 @@ public IdsQuery(GetFeaturesByIdEvent event, RoutingContext context, ApiResponseT
540534
public TaskPipeline<IdsQuery> createPipeline() {
541535
return TaskPipeline.create(this)
542536
.then(FeatureTaskHandler::resolveSpace)
543-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef and move back again after auth
544-
.then(FeatureTaskHandler::resolveVersionRef)
537+
.then(this::resolveVersionRef)
545538
.then(Authorization::authorizeComposite)
546539
.then(FeatureAuthorization::authorize)
547540
.then(FeatureTaskHandler::checkImmutability)
@@ -562,8 +555,7 @@ public IterateQuery(IterateFeaturesEvent event, RoutingContext context, ApiRespo
562555
public TaskPipeline<IterateQuery> createPipeline() {
563556
return TaskPipeline.create(this)
564557
.then(FeatureTaskHandler::resolveSpace)
565-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef and move back again after auth
566-
.then(FeatureTaskHandler::resolveVersionRef)
558+
.then(this::resolveVersionRef)
567559
.then(Authorization::authorizeComposite)
568560
.then(FeatureAuthorization::authorize)
569561
.then(FeatureTaskHandler::checkImmutability)
@@ -584,8 +576,7 @@ public SearchQuery(SearchForFeaturesEvent<?> event, RoutingContext context, ApiR
584576
public TaskPipeline<SearchQuery> createPipeline() {
585577
return TaskPipeline.create(this)
586578
.then(FeatureTaskHandler::resolveSpace)
587-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef and move back again after auth
588-
.then(FeatureTaskHandler::resolveVersionRef)
579+
.then(this::resolveVersionRef)
589580
.then(Authorization::authorizeComposite)
590581
.then(FeatureAuthorization::authorize)
591582
.then(FeatureTaskHandler::checkImmutability)
@@ -610,7 +601,7 @@ public TaskPipeline<GetStatistics> createPipeline() {
610601
.then(FeatureTaskHandler::resolveSpace)
611602
.then(Authorization::authorizeComposite)
612603
.then(FeatureAuthorization::authorize)
613-
.then(this::resolveBranch) //TODO: Merge into resolveVersionRef?
604+
.then(this::resolveVersionRef)
614605
.then(FeatureTaskHandler::readCache)
615606
.then(FeatureTaskHandler::invoke)
616607
.then(FeatureTaskHandler::convertResponse)
@@ -639,7 +630,7 @@ public void onPreProcessed(ModifySpaceEvent event) {
639630
public TaskPipeline<ModifySpaceQuery> createPipeline() {
640631
return TaskPipeline.create(this)
641632
.then(FeatureTaskHandler::resolveSpace)
642-
.then(this::resolveBranch)
633+
.then(this::resolveVersionRef)
643634
.then(SpaceTaskHandler::invokeConditionally);
644635
}
645636
}
@@ -699,7 +690,7 @@ public TaskPipeline<ConditionalOperation> createPipeline() {
699690
.then(FeatureTaskHandler::resolveSpace)
700691
.then(FeatureTaskHandler::registerRequestMemory)
701692
.then(FeatureTaskHandler::throttle)
702-
.then(this::resolveBranch)
693+
.then(this::resolveVersionRef)
703694
.then(FeatureTaskHandler::injectSpaceParams)
704695
.then(FeatureTaskHandler::checkPreconditions)
705696
.then(FeatureTaskHandler::prepareModifyFeatureOp)

‎xyz-hub-service/src/main/java/com/here/xyz/hub/task/FeatureTaskHandler.java‎

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -767,38 +767,6 @@ private static <T extends FeatureTask> CompletableFuture<XyzResponse> executePro
767767
return f;
768768
}
769769

770-
static <X extends FeatureTask> void resolveVersionRef(final X task, final Callback<X> callback) {
771-
if (!(task.getEvent() instanceof SelectiveEvent event)) {
772-
callback.call(task);
773-
return;
774-
}
775-
776-
if (event.getRef() == null || !event.getRef().isTag()) {
777-
callback.call(task);
778-
return;
779-
}
780-
781-
TagConfigClient.getInstance().getTag(task.getMarker(), event.getRef().getTag(), task.space.getId())
782-
.compose(tag -> {
783-
if (tag == null) {
784-
return Future.failedFuture(new HttpException(NOT_FOUND, "Version ref not found: " + event.getRef().getTag()));
785-
}
786-
787-
try {
788-
event.setRef(new Ref(tag.getVersion()));
789-
} catch (InvalidRef e) {
790-
return Future.failedFuture(e);
791-
}
792-
793-
return Future.succeededFuture(tag);
794-
})
795-
.onSuccess(tag -> callback.call(task))
796-
.onFailure(t -> {
797-
logger.warn(task.getMarker(), "Unable to resolve version ref.", t);
798-
callback.exception(t);
799-
});
800-
}
801-
802770
private static class RpcContextHolder {
803771
RpcContext rpcContext;
804772
}

‎xyz-hub-test/src/test/java/com/here/xyz/hub/rest/BranchFeatureApiIT.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,20 @@ else if (endpoint.equals("spatial"))
162162
.body("features.id", containsInAnyOrder("main0", "f1"));
163163
}
164164

165+
@Test
166+
public void readBranchFeaturesFromTag() throws Exception {
167+
createBranch(SPACE_ID, B1_MAIN, null)
168+
.body("id", equalTo(B1_MAIN));
169+
170+
// Add new features to branch
171+
addFeaturesToBranchAndVerify(B1_MAIN, Set.of("f1", "f2"), Set.of("main0", "f1", "f2"));
172+
173+
String tag = "tag_b1";
174+
createTag(SPACE_ID, tag, B1_MAIN + ":2");
175+
176+
addFeaturesToBranchAndVerify(tag, Set.of(), Set.of("main0", "f1"));
177+
}
178+
165179
private Set<String> extractFeatureIds(FeatureCollection featureCollection) throws JsonProcessingException {
166180
if (featureCollection == null || featureCollection.getFeatures() == null) return Set.of();
167181
return featureCollection.getFeatures()

0 commit comments

Comments
 (0)