Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use integers for date ordinals #3346

Merged
merged 5 commits into from
Feb 19, 2022
Merged

Use integers for date ordinals #3346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e3fc03
Use integers for date ordinals
jamesagnew Jan 29, 2022
0dd1d0c
Add changelog
jamesagnew Jan 29, 2022
e813502
Merge branch 'master' into ja_2022_date_ordinal_postgres_fail
jamesagnew Feb 4, 2022
b47c893
Adjust changelog
jamesagnew Feb 4, 2022
60fc1d1
Merge branch 'master' into ja_2022_date_ordinal_postgres_fail
jamesagnew Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/util/DateUtils.java
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions ...esources/ca/uhn/hapi/fhir/changelog/6_0_0/3346-always-use-integers-for-date-ordinals.yaml
View file
Open in desktop
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."
2 changes: 1 addition & 1 deletion hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java
View file
Open in desktop
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
51 changes: 24 additions & 27 deletions ...ver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/DatePredicateBuilder.java
View file
Open in desktop
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
70 changes: 70 additions & 0 deletions ...c/test/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilderDialectPostgresTest.java
View file
Open in desktop
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();
}
}