-
Notifications
You must be signed in to change notification settings - Fork 170
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.
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
Add more lenient coercing #1172
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,12 @@ protected BigDecimal coerceToBigDecimal(Object value) { | |
if (value instanceof DummyObject) { | ||
return BigDecimal.ZERO; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimCommas((String) value); | ||
return super.coerceToBigDecimal(sanitizedValue); | ||
} | ||
|
||
return super.coerceToBigDecimal(value); | ||
} | ||
|
||
|
@@ -41,6 +47,12 @@ protected BigInteger coerceToBigInteger(Object value) { | |
if (value instanceof DummyObject) { | ||
return BigInteger.ZERO; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimDecimal(trimCommas((String) value)); | ||
return super.coerceToBigInteger(sanitizedValue); | ||
} | ||
|
||
return super.coerceToBigInteger(value); | ||
} | ||
|
||
|
@@ -49,6 +61,12 @@ protected Double coerceToDouble(Object value) { | |
if (value instanceof DummyObject) { | ||
return 0d; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimCommas((String) value); | ||
return super.coerceToDouble(sanitizedValue); | ||
} | ||
|
||
return super.coerceToDouble(value); | ||
} | ||
|
||
|
@@ -57,6 +75,12 @@ protected Float coerceToFloat(Object value) { | |
if (value instanceof DummyObject) { | ||
return 0f; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimCommas((String) value); | ||
return super.coerceToFloat(sanitizedValue); | ||
} | ||
|
||
return super.coerceToFloat(value); | ||
} | ||
|
||
|
@@ -65,6 +89,11 @@ protected Long coerceToLong(Object value) { | |
if (value instanceof DummyObject) { | ||
return 0L; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimDecimal(trimCommas((String) value)); | ||
return super.coerceToLong(sanitizedValue); | ||
} | ||
return super.coerceToLong(value); | ||
} | ||
|
||
|
@@ -73,6 +102,11 @@ protected Integer coerceToInteger(Object value) { | |
if (value instanceof DummyObject) { | ||
return 0; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimDecimal(trimCommas((String) value)); | ||
return super.coerceToInteger(sanitizedValue); | ||
} | ||
return super.coerceToInteger(value); | ||
} | ||
|
||
|
@@ -81,6 +115,11 @@ protected Short coerceToShort(Object value) { | |
if (value instanceof DummyObject) { | ||
return 0; | ||
} | ||
|
||
if (value instanceof String) { | ||
String sanitizedValue = trimDecimal(trimCommas((String) value)); | ||
return super.coerceToShort(sanitizedValue); | ||
} | ||
return super.coerceToShort(value); | ||
} | ||
|
||
|
@@ -89,9 +128,36 @@ protected Byte coerceToByte(Object value) { | |
if (value instanceof DummyObject) { | ||
return 0; | ||
} | ||
if (value instanceof String) { | ||
String sanitizedValue = trimDecimal(trimCommas((String) value)); | ||
return super.coerceToByte(sanitizedValue); | ||
} | ||
Comment on lines
+131
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think we should do this when coercing to byte |
||
|
||
return super.coerceToByte(value); | ||
} | ||
|
||
private String trimCommas(String value) { | ||
if (!value.contains(",")) { | ||
return value; | ||
} | ||
|
||
if (value.matches(".*\\..*,.*")) { | ||
return value; | ||
} | ||
Comment on lines
+144
to
+146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer precompiled regex expressions |
||
return value.replaceAll(",", ""); | ||
} | ||
|
||
private String trimDecimal(String value) { | ||
if (!value.contains(".")) { | ||
return value; | ||
} | ||
|
||
if (!value.matches("\\d*\\.\\d*")) { | ||
return value; | ||
} | ||
Comment on lines
+155
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer precompiled regex expressions |
||
return value.substring(0, value.indexOf(".")); | ||
} | ||
|
||
@Override | ||
protected String coerceToString(Object value) { | ||
if (value instanceof DummyObject) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,17 @@ | ||
package com.hubspot.jinjava.lib.exptest; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
import com.google.common.collect.ImmutableMap; | ||
import com.hubspot.jinjava.BaseJinjavaTest; | ||
import com.hubspot.jinjava.interpret.FatalTemplateErrorsException; | ||
import com.hubspot.jinjava.objects.date.PyishDate; | ||
import java.time.Instant; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
import javax.el.ELException; | ||
import javax.el.MethodNotFoundException; | ||
import org.junit.Test; | ||
|
||
public class ComparisonExpTestsTest extends BaseJinjavaTest { | ||
|
@@ -66,4 +70,27 @@ public void testAliases() { | |
assertThat(jinjava.render("{{ 4 is >= 5 }}", new HashMap<>())).isEqualTo("false"); | ||
assertThat(jinjava.render("{{ 4 is != 5 }}", new HashMap<>())).isEqualTo("true"); | ||
} | ||
|
||
@Test | ||
public void itParsesFormattedNumbers() { | ||
assertThat(jinjava.render("{{ \"1,050.25\" is ge 4 }}", new HashMap<>())) | ||
.isEqualTo("true"); | ||
assertThat(jinjava.render("{{ \"4.1\" is gt 4 }}", new HashMap<>())) | ||
.isEqualTo("false"); | ||
assertThat(jinjava.render("{{ 4.0 is le 5.00 }}", new HashMap<>())).isEqualTo("true"); | ||
assertThat(jinjava.render("{{ \"4,500.75\" is le 10000.00 }}", new HashMap<>())) | ||
.isEqualTo("true"); | ||
} | ||
|
||
@Test | ||
public void itDoesNotAllowCommasAfterDecimalPointInNumbers() { | ||
assertThat( | ||
jinjava.renderForResult("{{ \"150.25,0\" is ge 4 }}", new HashMap<>()).getErrors() | ||
) | ||
.isNotEmpty(); | ||
Comment on lines
+87
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some languages have decimals and commas flipped and should be handled in the opposite manner There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, would your suggested approach here be to make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or perhaps just a |
||
assertThat( | ||
jinjava.renderForResult("{{ \"150.25,0\" is ge 4.0 }}", new HashMap<>()).getErrors() | ||
) | ||
.isNotEmpty(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer these to use similar logic as in
IntFilter
andFloatFilter
. Make use ofNumberFormat#parse