Add IdField for consistent id assignment

This change is supposed to solve the problem where the same id may occur in both the payload and the actual table column.

docs/en/development/storage.md

@@ -392,6 +392,40 @@ public class GroupDeletionNotifier implements StoreDeletionNotifier {
 }
 ```
 
+## Handling of IDs
+
+If you have an entity that has an id field, you can use the `@Id` annotation to mark this field as the ID of the entity.
+This field must be of type `String`. If such a field is present, the store will automatically use it as the ID of the
+entity. This means that
+
+- if an entity is stored using the put method without explicit ID parameter (`DataStore#put(T)`), the store will check
+  if the ID field has a non-null value. If this is the case, this value will be used as the ID. If the ID field is
+  null, a new ID will be generated and assigned to the annotated field of the entity.
+- if an entity is stored using the put method with an explicit ID parameter (`DataStore#put(String, T)`), this ID
+  will be used to store the entity. The ID field of the entity will be set with this given ID.
+
+Please note that if you change the ID field of an entity after it has been stored, the store will not automatically
+update the ID in the store. You must explicitly call the `put` method with the new ID to store the entity with the
+new ID and remove the old entry with the old ID.
+
+```java
+import lombok.Data;
+import sonia.scm.store.QueryableType;
+import sonia.scm.store.Id;
+
+@Data
+@QueryableType
+public class MyEntity {
+  @Id
+  private String id;
+  private String name;
+  private String alias;
+  private int age;
+  private List<String> tags;
+}
+```
+
+
 ## Update Steps
 
 Update steps can be used to update data in the database. The following example shows how to update all entities of a

gradle/changelog/id_annotation.yaml

@@ -0,0 +1,2 @@
+- type: added
+  description: Annotation for id fields in data objects

scm-core/src/main/java/sonia/scm/store/DataStore.java

@@ -29,31 +29,33 @@ import java.util.Map;
 public interface DataStore<T> extends MultiEntryStore<T> {
 
   /**
-   * Put a item with automatically generated id to the store.
-   *
+   * Put an item into the store. If the item has an attribute that is
+   * annotated with {@link Id}, then the value from this field will
+   * be taken as an id if it is not null. Otherwise, a new id will be
+   * generated and used.
    *
    * @param item item to store
    *
    * @return automatically generated id of the item
    */
-  public String put(T item);
+  String put(T item);
 
   /**
    * Put the item with the given id to the store.
-   *
+   * If the item has an attribute that is annotated with {@link Id},
+   * then this field will be set to the given id.
    *
    * @param id id of the item
    * @param item item to store
    */
-  public void put(String id, T item);
+  void put(String id, T item);
 
 
   /**
    * Returns a map of all stored items. The key of the map is the item id and 
    * the value is item.
    *
-   *
    * @return map of all stored items
    */
-  public Map<String, T> getAll();
+  Map<String, T> getAll();
 }

scm-core/src/main/java/sonia/scm/store/Id.java

@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2020 - present Cloudogu GmbH
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see https://www.gnu.org/licenses/.
+ */
+
+package sonia.scm.store;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
+
+/**
+ * Marks a field as id field. The value of this field will be used as id
+ * in a {@link DataStore}. The field must be of type {@code String}. Only one
+ * field in a class can be annotated with this annotation.
+ */
+@Retention(java.lang.annotation.RetentionPolicy.RUNTIME)
+@Target({ElementType.FIELD})
+public @interface Id {
+}

scm-persistence/src/main/java/sonia/scm/store/IdHandlerForStores.java

