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.

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 warnings when datetime-related filters called with null arguments #1064

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
13 changes: 13 additions & 0 deletions src/main/java/com/hubspot/jinjava/interpret/TemplateError.java
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,19 @@ public static TemplateError fromInvalidInputException(InvalidInputException ex)
);
}

public static TemplateError fromMissingFilterArgException(InvalidArgumentException ex) {
return new TemplateError(
ErrorType.WARNING,
ErrorReason.INVALID_ARGUMENT,
ErrorItem.FILTER,
ex.getMessage(),
ex.getName(),
ex.getLineNumber(),
ex.getStartPosition(),
ex
);
}

public static TemplateError fromException(Exception ex) {
int lineNumber = -1;
int startPosition = -1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.hubspot.jinjava.interpret.InvalidArgumentException;
import com.hubspot.jinjava.interpret.InvalidReason;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import com.hubspot.jinjava.interpret.TemplateError;
import com.hubspot.jinjava.interpret.TemplateSyntaxException;
import com.hubspot.jinjava.lib.fn.Functions;
import com.hubspot.jinjava.objects.date.PyishDate;
Expand Down Expand Up @@ -52,8 +53,8 @@ public Object filter(
);
}

ZonedDateTime start = getZonedDateTime(var);
ZonedDateTime end = getZonedDateTime(args[0]);
ZonedDateTime start = getZonedDateTime(var, "begin");
ZonedDateTime end = getZonedDateTime(args[0], "end");

