Skip to content

Commit

Permalink
Use integers for date ordinals (#3346)
Browse files Browse the repository at this point in the history
* Use integers for date ordinals

* Add changelog

* Adjust changelog
  • Loading branch information
jamesagnew authored Feb 19, 2022
1 parent c6122bc commit 058e536
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 34 deletions.
12 changes: 6 additions & 6 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/util/DateUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -242,27 +242,27 @@ public static <T> T notNull(final T argument, final String name) {
public static Pair<String, String> getCompletedDate(String theIncompleteDateStr) {

if (StringUtils.isBlank(theIncompleteDateStr))
return new ImmutablePair<String, String>(null, null);
return new ImmutablePair<>(null, null);

String lbStr, upStr;
// YYYY only, return the last day of the year
if (theIncompleteDateStr.length() == 4) {
lbStr = theIncompleteDateStr + "-01-01"; // first day of the year
upStr = theIncompleteDateStr + "-12-31"; // last day of the year
return new ImmutablePair<String, String>(lbStr, upStr);
return new ImmutablePair<>(lbStr, upStr);
}

// Not YYYY-MM, no change
if (theIncompleteDateStr.length() != 7)
return new ImmutablePair<String, String>(theIncompleteDateStr, theIncompleteDateStr);
return new ImmutablePair<>(theIncompleteDateStr, theIncompleteDateStr);

// YYYY-MM Only
Date lb=null;
Date lb;
try {
// first day of the month
lb = new SimpleDateFormat("yyyy-MM-dd").parse(theIncompleteDateStr+"-01");
} catch (ParseException e) {
return new ImmutablePair<String, String>(theIncompleteDateStr, theIncompleteDateStr);
return new ImmutablePair<>(theIncompleteDateStr, theIncompleteDateStr);
}

// last day of the month
Expand All @@ -278,7 +278,7 @@ public static Pair<String, String> getCompletedDate(String theIncompleteDateStr)
lbStr = new SimpleDateFormat("yyyy-MM-dd").format(lb);
upStr = new SimpleDateFormat("yyyy-MM-dd").format(ub);

return new ImmutablePair<String, String>(lbStr, upStr);
return new ImmutablePair<>(lbStr, upStr);
}

public static Date getEndOfDay(Date theDate) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
type: fix
issue: 3346
title: "When searching for date search parameters on Postgres, ordinals could sometimes be
represented as strings, causing a search failure. This has been corrected."
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ public Condition createPredicateDate(@Nullable DbColumn theSourceJoinColumn, Str
List<Condition> codePredicates = new ArrayList<>();

for (IQueryParameterType nextOr : theList) {
Condition p = predicateBuilder.createPredicateDateWithoutIdentityPredicate(nextOr, predicateBuilder, theOperation);
Condition p = predicateBuilder.createPredicateDateWithoutIdentityPredicate(nextOr, theOperation);
codePredicates.add(p);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import ca.uhn.fhir.rest.param.ParamPrefixEnum;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.util.DateUtils;

import com.google.common.annotations.VisibleForTesting;
import com.healthmarketscience.sqlbuilder.ComboCondition;
import com.healthmarketscience.sqlbuilder.Condition;
import com.healthmarketscience.sqlbuilder.dbspec.basic.DbColumn;
Expand All @@ -48,7 +48,6 @@ public class DatePredicateBuilder extends BaseSearchParamPredicateBuilder {
private final DbColumn myColumnValueLow;
private final DbColumn myColumnValueLowDateOrdinal;
private final DbColumn myColumnValueHighDateOrdinal;

@Autowired
private DaoConfig myDaoConfig;

Expand All @@ -64,9 +63,12 @@ public DatePredicateBuilder(SearchQueryBuilder theSearchSqlBuilder) {
myColumnValueHighDateOrdinal = getTable().addColumn("SP_VALUE_HIGH_DATE_ORDINAL");
}

@VisibleForTesting
public void setDaoConfigForUnitTest(DaoConfig theDaoConfig) {
myDaoConfig = theDaoConfig;
}

public Condition createPredicateDateWithoutIdentityPredicate(IQueryParameterType theParam,
DatePredicateBuilder theFrom,
SearchFilterParser.CompareOperation theOperation) {

Condition p;
Expand All @@ -77,26 +79,22 @@ public Condition createPredicateDateWithoutIdentityPredicate(IQueryParameterType
date = new DateParam(ParamPrefixEnum.EQUAL, date.getValueAsString());
}
DateRangeParam range = new DateRangeParam(date);
p = createPredicateDateFromRange(theFrom, range, theOperation);
p = createPredicateDateFromRange(range, theOperation);
} else {
// TODO: handle missing date param?
p = null;
}
} else if (theParam instanceof DateRangeParam) {
DateRangeParam range = (DateRangeParam) theParam;
p = createPredicateDateFromRange(
theFrom,
range,
theOperation);
p = createPredicateDateFromRange(range, theOperation);
} else {
throw new IllegalArgumentException(Msg.code(1251) + "Invalid token type: " + theParam.getClass());
}

return p;
}

private Condition createPredicateDateFromRange(DatePredicateBuilder theFrom,
DateRangeParam theRange,
private Condition createPredicateDateFromRange(DateRangeParam theRange,
SearchFilterParser.CompareOperation theOperation) {


Expand Down Expand Up @@ -129,46 +127,45 @@ private Condition createPredicateDateFromRange(DatePredicateBuilder theFrom,
genericLowerBound = lowerBoundAsOrdinal;
genericUpperBound = upperBoundAsOrdinal;
if (upperBound != null && upperBound.getPrecision().ordinal() <= TemporalPrecisionEnum.MONTH.ordinal()) {
genericUpperBound = DateUtils.getCompletedDate(upperBound.getValueAsString()).getRight().replace("-", "");

genericUpperBound = Integer.parseInt(DateUtils.getCompletedDate(upperBound.getValueAsString()).getRight().replace("-", ""));
}
} else {
lowValueField = DatePredicateBuilder.ColumnEnum.LOW;
highValueField = DatePredicateBuilder.ColumnEnum.HIGH;
genericLowerBound = lowerBoundInstant;
genericUpperBound = upperBoundInstant;
if (upperBound != null && upperBound.getPrecision().ordinal() <= TemporalPrecisionEnum.MONTH.ordinal()) {
String theCompleteDateStr = DateUtils.getCompletedDate(upperBound.getValueAsString()).getRight().replace("-", "");
String theCompleteDateStr = DateUtils.getCompletedDate(upperBound.getValueAsString()).getRight().replace("-", "");
genericUpperBound = DateUtils.parseDate(theCompleteDateStr);
}
}

if (theOperation == SearchFilterParser.CompareOperation.lt || theOperation == SearchFilterParser.CompareOperation.le) {
// use lower bound first
if (lowerBoundInstant != null) {
lb = theFrom.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericLowerBound);
lb = this.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericLowerBound);
if (myDaoConfig.isAccountForDateIndexNulls()) {
lb = ComboCondition.or(lb, theFrom.createPredicate(highValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericLowerBound));
lb = ComboCondition.or(lb, this.createPredicate(highValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericLowerBound));
}
} else if (upperBoundInstant != null) {
ub = theFrom.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound);
ub = this.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound);
if (myDaoConfig.isAccountForDateIndexNulls()) {
ub = ComboCondition.or(ub, theFrom.createPredicate(highValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound));
ub = ComboCondition.or(ub, this.createPredicate(highValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound));
}
} else {
throw new InvalidRequestException(Msg.code(1252) + "lowerBound and upperBound value not correctly specified for comparing " + theOperation);
}
} else if (theOperation == SearchFilterParser.CompareOperation.gt || theOperation == SearchFilterParser.CompareOperation.ge) {
// use upper bound first, e.g value between 6 and 10
if (upperBoundInstant != null) {
ub = theFrom.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericUpperBound);
ub = this.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericUpperBound);
if (myDaoConfig.isAccountForDateIndexNulls()) {
ub = ComboCondition.or(ub, theFrom.createPredicate(lowValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericUpperBound));
ub = ComboCondition.or(ub, this.createPredicate(lowValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericUpperBound));
}
} else if (lowerBoundInstant != null) {
lb = theFrom.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound);
lb = this.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound);
if (myDaoConfig.isAccountForDateIndexNulls()) {
lb = ComboCondition.or(lb, theFrom.createPredicate(lowValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound));
lb = ComboCondition.or(lb, this.createPredicate(lowValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound));
}
} else {
throw new InvalidRequestException(Msg.code(1253) + "upperBound and lowerBound value not correctly specified for compare theOperation");
Expand All @@ -178,16 +175,16 @@ private Condition createPredicateDateFromRange(DatePredicateBuilder theFrom,
(upperBoundInstant == null)) {
throw new InvalidRequestException(Msg.code(1254) + "lowerBound and/or upperBound value not correctly specified for compare theOperation");
}
lt = theFrom.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN, genericLowerBound);
gt = theFrom.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN, genericUpperBound);
lt = this.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN, genericLowerBound);
gt = this.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN, genericUpperBound);
lb = ComboCondition.or(lt, gt);
} else if ((theOperation == SearchFilterParser.CompareOperation.eq)
|| (theOperation == SearchFilterParser.CompareOperation.sa)
|| (theOperation == SearchFilterParser.CompareOperation.eb)
|| (theOperation == null)) {
if (lowerBoundInstant != null) {
gt = theFrom.createPredicate(lowValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound);
lt = theFrom.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound);
gt = this.createPredicate(lowValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound);
lt = this.createPredicate(highValueField, ParamPrefixEnum.GREATERTHAN_OR_EQUALS, genericLowerBound);

if (lowerBound.getPrefix() == ParamPrefixEnum.STARTS_AFTER || lowerBound.getPrefix() == ParamPrefixEnum.EQUAL) {
lb = gt;
Expand All @@ -197,8 +194,8 @@ private Condition createPredicateDateFromRange(DatePredicateBuilder theFrom,
}

if (upperBoundInstant != null) {
gt = theFrom.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound);
lt = theFrom.createPredicate(highValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound);
gt = this.createPredicate(lowValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound);
lt = this.createPredicate(highValueField, ParamPrefixEnum.LESSTHAN_OR_EQUALS, genericUpperBound);


if (theRange.getUpperBound().getPrefix() == ParamPrefixEnum.ENDS_BEFORE || theRange.getUpperBound().getPrefix() == ParamPrefixEnum.EQUAL) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package ca.uhn.fhir.jpa.search.builder.sql;

import ca.uhn.fhir.jpa.api.config.DaoConfig;
import ca.uhn.fhir.jpa.dao.predicate.SearchFilterParser;
import ca.uhn.fhir.jpa.search.builder.predicate.BaseJoiningPredicateBuilder;
import ca.uhn.fhir.jpa.search.builder.predicate.DatePredicateBuilder;
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceTablePredicateBuilder;
import ca.uhn.fhir.rest.param.DateParam;
import ca.uhn.fhir.rest.param.DateRangeParam;
import com.healthmarketscience.sqlbuilder.Condition;
import org.apache.commons.lang3.StringUtils;
import org.hibernate.dialect.Dialect;
import org.hibernate.dialect.PostgreSQL10Dialect;
import org.hibernate.dialect.PostgreSQL95Dialect;
import org.hibernate.dialect.SQLServer2012Dialect;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;

import javax.annotation.Nonnull;
import java.util.Locale;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
public class SearchQueryBuilderDialectPostgresTest extends BaseSearchQueryBuilderDialectTest {

/**
* Make sure we're using integers and not strings as bind variables
* for ordinals
*/
@Test
public void testOrdinalSearchesUseIntegerParameters() {
DaoConfig daoConfig = new DaoConfig();
daoConfig.getModelConfig().setUseOrdinalDatesForDayPrecisionSearches(true);

SearchQueryBuilder searchQueryBuilder = createSearchQueryBuilder();
when(mySqlObjectFactory.dateIndexTable(any())).thenReturn(new DatePredicateBuilder(searchQueryBuilder));

DatePredicateBuilder datePredicateBuilder = searchQueryBuilder.addDatePredicateBuilder(null);
datePredicateBuilder.setDaoConfigForUnitTest(daoConfig);

Condition datePredicate = datePredicateBuilder.createPredicateDateWithoutIdentityPredicate(new DateParam("2022"), SearchFilterParser.CompareOperation.eq);
Condition comboPredicate = datePredicateBuilder.combineWithHashIdentityPredicate("Observation", "date", datePredicate);

searchQueryBuilder.addPredicate(comboPredicate);

GeneratedSql generatedSql = searchQueryBuilder.generate(0, 500);
logSql(generatedSql);

String sql = generatedSql.getSql();
assertEquals("SELECT t0.RES_ID FROM HFJ_SPIDX_DATE t0 WHERE ((t0.HASH_IDENTITY = ?) AND ((t0.SP_VALUE_LOW_DATE_ORDINAL >= ?) AND (t0.SP_VALUE_HIGH_DATE_ORDINAL <= ?))) limit ?", sql);

assertEquals(4, StringUtils.countMatches(sql, "?"));
assertEquals(4, generatedSql.getBindVariables().size());
assertEquals(123682819940570799L, generatedSql.getBindVariables().get(0));
assertEquals(20220101, generatedSql.getBindVariables().get(1));
assertEquals(20221231, generatedSql.getBindVariables().get(2));
assertEquals(500, generatedSql.getBindVariables().get(3));
}

@Nonnull
@Override
protected Dialect createDialect() {
return new PostgreSQL10Dialect();
}
}

0 comments on commit 058e536

Please sign in to comment.