Skip to content

Commit

Permalink
Fix #696: Corresctly handle negative numbers
Browse files Browse the repository at this point in the history
  • Loading branch information
@jamesagnew
jamesagnew committed Jul 26, 2017
1 parent 6aa58cf commit 274e218
Show file tree
Hide file tree
Showing 5 changed files with 186 additions and 8 deletions.
13 changes: 9 additions & 4 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/BaseParamWithPrefix.java
View file
Expand Up @@ -10,7 +10,7 @@
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
Expand Down Expand Up @@ -44,8 +44,13 @@ public abstract class BaseParamWithPrefix<T extends BaseParam> extends BaseParam
String extractPrefixAndReturnRest(String theString) {
int offset = 0;
while (true) {
if (theString.length() == offset || Character.isDigit(theString.charAt(offset))) {
if (theString.length() == offset) {
break;
} else {
char nextChar = theString.charAt(offset);
if (nextChar == '-' || Character.isDigit(nextChar)) {
break;
}
}
offset++;
}
Expand All @@ -60,15 +65,15 @@ String extractPrefixAndReturnRest(String theString) {
}

/**
* @deprecated Use {@link #getPrefix() instead}
* @deprecated Use {@link #getPrefix()} instead
*/
@Deprecated
public QuantityCompararatorEnum getComparator() {
ParamPrefixEnum prefix = getPrefix();
if (prefix == null) {
return null;
}

return QuantityCompararatorEnum.forCode(prefix.getDstu1Value());
}

Expand Down
26 changes: 26 additions & 0 deletions ...paserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java
View file
Expand Up @@ -11,6 +11,7 @@
import static org.junit.Assert.fail;

import java.io.*;
import java.math.BigDecimal;
import java.net.*;
import java.nio.charset.StandardCharsets;
import java.util.*;
Expand Down Expand Up @@ -3197,6 +3198,31 @@ public void testSearchWithInvalidQuantityPrefix() throws Exception {
}
}

@Test()
public void testSearchNegativeNumbers() throws Exception {
Observation o = new Observation();
o.setValue(new Quantity().setValue(new BigDecimal("-10")));
String oid1 = myObservationDao.create(o, mySrd).getId().toUnqualifiedVersionless().getValue();

Observation o2 = new Observation();
o2.setValue(new Quantity().setValue(new BigDecimal("-20")));
String oid2 = myObservationDao.create(o2, mySrd).getId().toUnqualifiedVersionless().getValue();

HttpGet get = new HttpGet(ourServerBase + "/Observation?value-quantity=gt-15");
CloseableHttpResponse resp = ourHttpClient.execute(get);
try {
assertEquals(200, resp.getStatusLine().getStatusCode());
Bundle bundle = myFhirCtx.newXmlParser().parseResource(Bundle.class, IOUtils.toString(resp.getEntity().getContent(), Constants.CHARSET_UTF8));

List<String> ids = toUnqualifiedVersionlessIdValues(bundle);
assertThat(ids, contains(oid1));
assertThat(ids, not(contains(oid2)));
} finally {
IOUtils.closeQuietly(resp);
}

}

@Test(expected = InvalidRequestException.class)
public void testSearchWithInvalidSort() throws Exception {
Observation o = new Observation();
Expand Down
76 changes: 76 additions & 0 deletions hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/NumberParamTest.java
View file
@@ -0,0 +1,76 @@
package ca.uhn.fhir.rest.param;

import static org.junit.Assert.assertEquals;

import java.math.BigDecimal;

import org.junit.AfterClass;
import org.junit.Test;

import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.util.TestUtil;

public class NumberParamTest {
private static FhirContext ourCtx = FhirContext.forDstu3();

@Test
public void testFull() {
NumberParam p = new NumberParam();
p.setValueAsQueryToken(ourCtx, null, null, "<5.4");
assertEquals(ParamPrefixEnum.LESSTHAN, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("lt5.4", p.getValueAsQueryToken(ourCtx));
}

@Test
public void testApproximateLegacy() {
NumberParam p = new NumberParam();
p.setValueAsQueryToken(ourCtx, null, null, "~5.4");
assertEquals(null,p.getComparator());
assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("ap5.4", p.getValueAsQueryToken(ourCtx));
}

@Test
public void testApproximate() {
NumberParam p = new NumberParam();
p.setValueAsQueryToken(ourCtx, null, null, "ap5.4");
assertEquals(null,p.getComparator());
assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("ap5.4", p.getValueAsQueryToken(ourCtx));
}

@Test
public void testNoQualifier() {
NumberParam p = new NumberParam();
p.setValueAsQueryToken(ourCtx, null, null, "5.4");
assertEquals(null, p.getComparator());
assertEquals(null, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("5.4", p.getValueAsQueryToken(ourCtx));
}


/**
* See #696
*/
@Test
public void testNegativeNumber() {
NumberParam p = new NumberParam();
p.setValueAsQueryToken(ourCtx, null, null, "-5.4");
assertEquals(null, p.getComparator());
assertEquals(null, p.getPrefix());
assertEquals("-5.4", p.getValue().toPlainString());
assertEquals(new BigDecimal("-5.4"), p.getValue());
assertEquals("-5.4", p.getValueAsQueryToken(ourCtx));
}


@AfterClass
public static void afterClassClearContext() {
TestUtil.clearAllStaticFieldsForUnitTest();
}

}
74 changes: 70 additions & 4 deletions hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java
View file
Expand Up @@ -2,6 +2,8 @@

import static org.junit.Assert.*;

import java.math.BigDecimal;

import org.junit.AfterClass;
import org.junit.Test;

Expand All @@ -10,37 +12,52 @@
import ca.uhn.fhir.util.TestUtil;

public class QuantityParamTest {
private static FhirContext ourCtx = FhirContext.forDstu1();
private static FhirContext ourCtx = FhirContext.forDstu3();

@Test
public void testFull() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "<5.4|http://unitsofmeasure.org|mg");
assertEquals(QuantityCompararatorEnum.LESSTHAN,p.getComparator());
assertEquals(ParamPrefixEnum.LESSTHAN, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("http://unitsofmeasure.org", p.getSystem());
assertEquals("mg", p.getUnits());
assertEquals("<5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx));
assertEquals("lt5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx));
}

@Test
public void testApproximate() {
public void testApproximateLegacy() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "~5.4|http://unitsofmeasure.org|mg");
assertEquals(null,p.getComparator());
assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix());
assertEquals(true, p.isApproximate());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("http://unitsofmeasure.org", p.getSystem());
assertEquals("mg", p.getUnits());
assertEquals("~5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx));
assertEquals("ap5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx));
}

@Test
public void testApproximate() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "ap5.4|http://unitsofmeasure.org|mg");
assertEquals(null,p.getComparator());
assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix());
assertEquals(true, p.isApproximate());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("http://unitsofmeasure.org", p.getSystem());
assertEquals("mg", p.getUnits());
assertEquals("ap5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx));
}

@Test
public void testNoQualifier() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "5.4|http://unitsofmeasure.org|mg");
assertEquals(null, p.getComparator());
assertEquals(null, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals("http://unitsofmeasure.org", p.getSystem());
assertEquals("mg", p.getUnits());
Expand All @@ -53,6 +70,7 @@ public void testNoUnits() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "5.4");
assertEquals(null, p.getComparator());
assertEquals(null, p.getPrefix());
assertEquals("5.4", p.getValue().toPlainString());
assertEquals(null, p.getSystem());
assertEquals(null, p.getUnits());
Expand Down Expand Up @@ -84,6 +102,54 @@ public void testNoSystem() {
assertEquals("mg", param.getUnits());
}

/**
* See #696
*/
@Test
public void testNegativeQuantityWithUnits() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "-5.4|http://unitsofmeasure.org|mg");
assertEquals(null, p.getComparator());
assertEquals(null, p.getPrefix());
assertEquals("-5.4", p.getValue().toPlainString());
assertEquals(new BigDecimal("-5.4"), p.getValue());
assertEquals("http://unitsofmeasure.org", p.getSystem());
assertEquals("mg", p.getUnits());
assertEquals("-5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx));
}

/**
* See #696
*/
@Test
public void testNegativeQuantityWithoutUnits() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "-5.4");
assertEquals(null, p.getComparator());
assertEquals(null, p.getPrefix());
assertEquals("-5.4", p.getValue().toPlainString());
assertEquals(new BigDecimal("-5.4"), p.getValue());
assertEquals(null, p.getSystem());
assertEquals(null, p.getUnits());
assertEquals("-5.4||", p.getValueAsQueryToken(ourCtx));
}

