fix: use generated user name column for make query DB only [DHIS2-172…

…00] (#19294)

* fix: use generated user name column for make query DB only [DHIS2-17200]

* fix: User name hibernate mapping

* fix: fallback to compute name

* fix: H2 controller tests

* fix: postgres integration tests

* chore: cleanup SQL

dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java

@@ -28,6 +28,7 @@
 package org.hisp.dhis.user;
 
 import static org.apache.commons.collections4.CollectionUtils.emptyIfNull;
+import static org.hisp.dhis.schema.annotation.Property.Value.FALSE;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -525,7 +526,7 @@ public void setPreviousPasswords(List<String> previousPasswords) {
 
   @JsonProperty
   @JacksonXmlProperty(namespace = DxfNamespaces.DXF_2_0)
-  @Property(value = PropertyType.TEXT, required = Property.Value.FALSE)
+  @Property(value = PropertyType.TEXT, required = FALSE)
   public String getUsername() {
     return username;
   }
@@ -702,10 +703,17 @@ public void updateOrganisationUnits(Set<OrganisationUnit> updates) {
     }
   }
 
-  /** Returns the concatenated first name and surname. */
+  /**
+   * Note that setting read-only both ways seems needed when this is a DB field that is not null but
+   * generated.
+   */
   @Override
+  @JsonProperty(access = JsonProperty.Access.READ_ONLY)
+  @Property(required = FALSE, access = Property.Access.READ_ONLY)
   public String getName() {
-    return firstName + " " + surname;
+    // this is to maintain name for transient User objects initialized without setting name
+    if (name == null) return firstName + " " + surname;
+    return name;
   }
 
   /**

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/gist/GistPlanner.java

@@ -62,7 +62,6 @@
 import org.hisp.dhis.schema.RelativePropertyContext;
 import org.hisp.dhis.schema.Schema;
 import org.hisp.dhis.schema.annotation.Gist.Transform;
-import org.hisp.dhis.user.User;
 
 /**
  * The {@link GistPlanner} is responsible to expand the list of {@link Field}s following the {@link
@@ -90,7 +89,6 @@ private List<Field> planFields() {
     fields = withPresetFields(fields); // 1:n
     fields = withAttributeFields(fields); // 1:1
     fields = withDisplayAsTranslatedFields(fields); // 1:1
-    fields = withUserNameAsFromTransformedField(fields); // 1:1
     fields = withInnerAsSeparateFields(fields); // 1:n
     fields = withCollectionItemPropertyAsTransformation(fields); // 1:1
     fields = withEffectiveTransformation(fields); // 1:1
@@ -274,19 +272,6 @@ private List<Field> withDisplayAsTranslatedFields(List<Field> fields) {
                 .withPropertyPath(pathOnSameParent(f.getPropertyPath(), "shortName")));
   }
 
-  private List<Field> withUserNameAsFromTransformedField(List<Field> fields) {
-    return query.getElementType() != User.class
-        ? fields
-        : map1to1(
-            fields,
-            f -> f.getPropertyPath().equals("name"),
-            f ->
-                f.toBuilder()
-                    .transformation(Transform.FROM)
-                    .transformationArgument("firstName,surname")
-                    .build());
-  }
-
   /** Transforms {@code field[a,b]} syntax to {@code field.a,field.b} */
   private List<Field> withInnerAsSeparateFields(List<Field> fields) {
     List<Field> expanded = new ArrayList<>();

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/query/Restrictions.java

@@ -133,8 +133,20 @@ public static Restriction isEmpty(String path) {
     return new Restriction(path, new EmptyOperator<>());
   }
 
+  /**
+   * Builds a PR group to match the query string against id, code and name.
+   *
+   * @param schema of the root entity
+   * @param query the query string value used the URL {@code query} parameter
+   * @return OR group with the filters for the query string
+   */
   public static Disjunction query(Schema schema, String query) {
-    return or(schema, eq("id", query), eq("code", query), ilike("name", query, MatchMode.ANYWHERE));
+    Restriction name = ilike("name", query, MatchMode.ANYWHERE);
+    Restriction code = eq("code", query);
+    if (query.length() != 11) return or(schema, code, name);
+    // only a query with length 11 has a chance of matching a UID
+    Restriction id = eq("id", query);
+    return or(schema, id, code, name);
   }
 
   public static Disjunction or(Schema schema, Criterion... filters) {

dhis-2/dhis-services/dhis-service-core/src/main/resources/org/hisp/dhis/user/hibernate/User.hbm.xml

@@ -25,9 +25,11 @@
 
     <property name="created" type="timestamp" not-null="true" />
 
-    <property name="surname" not-null="false" length="160" />
+    <property name="surname" not-null="true" length="160" />
 
-    <property name="firstName" not-null="false" length="160" />
+    <property name="firstName" not-null="true" length="160" />
+
+    <property name="name" length="321" insert="false" update="false" />
 
     <property name="email" length="160" />
 

dhis-2/dhis-support/dhis-support-db-migration/src/main/resources/org/hisp/dhis/db/migration/2.42/V2_42_32__User_name_as_generated_column.sql

@@ -0,0 +1,11 @@
+/*
+Adds the "name" column to users as a computed column based on first and last name
+to move named based filters into the DB.
+This is important as the web API "query" search is a combination of multiple columns
+one of which is name. Such combinations must stay all in DB to be correct.
+
+Since both source columns are not null the name can and should also be not null
+and the computation does not need to handle null.
+*/
+alter table userinfo
+    add column if not exists name character varying(321) not null generated always as ( firstname || ' ' || surname ) stored;

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/fieldfiltering/FieldPathHelperTest.java

@@ -125,7 +125,7 @@ void skipSharingFieldsExcludeCorrectFieldsTest() {
 
     // then only matching exclusions should have been applied
     // and fields starting with 'user' should still be present
-    assertEquals(57, result.size()); // all user properties
+    assertEquals(58, result.size()); // all user properties
     assertTrue(
         result.stream()
             .map(FieldPath::getName)

dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/user/UserServiceTest.java

@@ -68,7 +68,6 @@
 import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.transaction.annotation.Transactional;
-import org.springframework.transaction.support.TransactionTemplate;
 
 /**
  * @author Lars Helge Overland
@@ -79,8 +78,6 @@ class UserServiceTest extends PostgresIntegrationTestBase {
 
   @Autowired private UserGroupService userGroupService;
 
-  @Autowired private UserSettingsService userSettingsService;
-
   @Autowired private OrganisationUnitService organisationUnitService;
 
   @Autowired private SystemSettingsService settingsService;
@@ -89,8 +86,6 @@ class UserServiceTest extends PostgresIntegrationTestBase {
 
   @Autowired private PasswordManager passwordManager;
 
-  @Autowired private TransactionTemplate transactionTemplate;
-
   private OrganisationUnit unitA;
 
   private OrganisationUnit unitB;
@@ -680,6 +675,8 @@ void testGetDisplayNameNull() {
   @Test
   void testBCryptedPasswordOnInputError() {
     User user = new User();
+    user.setFirstName("test");
+    user.setSurname("tester");
     user.setUsername("test");
     user.setPassword("password");
     userService.addUser(user);

dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/AbstractGistControllerPostgresTest.java

@@ -0,0 +1,84 @@
+/*
+ * Copyright (c) 2004-2024, University of Oslo
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ * Neither the name of the HISP project nor the names of its contributors may
+ * be used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.hisp.dhis.webapi.controller;
+
+import static org.hisp.dhis.http.HttpAssertions.assertStatus;
+import static org.hisp.dhis.http.HttpClientAdapter.Body;
+
+import org.hisp.dhis.http.HttpStatus;
+import org.hisp.dhis.test.webapi.PostgresControllerIntegrationTestBase;
+import org.hisp.dhis.user.User;
+import org.junit.jupiter.api.BeforeEach;
+
+/**
+ * Base class for controller tests of the Gist API that require an actual postgres DB.
+ *
+ * <p>This usually is because of the use of JSONB functions. Another reason is use of generated
+ * columns.
+ *
+ * @author Jan Bernitt
+ */
+abstract class AbstractGistControllerPostgresTest extends PostgresControllerIntegrationTestBase {
+
+  protected String userGroupId;
+  protected String orgUnitId;
+  protected String dataSetId;
+  protected User userA;
+
+  @BeforeEach
+  void setUp() {
+    userA = createUserWithAuth("userGist", "ALL");
+
+    switchContextToUser(userA);
+
+    userGroupId =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST("/userGroups/", "{'name':'groupX', 'users':[{'id':'" + getAdminUid() + "'}]}"));
+    assertStatus(
+        HttpStatus.OK,
+        PATCH(
+            "/users/{id}?importReportMode=ERRORS",
+            getAdminUid(),
+            Body("[{'op': 'add', 'path': '/birthday', 'value': '1980-12-12'}]")));
+    orgUnitId =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/organisationUnits/",
+                "{'name':'unitA', 'shortName':'unitA', 'openingDate':'2021-01-01'}"));
+    dataSetId =
+        assertStatus(
+            HttpStatus.CREATED,
+            POST(
+                "/dataSets/",
+                "{'name':'set1', 'shortName':'set1', 'organisationUnits': [{'id':'"
+                    + orgUnitId
+                    + "'}], 'periodType':'Daily'}"));
+  }
+}

dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/GistControllerPostgresTest.java

@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2004-2024, University of Oslo
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ * Neither the name of the HISP project nor the names of its contributors may
+ * be used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.hisp.dhis.webapi.controller;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import org.hisp.dhis.jsontree.JsonArray;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for the Gist API that cannot run on H2 but require an actual postgres DB.
+ *
+ * @author Jan Bernitt
+ */
+class GistControllerPostgresTest extends AbstractGistControllerPostgresTest {
+
+  /**
+   * Note: this test was moved here unchanged to verify the functionality in the API hasn't changed.
+   * However, when the "name" became a generated column instead of using a from transformation it
+   * has to run with an actual postgres DB. The test name is kept to allow seeing the evolution
+   * looking back to versions that do use from instead of a generated column to synthesize the name
+   * from firstName and surname post DB query.
+   */
+  @Test
+  void testField_UserNameAutomaticFromTransformation() {
+    JsonArray users = GET("/users/gist?fields=id,name&headless=true").content();
+    assertEquals(
+        "FirstNameuserGist SurnameuserGist", users.getObject(1).getString("name").string());
+  }
+
+  /** Note: now that user "name" is a generated column we can also filter on it */
+  @Test
+  void testFilter_UserName() {
+    JsonArray users =
+        GET("/users/gist?fields=id,name&headless=true&filter=name:like:Gist").content();
+    assertFalse(users.isEmpty());
+  }
+}

dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/GistFieldsControllerTest.java

@@ -194,13 +194,6 @@ void testField_Access() {
     assertTrue(access.getBoolean("delete").booleanValue());
   }
 
-  @Test
-  void testField_UserNameAutomaticFromTransformation() {
-    JsonArray users = GET("/users/gist?fields=id,name&headless=true").content();
-    assertEquals(
-        "FirstNameuserGist SurnameuserGist", users.getObject(1).getString("name").string());
-  }
-
   @Test
   void testNestedFieldsOfListProperty() {
     JsonArray groups =

dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/SchemaControllerTest.java

@@ -33,10 +33,12 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.util.Optional;
 import org.hisp.dhis.http.HttpStatus;
 import org.hisp.dhis.jsontree.JsonObject;
 import org.hisp.dhis.schema.PropertyType;
 import org.hisp.dhis.test.webapi.H2ControllerIntegrationTestBase;
+import org.hisp.dhis.test.webapi.json.domain.JsonProperty;
 import org.hisp.dhis.test.webapi.json.domain.JsonSchema;
 import org.junit.jupiter.api.Test;
 import org.springframework.transaction.annotation.Transactional;
@@ -130,4 +132,17 @@ void testAttributeWritable() {
               }
             });
   }
+
+  @Test
+  void testUserNameIsPersistedButReadOnly() {
+    JsonSchema user = GET("/schemas/user").content().as(JsonSchema.class);
+    Optional<JsonProperty> maybeName =
+        user.getProperties().stream().filter(p -> "name".equals(p.getName())).findFirst();
+    assertTrue(maybeName.isPresent());
+    JsonProperty name = maybeName.get();
+    assertTrue(name.isPersisted());
+    assertTrue(name.isReadable());
+    assertFalse(name.isWritable());
+    assertFalse(name.isRequired());
+  }
 }