Skip to content

Commit

Permalink
fix: use generated user name column for make query DB only [DHIS2-17200]
Browse files Browse the repository at this point in the history
  • Loading branch information
jbee committed Nov 26, 2024
1 parent 7533a4e commit af3a5f2
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 22 deletions.
8 changes: 5 additions & 3 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -702,10 +703,11 @@ public void updateOrganisationUnits(Set<OrganisationUnit> updates) {
}
}

/** Returns the concatenated first name and surname. */
@Override
@JsonProperty(access = JsonProperty.Access.READ_ONLY)
@Property(required = FALSE, access = Property.Access.READ_ONLY)
public String getName() {
return firstName + " " + surname;
return name;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,12 @@ public static Restriction isEmpty(String path) {
}

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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" not-null="true" length="321" insert="false" update="false" />

<property name="email" length="160" />

Expand Down
Original file line number Diff line number Diff line change
@@ -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 || ' ' || userinfo.surname ) stored;
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import org.hisp.dhis.query.Criterion;
import org.hisp.dhis.query.GetObjectListParams;
import org.hisp.dhis.query.GetObjectParams;
import org.hisp.dhis.query.Junction;
import org.hisp.dhis.query.Query;
import org.hisp.dhis.query.QueryParserException;
import org.hisp.dhis.query.QueryService;
Expand Down Expand Up @@ -200,7 +201,11 @@ protected final ResponseEntity<StreamingJsonRoot<T>> getObjectListInternal(

addProgrammaticModifiers(params);

boolean isAlwaysEmpty = additionalFilters.stream().anyMatch(Criterion::isAlwaysFalse);
// a top level restriction combined with AND that is always false always results in an empty
// list
boolean isAlwaysEmpty =
params.getRootJunction() == Junction.Type.AND
&& additionalFilters.stream().anyMatch(Criterion::isAlwaysFalse);
List<T> entities = isAlwaysEmpty ? List.of() : getEntityList(params, additionalFilters);
postProcessResponseEntities(entities, params);

Expand Down

0 comments on commit af3a5f2

Please sign in to comment.