@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2020 - present Cloudogu GmbH
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see https://www.gnu.org/licenses/.
+ */
+
+package sonia.scm.store;
+
+import sonia.scm.security.KeyGenerator;
+
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import java.util.function.BiConsumer;
+import java.util.function.Function;
+
+import static java.lang.String.format;
+
+public class IdHandlerForStores<T> {
+
+  private static final Map<Class<?>, Optional<Field>> FIELD_CACHE = new HashMap<>();
+
+  private final KeyGenerator keyGenerator;
+
+  private final Function<T, String> idExtractor;
+  private final BiConsumer<T, String> idSetter;
+  private final BiConsumer<String, T> doPut;
+
+  @SuppressWarnings("unchecked")
+  public IdHandlerForStores(Class<T> clazz, KeyGenerator keyGenerator, BiConsumer<String, T> doPut) {
+    this.idExtractor = getIdExtractor(clazz);
+    this.idSetter = getIdSetter(clazz);
+    this.keyGenerator = keyGenerator;
+    this.doPut = doPut;
+  }
+
+  private static <T> Function<T, String> getIdExtractor(Class<?> clazz) {
+    return FIELD_CACHE.computeIfAbsent(clazz, IdHandlerForStores::getIdField)
+      .map(field -> (Function<T, String>) item -> {
+        try {
+          return (String) field.get(item);
+        } catch (IllegalAccessException e) {
+          throw new StoreException("Failed to get id from object", e);
+        }
+      })
+      .orElse(object -> null);
+  }
+
+  private static <T> BiConsumer<T, String> getIdSetter(Class<?> clazz) {
+    return FIELD_CACHE.computeIfAbsent(clazz, IdHandlerForStores::getIdField)
+      .map(field -> (BiConsumer<T, String>) (object, id) -> {
+        try {
+          field.set(object, id);
+        } catch (IllegalAccessException e) {
+          throw new StoreException("Failed to set id on object", e);
+        }
+      })
+      .orElse((object, id) -> {
+        // do nothing
+      });
+  }
+
+  private static Optional<Field> getIdField(Class<?> clazz) {
+    for (var field : clazz.getDeclaredFields()) {
+      if (field.isAnnotationPresent(Id.class)) {
+        if (field.getType() != String.class) {
+          throw new StoreException(format("The field '%s' annotated with @Id in class %s must be of type String.", field.getName(), clazz.getName()));
+        }
+        field.setAccessible(true);
+        return Optional.of(field);
+      }
+    }
+    if (clazz.getSuperclass() != null) {
+      return getIdField(clazz.getSuperclass());
+    }
+    return Optional.empty();
+  }
+
+  public String put(T item) {
+    String idFromItem = idExtractor.apply(item);
+    if (idFromItem == null) {
+      String id = keyGenerator.createKey();
+      put(id, item);
+      return id;
+    } else {
+      put(idFromItem, item);
+      return idFromItem;
+    }
+  }
+
+  public void put(String id, T item) {
+    idSetter.accept(item, id);
+    doPut.accept(id, item);
+  }
+}

scm-persistence/src/main/java/sonia/scm/store/file/JAXBConfigurationEntryStore.java

@@ -24,6 +24,7 @@ import org.slf4j.LoggerFactory;
 import sonia.scm.CopyOnWrite;
 import sonia.scm.security.KeyGenerator;
 import sonia.scm.store.ConfigurationEntryStore;
+import sonia.scm.store.IdHandlerForStores;
 import sonia.scm.xml.XmlStreams;
 import sonia.scm.xml.XmlStreams.AutoCloseableXMLReader;
 import sonia.scm.xml.XmlStreams.AutoCloseableXMLWriter;
@@ -44,19 +45,17 @@ class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V> {
   private static final String TAG_KEY = "key";
   private static final String TAG_VALUE = "value";
 
- 
   private static final Logger LOG = LoggerFactory.getLogger(JAXBConfigurationEntryStore.class);
 
-
   private final File file;
-  private final KeyGenerator keyGenerator;
   private final Class<V> type;
   private final TypedStoreContext<V> context;
   private final Map<String, V> entries = Maps.newHashMap();
 
+  private final IdHandlerForStores<V> idHandlerForStores;
+
   JAXBConfigurationEntryStore(File file, KeyGenerator keyGenerator, Class<V> type, TypedStoreContext<V> context) {
     this.file = file;
-    this.keyGenerator = keyGenerator;
     this.type = type;
     this.context = context;
     // initial load
@@ -65,6 +64,7 @@ class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V> {
         load();
       }
     }).withLockedFileForRead(file);
+    this.idHandlerForStores = new IdHandlerForStores<>(type, keyGenerator, this::doPut);
   }
 
   @Override