Object args1 = args[1];
if (args1 == null) {
Expand All @@ -64,12 +65,24 @@ public Object filter(
return temporalUnit.between(start, end);
}

private ZonedDateTime getZonedDateTime(Object var) {
private ZonedDateTime getZonedDateTime(Object var, String position) {
if (var instanceof ZonedDateTime) {
return (ZonedDateTime) var;
} else if (var instanceof PyishDate) {
return ((PyishDate) var).toDateTime();
} else {
JinjavaInterpreter interpreter = JinjavaInterpreter.getCurrent();

interpreter.addError(
TemplateError.fromMissingFilterArgException(
new InvalidArgumentException(
interpreter,
getName() + " filter called with null " + position,
getName()
)
)
);

return Functions.getDateTimeArg(var, ZoneOffset.UTC);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.hubspot.jinjava.JinjavaConfig;
import com.hubspot.jinjava.interpret.InvalidArgumentException;
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import com.hubspot.jinjava.interpret.TemplateError;
import com.hubspot.jinjava.lib.fn.Functions;
import com.hubspot.jinjava.objects.date.InvalidDateFormatException;
import java.time.DateTimeException;
Expand Down Expand Up @@ -92,4 +93,20 @@ private DateTimeFormatter buildFormatter(String format) {
}
}
}

public void checkForNullVar(Object var, String name) {
if (var == null) {
JinjavaInterpreter
.getCurrent()
.addError(
TemplateError.fromMissingFilterArgException(
new InvalidArgumentException(
JinjavaInterpreter.getCurrent(),
name,
name + " filter called with null value"
)
)
);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class FormatDateFilter implements Filter {

@Override
public Object filter(Object var, JinjavaInterpreter interpreter, String... args) {
HELPER.checkForNullVar(var, NAME);
return format(var, args);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public Object filter(Object var, JinjavaInterpreter interpreter, String... args)
}

public static Object format(Object var, String... args) {
HELPER.checkForNullVar(var, NAME);
return HELPER.format(var, args);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public Object filter(Object var, JinjavaInterpreter interpreter, String... args)
}

public static Object format(Object var, String... args) {
HELPER.checkForNullVar(var, NAME);
return HELPER.format(var, args);
}

Expand Down
34 changes: 30 additions & 4 deletions src/main/java/com/hubspot/jinjava/lib/fn/Functions.java
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,20 @@ public static String dateTimeFormat(Object var, String... format) {
zoneOffset = ((PyishDate) var).toDateTime().getZone();
}

if (var == null) {
JinjavaInterpreter
.getCurrent()
.addError(
TemplateError.fromMissingFilterArgException(
new InvalidArgumentException(
JinjavaInterpreter.getCurrent(),
"datetimeformat",
"datetimeformat filter called with null datetime"
)
)
);
}

ZonedDateTime d = getDateTimeArg(var, zoneOffset);
if (d == null) {
return "";
Expand Down Expand Up @@ -283,10 +297,22 @@ public static ZonedDateTime getDateTimeArg(Object var, ZoneId zoneOffset) {
}
)
public static long unixtimestamp(Object... var) {
ZonedDateTime d = getDateTimeArg(
var == null || var.length == 0 ? null : var[0],
ZoneOffset.UTC
);
Object filterVar = var == null || var.length == 0 ? null : var[0];

if (filterVar == null) {
JinjavaInterpreter
.getCurrent()
.addError(
TemplateError.fromInvalidArgumentException(
new InvalidArgumentException(
JinjavaInterpreter.getCurrent(),
"unixtimestamp",
"unixtimestamp filter called with null value"
)
)
);
}
ZonedDateTime d = getDateTimeArg(filterVar, ZoneOffset.UTC);

if (d == null) {
return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,10 @@
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;

public class BetweenTimesFilterTest extends BaseJinjavaTest {

@Before
public void setup() {
jinjava.getGlobalContext().registerClasses(EscapeJsFilter.class);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this was copied from some other test

}

@Test
public void itGetsDurationBetweenTimes() {
long timestamp = 1543354954000L;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,29 +30,31 @@ public void setup() {
}

@Test
public void itUsesTodayIfNoDateProvided() throws Exception {
public void itUsesTodayIfNoDateProvided() {
assertThat(filter.filter(null, interpreter))
.isEqualTo(StrftimeFormatter.format(ZonedDateTime.now(ZoneOffset.UTC)));
assertThat(interpreter.getErrors().get(0).getMessage())
.contains("datetimeformat filter called with null datetime");
}

@Test
public void itSupportsLongAsInput() throws Exception {
public void itSupportsLongAsInput() {
assertThat(filter.filter(d, interpreter)).isEqualTo(StrftimeFormatter.format(d));
}

@Test
public void itUsesDefaultFormatStringIfNoneSpecified() throws Exception {
public void itUsesDefaultFormatStringIfNoneSpecified() {
assertThat(filter.filter(d, interpreter)).isEqualTo("14:22 / 06-11-2013");
}

@Test
public void itUsesSpecifiedFormatString() throws Exception {
public void itUsesSpecifiedFormatString() {
assertThat(filter.filter(d, interpreter, "%B %d, %Y, at %I:%M %p"))
.isEqualTo("November 06, 2013, at 02:22 PM");
}

@Test
public void itHandlesVarsAndLiterals() throws Exception {
public void itHandlesVarsAndLiterals() {
interpreter.getContext().put("d", d);
interpreter.getContext().put("foo", "%Y-%m");

Expand All @@ -64,7 +66,7 @@ public void itHandlesVarsAndLiterals() throws Exception {
}

@Test
public void itSupportsTimezones() throws Exception {
public void itSupportsTimezones() {
assertThat(filter.filter(1539277785000L, interpreter, "%B %d, %Y, at %I:%M %p"))
.isEqualTo("October 11, 2018, at 05:09 PM");
assertThat(
Expand All @@ -83,7 +85,7 @@ public void itSupportsTimezones() throws Exception {
}

@Test(expected = InvalidArgumentException.class)
public void itThrowsExceptionOnInvalidTimezone() throws Exception {
public void itThrowsExceptionOnInvalidTimezone() {
filter.filter(
1539277785000L,
interpreter,
Expand All @@ -93,7 +95,7 @@ public void itThrowsExceptionOnInvalidTimezone() throws Exception {
}

@Test(expected = InvalidDateFormatException.class)
public void itThrowsExceptionOnInvalidDateformat() throws Exception {
public void itThrowsExceptionOnInvalidDateformat() {
filter.filter(1539277785000L, interpreter, "Not a format");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,10 @@
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;

public class MinusTimeFilterTest extends BaseJinjavaTest {

@Before
public void setup() {
jinjava.getGlobalContext().registerClasses(EscapeJsFilter.class);
}

@Test
public void itSubtractsTime() {
long timestamp = 1543352736000L;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,10 @@
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;

public class PlusTimeFilterTest extends BaseJinjavaTest {

@Before
public void setup() {
jinjava.getGlobalContext().registerClasses(EscapeJsFilter.class);
}

@Test
public void itAddsTime() {
long timestamp = 1543352736000L;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.hubspot.jinjava.objects.date.FixedDateTimeProvider;
import java.time.ZonedDateTime;
import org.assertj.core.data.Offset;
import org.junit.After;
import org.junit.Test;

public class UnixTimestampFunctionTest {
Expand All @@ -19,8 +20,20 @@ public class UnixTimestampFunctionTest {
);
private final long epochMilliseconds = d.toEpochSecond() * 1000 + 345;

@After
public void tearDown() {
JinjavaInterpreter.popCurrent();
}

@Test
public void itGetsUnixTimestamps() {
JinjavaInterpreter.pushCurrent(
new JinjavaInterpreter(
new Jinjava(),
new Context(),
JinjavaConfig.newBuilder().build()
)
);
assertThat(Functions.unixtimestamp())
.isGreaterThan(0)
.isLessThanOrEqualTo(System.currentTimeMillis());
Expand All @@ -44,11 +57,7 @@ public void itUsesFixedDateTimeProvider() {
.build()
)
);
try {
assertThat(Functions.unixtimestamp((Object) null)).isEqualTo(ts);
} finally {
JinjavaInterpreter.popCurrent();
}
assertThat(Functions.unixtimestamp((Object) null)).isEqualTo(ts);
}

@Test(expected = DeferredValueException.class)
Expand All @@ -63,10 +72,6 @@ public void itDefersWhenExecutingEagerly() {
.build()
)
);
try {
Functions.unixtimestamp((Object) null);
} finally {
JinjavaInterpreter.popCurrent();
}
Functions.unixtimestamp((Object) null);
}
}