From 5d0d6406ecb794908eea018a42e2b244387357b0 Mon Sep 17 00:00:00 2001 From: Kaveh Shahedi Date: Wed, 20 Nov 2024 10:24:09 -0500 Subject: [PATCH] server: Use randomized UUID for bookmarks To have persistent UUID for each bookmark, we now use randomly generated UUIDs instead of generating them based on the bookmark's properties (e.g., name or start/end times). The new system allows to keep the bookmarks' UUIDs consistent even if their properties changed (i.e., through PUT endpoint) [Changed] Bookmark's UUID generation is now randomized instead of property-based Signed-off-by: Kaveh Shahedi --- .../services/BookmarkManagerServiceTest.java | 102 +++++------------- .../core/tests/stubs/BookmarkModelStub.java | 7 +- .../core/services/BookmarkManagerService.java | 66 +++++------- 3 files changed, 56 insertions(+), 119 deletions(-) diff --git a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java index a8a71425..0feba97b 100644 --- a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java +++ b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/services/BookmarkManagerServiceTest.java @@ -15,7 +15,10 @@ import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; +import java.util.UUID; import javax.ws.rs.client.Entity; import javax.ws.rs.client.WebTarget; @@ -177,30 +180,35 @@ public void testCreateBookmarkNoEndTime() { * Test the creation of a bookmark with a repetitive data. */ @Test - public void testCreateBookmarkRepetitiveData() { + public void testCreateIdenticalBookmarks() { WebTarget application = getApplicationEndpoint(); WebTarget bookmarkTarget = application.path(EXPERIMENTS) .path(experiment.getUUID().toString()) .path(BOOKMARKS); - // Create first bookmark - Map parameters = new HashMap<>(); - parameters.put(NAME, BOOKMARK.getName()); - parameters.put(START, START_TIME); - parameters.put(END, END_TIME); - - Response response = bookmarkTarget.request().post(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - BookmarkModelStub firstBookmark = response.readEntity(BookmarkModelStub.class); - assertEquals("First bookmark creation should succeed", 200, response.getStatus()); - assertNotNull("First bookmark should not be null", firstBookmark); + Set uuids = new HashSet<>(); + for (int i = 0; i < 3; i++) { + Map parameters = new HashMap<>(); + parameters.put(NAME, BOOKMARK.getName()); + parameters.put(START, START_TIME); + parameters.put(END, END_TIME); + + Response response = bookmarkTarget.request().post(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); + assertEquals("Response status should be 200", 200, response.getStatus()); + + BookmarkModelStub expStub = response.readEntity(BookmarkModelStub.class); + assertNotNull("Response body should not be null", expStub); + assertEquals("Bookmark name should match", BOOKMARK.getName(), expStub.getName()); + assertEquals("Start time should match", BOOKMARK.getStart(), expStub.getStart()); + assertEquals("End time should match", BOOKMARK.getEnd(), expStub.getEnd()); + assertNotNull("UUID should not be null", expStub.getUUID()); + + // Check if the UUID is unique + assertFalse("UUID should be unique", uuids.contains(expStub.getUUID())); + uuids.add(expStub.getUUID()); + } - response = bookmarkTarget.request().post(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - assertEquals("Should return conflict for duplicate data", 409, response.getStatus()); - // Verify the original bookmark wasn't modified - Response getResponse = bookmarkTarget.path(firstBookmark.getUUID().toString()).request().get(); - BookmarkModelStub retrievedBookmark = getResponse.readEntity(BookmarkModelStub.class); - assertEquals("Original bookmark should remain unchanged", firstBookmark, retrievedBookmark); } /** @@ -326,71 +334,13 @@ public void testUpdateBookmark() { assertEquals("Update should succeed", 200, response.getStatus()); BookmarkModelStub updatedBookmark = response.readEntity(BookmarkModelStub.class); - assertNotNull("Updated bookmark should not be null", updatedBookmark); - assertNotEquals("UUID should be different", originalBookmark.getUUID(), updatedBookmark.getUUID()); + assertEquals("UUID should be the same", originalBookmark.getUUID(), updatedBookmark.getUUID()); assertEquals("Name should be updated", "Updated Name", updatedBookmark.getName()); assertEquals("Start time should be updated", START_TIME + 5, updatedBookmark.getStart()); assertEquals("End time should be updated", END_TIME + 5, updatedBookmark.getEnd()); } - /** - * Test updating a bookmark. - */ - @Test - public void testUpdateBookmarkRepetitiveData() { - WebTarget application = getApplicationEndpoint(); - WebTarget bookmarkTarget = application.path(EXPERIMENTS) - .path(experiment.getUUID().toString()) - .path(BOOKMARKS); - - // Create initial bookmark - Map parameters = new HashMap<>(); - parameters.put(NAME, BOOKMARK.getName()); - parameters.put(START, START_TIME); - parameters.put(END, END_TIME); - - Response response = bookmarkTarget.request().post(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - BookmarkModelStub originalBookmark = response.readEntity(BookmarkModelStub.class); - assertEquals("Initial bookmark creation should succeed", 200, response.getStatus()); - - // Create a secondary bookmark - parameters.put(NAME, BOOKMARK.getName()); - parameters.put(START, START_TIME + 1); - parameters.put(END, END_TIME + 1); - - response = bookmarkTarget.request().post(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - BookmarkModelStub secondaryBookmark = response.readEntity(BookmarkModelStub.class); - assertEquals("Secondary bookmark creation should succeed", 200, response.getStatus()); - - // Test updating non-existent bookmark - WebTarget nonExistentTarget = bookmarkTarget.path("non-existent-uuid"); - Response nonExistentResponse = nonExistentTarget.request() - .put(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - assertEquals("Should return 404 for non-existent bookmark", 404, nonExistentResponse.getStatus()); - - // Test updating with invalid parameters - parameters.put(START, END_TIME); - parameters.put(END, START_TIME); - Response invalidResponse = bookmarkTarget.path(originalBookmark.getUUID().toString()) - .request() - .put(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - assertEquals("Should return 400 for invalid parameters", 400, invalidResponse.getStatus()); - - // Test successful update - parameters.put(START, START_TIME + 1); - parameters.put(END, END_TIME + 1); - - response = bookmarkTarget.path(originalBookmark.getUUID().toString()) - .request() - .put(Entity.json(new QueryParameters(parameters, Collections.emptyList()))); - BookmarkModelStub updatedBookmark = response.readEntity(BookmarkModelStub.class); - assertEquals("Should return conflict for duplicate name", 409, response.getStatus()); - assertEquals("Name should be as before", secondaryBookmark.getName(), updatedBookmark.getName()); - assertEquals("Start time should be as before", secondaryBookmark.getStart(), updatedBookmark.getStart()); - assertEquals("End time should be as before", secondaryBookmark.getEnd(), updatedBookmark.getEnd()); - } - /** * Test the deletion of a bookmark with various scenarios. */ diff --git a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/stubs/BookmarkModelStub.java b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/stubs/BookmarkModelStub.java index 9c510f11..ad3c45c2 100644 --- a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/stubs/BookmarkModelStub.java +++ b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests/src/org/eclipse/tracecompass/incubator/trace/server/jersey/rest/core/tests/stubs/BookmarkModelStub.java @@ -12,7 +12,6 @@ package org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core.tests.stubs; import java.io.Serializable; -import java.nio.charset.Charset; import java.util.Objects; import java.util.UUID; @@ -70,11 +69,7 @@ public BookmarkModelStub( * end time */ public BookmarkModelStub(String name, long start, long end) { - this(getUUID(name), name, start, end); - } - - private static UUID getUUID(String name) { - return UUID.nameUUIDFromBytes(Objects.requireNonNull(name.getBytes(Charset.defaultCharset()))); + this(UUID.randomUUID(), name, start, end); } /** diff --git a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java index 677fc6ff..2ac2c26f 100644 --- a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java +++ b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/services/BookmarkManagerService.java @@ -17,7 +17,6 @@ import static org.eclipse.tracecompass.incubator.internal.trace.server.jersey.rest.core.services.EndpointConstants.NO_SUCH_EXPERIMENT; import static org.eclipse.tracecompass.incubator.internal.trace.server.jersey.rest.core.services.EndpointConstants.BKM; -import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -197,28 +196,21 @@ public Response createBookmark( return Response.status(Status.NOT_FOUND).entity(NO_SUCH_EXPERIMENT).build(); } - String name = Objects.requireNonNull((String) parameters.get(BOOKMARK_NAME)); - long start = Objects.requireNonNull((Number) parameters.get(BOOKMARK_START)).longValue(); - long end = Objects.requireNonNull((Number) parameters.get(BOOKMARK_END)).longValue(); - - String uuidReference = name + start + end; - UUID bookmarkUUID = UUID.nameUUIDFromBytes(Objects.requireNonNull(uuidReference.getBytes(Charset.defaultCharset()))); - - Bookmark bookmark = new Bookmark(bookmarkUUID, name, start, end); - IFile editorFile = TmfTraceManager.getInstance().getTraceEditorFile(experiment); if (editorFile == null) { return Response.status(Status.NOT_FOUND).entity(EndpointConstants.BOOKMARK_NOT_FOUND).build(); } try { - IMarker[] markers = findBookmarkMarkers(editorFile); - if (bookmarkExists(markers, bookmark.getUUID())) { - return Response.status(Status.CONFLICT).entity(bookmark).build(); - } + String name = Objects.requireNonNull((String) parameters.get(BOOKMARK_NAME)); + long start = Objects.requireNonNull((Number) parameters.get(BOOKMARK_START)).longValue(); + long end = Objects.requireNonNull((Number) parameters.get(BOOKMARK_END)).longValue(); + UUID uuid = generateUUID(editorFile); - IMarker newMarker = editorFile.createMarker(IMarker.BOOKMARK); - setMarkerAttributes(newMarker, bookmark); + Bookmark bookmark = new Bookmark(uuid, name, start, end); + + IMarker marker = editorFile.createMarker(IMarker.BOOKMARK); + setMarkerAttributes(marker, bookmark); return Response.ok(bookmark).build(); } catch (CoreException e) { Activator.getInstance().logError("Failed to create bookmark", e); //$NON-NLS-1$ @@ -268,20 +260,17 @@ public Response updateBookmark( return Response.status(Status.NOT_FOUND).entity(NO_SUCH_EXPERIMENT).build(); } - String name = Objects.requireNonNull((String) parameters.get(BOOKMARK_NAME)); - long start = Objects.requireNonNull((Number) parameters.get(BOOKMARK_START)).longValue(); - long end = Objects.requireNonNull((Number) parameters.get(BOOKMARK_END)).longValue(); - - String uuidReference = name + start + end; - String newBookmarkUUID = UUID.nameUUIDFromBytes(Objects.requireNonNull(uuidReference.getBytes(Charset.defaultCharset()))).toString(); - - Bookmark bookmark = new Bookmark(UUID.fromString(newBookmarkUUID), name, start, end); - IFile editorFile = TmfTraceManager.getInstance().getTraceEditorFile(experiment); if (editorFile == null) { return Response.status(Status.NOT_FOUND).entity(EndpointConstants.BOOKMARK_NOT_FOUND).build(); } + String name = Objects.requireNonNull((String) parameters.get(BOOKMARK_NAME)); + long start = Objects.requireNonNull((Number) parameters.get(BOOKMARK_START)).longValue(); + long end = Objects.requireNonNull((Number) parameters.get(BOOKMARK_END)).longValue(); + + Bookmark bookmark = new Bookmark(bookmarkUUID, name, start, end); + try { IMarker[] markers = findBookmarkMarkers(editorFile); IMarker marker = findMarkerByUUID(markers, bookmarkUUID); @@ -289,11 +278,6 @@ public Response updateBookmark( return Response.status(Status.NOT_FOUND).entity(EndpointConstants.BOOKMARK_NOT_FOUND).build(); } - IMarker duplicateMarker = findMarkerByUUID(markers, UUID.fromString(newBookmarkUUID)); - if (duplicateMarker != null) { - return Response.status(Status.CONFLICT).entity(bookmark).build(); - } - setMarkerAttributes(marker, bookmark); return Response.ok(bookmark).build(); } catch (CoreException e) { @@ -352,6 +336,21 @@ public Response deleteBookmark( } } + /** + * Generate a random UUID for a new bookmark + */ + private static UUID generateUUID(IFile editorFile) throws CoreException { + IMarker[] markers = findBookmarkMarkers(editorFile); + while (true) { + UUID uuid = UUID.randomUUID(); + + // Check if the UUID hasn't been used yet + if (findMarkerByUUID(markers, uuid) == null) { + return uuid; + } + } + } + /** * Find all bookmark markers in the editor file */ @@ -450,11 +449,4 @@ private static List markersToBookmarks(IMarker[] markers) { } return bookmarks; } - - /** - * Check if a bookmark with the same UUID already exists - */ - private static boolean bookmarkExists(IMarker[] markers, UUID bookmarkUUID) { - return findMarkerByUUID(markers, bookmarkUUID) != null; - } } \ No newline at end of file