/**
* See #696
*/
@Test
public void testNegativeQuantityWithoutUnitsWithComparator() {
QuantityParam p = new QuantityParam();
p.setValueAsQueryToken(ourCtx, null, null, "gt-5.4");
assertEquals(QuantityCompararatorEnum.GREATERTHAN, p.getComparator());
assertEquals(ParamPrefixEnum.GREATERTHAN, p.getPrefix());
assertEquals("-5.4", p.getValue().toPlainString());
assertEquals(new BigDecimal("-5.4"), p.getValue());
assertEquals(null, p.getSystem());
assertEquals(null, p.getUnits());
assertEquals("gt-5.4||", p.getValueAsQueryToken(ourCtx));
}

@AfterClass
public static void afterClassClearContext() {
TestUtil.clearAllStaticFieldsForUnitTest();
Expand Down
5 changes: 5 additions & 0 deletions src/changes/changes.xml
View file
Expand Up @@ -188,6 +188,11 @@
instead of the previous
<![CDATA[<code>Bundle.entry.resource</code>]]>
<action>
<action type="fix" issue="696">
An issue was corrected where search parameters containing negative numbers
were sometimes treated as positive numbers when processing the search. Thanks
to Keith Boone for reporting and suggesting a fix!
</action>
</release>
<release version="2.5" date="2017-06-08">
<action type="fix">
Expand Down

0 comments on commit 274e218

Please sign in to comment.