@@ -79,15 +79,15 @@ class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V> {
 
   @Override
   public String put(V item) {
-    String id = keyGenerator.createKey();
-
-    put(id, item);
-
-    return id;
+    return idHandlerForStores.put(item);
   }
 
   @Override
   public void put(String id, V item) {
+    idHandlerForStores.put(id, item);
+  }
+
+  private void doPut(String id, V item) {
     LOG.debug("put item {} to configuration store", id);
 
     execute(() -> {

scm-persistence/src/main/java/sonia/scm/store/file/JAXBDataStore.java

@@ -25,6 +25,7 @@ import org.slf4j.LoggerFactory;
 import sonia.scm.CopyOnWrite;
 import sonia.scm.security.KeyGenerator;
 import sonia.scm.store.DataStore;
+import sonia.scm.store.IdHandlerForStores;
 import sonia.scm.store.StoreException;
 import sonia.scm.xml.XmlStreams;
 
@@ -44,20 +45,30 @@ class JAXBDataStore<T> extends FileBasedStore<T> implements DataStore<T> {
  
   private static final Logger LOG = LoggerFactory.getLogger(JAXBDataStore.class);
 
-  private final KeyGenerator keyGenerator;
   private final TypedStoreContext<T> context;
   private final DataFileCache.DataFileCacheInstance cache;
 
+  private final IdHandlerForStores<T> idHandlerForStores;
+
   JAXBDataStore(KeyGenerator keyGenerator, TypedStoreContext<T> context, File directory, boolean readOnly, DataFileCache.DataFileCacheInstance cache) {
     super(directory, StoreConstants.FILE_EXTENSION, readOnly);
-    this.keyGenerator = keyGenerator;
     this.cache = cache;
     this.directory = directory;
     this.context = context;
+    this.idHandlerForStores = new IdHandlerForStores<>(context.getType(), keyGenerator, this::doPut);
+  }
+
+  @Override
+  public String put(T item) {
+    return idHandlerForStores.put(item);
   }
 
   @Override
   public void put(String id, T item) {
+    idHandlerForStores.put(id, item);
+  }
+
+  private void doPut(String id, T item) {
     LOG.debug("put item {} to store", id);
 
     assertNotReadOnly();
@@ -79,15 +90,6 @@ class JAXBDataStore<T> extends FileBasedStore<T> implements DataStore<T> {
     }
   }
 
-  @Override
-  public String put(T item) {
-    String key = keyGenerator.createKey();
-
-    put(key, item);
-
-    return key;
-  }
-
   @Override
   public Map<String, T> getAll() {
     LOG.trace("get all items from data store");

scm-persistence/src/main/java/sonia/scm/store/file/TypedStoreContext.java

@@ -86,7 +86,7 @@ final class TypedStoreContext<T> {
     withClassLoader(consumer, unmarshaller);
   }
 
-  Class<?> getType() {
+  Class<T> getType() {
     return parameters.getType();
   }
 

scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStore.java

@@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper;
 import lombok.extern.slf4j.Slf4j;
 import sonia.scm.plugin.QueryableTypeDescriptor;
 import sonia.scm.security.KeyGenerator;
+import sonia.scm.store.IdHandlerForStores;
 import sonia.scm.store.QueryableMutableStore;
 import sonia.scm.store.StoreException;
 
@@ -38,11 +39,12 @@ import java.util.concurrent.locks.ReadWriteLock;
 class SQLiteQueryableMutableStore<T> extends SQLiteQueryableStore<T> implements QueryableMutableStore<T> {
 
   private final ObjectMapper objectMapper;
-  private final KeyGenerator keyGenerator;
 
   private final Class<T> clazz;
   private final String[] parentIds;
 
+  private final IdHandlerForStores<T> idHandlerForStores;
+
   public SQLiteQueryableMutableStore(ObjectMapper objectMapper,
                                      KeyGenerator keyGenerator,
                                      Connection connection,
@@ -52,20 +54,22 @@ class SQLiteQueryableMutableStore<T> extends SQLiteQueryableStore<T> implements
                                      ReadWriteLock lock) {
     super(objectMapper, connection, clazz, queryableTypeDescriptor, parentIds, lock);
     this.objectMapper = objectMapper;
-    this.keyGenerator = keyGenerator;
     this.clazz = clazz;
     this.parentIds = parentIds;
+    this.idHandlerForStores = new IdHandlerForStores<>(clazz, keyGenerator, this::doPut);
   }
 
   @Override
   public String put(T item) {
-    String id = keyGenerator.createKey();
-    put(id, item);
-    return id;
+    return idHandlerForStores.put(item);
   }
 
   @Override
   public void put(String id, T item) {
+    idHandlerForStores.put(id, item);
+  }
+
+  private void doPut(String id, T item) {
     List<String> columnsToInsert = new ArrayList<>(Arrays.asList(parentIds));
     columnsToInsert.add(id);
     columnsToInsert.add(marshal(item));

scm-persistence/src/test/java/sonia/scm/store/sqlite/SQLiteQueryableMutableStoreTest.java

@@ -32,6 +32,7 @@ import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+@SuppressWarnings("resource")
 class SQLiteQueryableMutableStoreTest {
 
   private Connection connection;
@@ -43,6 +44,11 @@ class SQLiteQueryableMutableStoreTest {
     connection = DriverManager.getConnection(connectionString);
   }
 
+  @AfterEach
+  void closeDB() throws SQLException {
+    connection.close();
+  }
+
   @Nested
   class Put {
 
@@ -163,7 +169,6 @@ class SQLiteQueryableMutableStoreTest {
     @Test
     void shouldReturnForNotExistingValue() {
       SQLiteQueryableMutableStore<User> store = new StoreTestBuilder(connectionString).withIds();
-
       User earth = store.get("earth");
 
       assertThat(earth)
@@ -233,6 +238,48 @@ class SQLiteQueryableMutableStoreTest {
     }
   }
 
+  @Nested
+  class WithAnnotatedId {
+
+    @Test
+    void shouldUseIdFromItemOnPut() {
+      SQLiteQueryableMutableStore<SpaceshipWithId> store = new StoreTestBuilder(connectionString).forClassWithIds(SpaceshipWithId.class);
+
+      String id = store.put(new SpaceshipWithId("Heart of Gold", 42));
+      SpaceshipWithId spaceship = store.get("Heart of Gold");
+
+      assertThat(id).isEqualTo("Heart of Gold");
+      assertThat(spaceship).isNotNull();
+    }
+
+    @Test
+    void shouldSetNewIdInItemOnPut() {
+      SQLiteQueryableMutableStore<SpaceshipWithId> store = new StoreTestBuilder(connectionString).forClassWithIds(SpaceshipWithId.class);
+
+      String id = store.put(new SpaceshipWithId());
+      SpaceshipWithId spaceship = store.get(id);
+
+      assertThat(spaceship.getName()).isNotNull();
+      assertThat(spaceship.getName()).isEqualTo(id);
+    }
+
+    @Test
+    void shouldStoreWithNewIdAfterManualChange() {
+      SQLiteQueryableMutableStore<SpaceshipWithId> store = new StoreTestBuilder(connectionString).forClassWithIds(SpaceshipWithId.class);
+
+      store.put(new SpaceshipWithId("Heart of Gold", 42));
+      SpaceshipWithId spaceship = store.get("Heart of Gold");
+
+      store.put("Space Zeppelin", spaceship);
+
+      SpaceshipWithId zeppelin = store.get("Space Zeppelin");
+      assertThat(zeppelin.getName()).isEqualTo("Space Zeppelin");
+
+      spaceship = store.get("Heart of Gold");
+      assertThat(spaceship.getName()).isEqualTo("Heart of Gold");
+    }
+  }
+
   @Nested
   class Clear {
     @Test
@@ -262,9 +309,4 @@ class SQLiteQueryableMutableStoreTest {
       assertThat(store.getAll()).containsOnlyKeys("tricia");
     }
   }
-
-  @AfterEach
-  void closeDB() throws SQLException {
-    connection.close();
-  }
 }

scm-persistence/src/test/java/sonia/scm/store/sqlite/SpaceshipWithId.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2020 - present Cloudogu GmbH
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see https://www.gnu.org/licenses/.
+ */
+
+package sonia.scm.store.sqlite;
+
+import jakarta.xml.bind.annotation.XmlAccessType;
+import jakarta.xml.bind.annotation.XmlAccessorType;
+import lombok.EqualsAndHashCode;
+import sonia.scm.store.Id;
+import sonia.scm.store.QueryableType;
+
+@QueryableType
+@XmlAccessorType(XmlAccessType.FIELD)
+@EqualsAndHashCode
+class SpaceshipWithId {
+  @Id
+  private String name;
+  int flightCount;
+
+  public SpaceshipWithId() {
+  }
+
+  public SpaceshipWithId(String name, int flightCount) {
+    this.name = name;
+    this.flightCount = flightCount;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public int getFlightCount() {
+    return flightCount;
+  }
+
+  public void setFlightCount(int flightCount) {
+    this.flightCount = flightCount;
+  }
+}