From 965e49e2ca032eb30fc3eaed42343d37cf9542c9 Mon Sep 17 00:00:00 2001 From: mattg Date: Mon, 27 Mar 2017 20:42:31 -0400 Subject: [PATCH] Fixes Issue #234 using late binding, ideally this might be a lambda that encapsulates its state -- given support for JDK 6+, its encapsulated state is maintained in an implementation of the interface ILateBindingValue, one for PATH functions one for JSON - its likely the JSON version doesn't have a purpose unless the JSON dynamically changes as a result of the function implementation but its better to create another impl for JSON if in the future JSON can be dynamically changed via functions(?) The fault is in the Parameter object which obviously cached its value and when the outside reference changes its oblivious to that state change due to its internal cached instance of the state. Cache.getCache()... that singleton call inside of JsonContext then grabs an instance of the cached parameter, it would be thread unsafe to simple invalidate the cache because who knows for whom the cache is being invalidated. Not caching function paths isn't an answer -- the input to a function could itself be another function - meaning the input (parameter) value would never be observed to the wrapping function and things such as take the $.max(3, 4, 5, $.avg(...)) would yield an answer without average being computed. --- .../java/com/jayway/jsonpath/JsonPath.java | 1 + .../jayway/jsonpath/internal/JsonContext.java | 5 ++- .../jsonpath/internal/function/Parameter.java | 12 +++--- .../internal/function/json/Append.java | 2 +- .../latebinding/ILateBindingValue.java | 10 +++++ .../latebinding/JsonLateBindingValue.java | 22 +++++++++++ .../latebinding/PathLateBindingValue.java | 33 ++++++++++++++++ .../function/numeric/AbstractAggregation.java | 4 +- .../internal/function/text/Concatenate.java | 4 +- .../internal/path/FunctionPathToken.java | 8 +++- .../jsonpath/internal/function/Issue234.java | 39 +++++++++++++++++++ 11 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/ILateBindingValue.java create mode 100644 json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/JsonLateBindingValue.java create mode 100644 json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/PathLateBindingValue.java create mode 100644 json-path/src/test/java/com/jayway/jsonpath/internal/function/Issue234.java diff --git a/json-path/src/main/java/com/jayway/jsonpath/JsonPath.java b/json-path/src/main/java/com/jayway/jsonpath/JsonPath.java index 8ce853d5..bc4e6144 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/JsonPath.java +++ b/json-path/src/main/java/com/jayway/jsonpath/JsonPath.java @@ -21,6 +21,7 @@ import com.jayway.jsonpath.internal.Path; import com.jayway.jsonpath.internal.PathRef; import com.jayway.jsonpath.internal.Utils; import com.jayway.jsonpath.internal.path.PathCompiler; +import com.jayway.jsonpath.spi.cache.Cache; import com.jayway.jsonpath.spi.json.JsonProvider; import java.io.File; diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/JsonContext.java b/json-path/src/main/java/com/jayway/jsonpath/internal/JsonContext.java index 50b84a08..6f2706b1 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/JsonContext.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/JsonContext.java @@ -26,6 +26,7 @@ import com.jayway.jsonpath.ReadContext; import com.jayway.jsonpath.TypeRef; import com.jayway.jsonpath.spi.cache.Cache; import com.jayway.jsonpath.spi.cache.CacheProvider; +import com.jayway.jsonpath.spi.cache.LRUCache; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -146,8 +147,8 @@ public class JsonContext implements ParseContext, DocumentContext { if(jsonPath != null){ return read(jsonPath); } else { - jsonPath = compile(path, filters); - cache.put(cacheKey, jsonPath); + jsonPath = compile(path, filters); + cache.put(cacheKey, jsonPath); return read(jsonPath); } diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/Parameter.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/Parameter.java index e8eabf97..75824ff8 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/function/Parameter.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/Parameter.java @@ -1,6 +1,8 @@ package com.jayway.jsonpath.internal.function; import com.jayway.jsonpath.internal.Path; +import com.jayway.jsonpath.internal.function.latebinding.ILateBindingValue; +import com.jayway.jsonpath.internal.function.latebinding.PathLateBindingValue; /** * Created by matt@mjgreenwood.net on 12/10/15. @@ -8,7 +10,7 @@ import com.jayway.jsonpath.internal.Path; public class Parameter { private ParamType type; private Path path; - private Object cachedValue; + private ILateBindingValue lateBinding; private Boolean evaluated = false; private String json; @@ -24,12 +26,12 @@ public class Parameter { this.type = ParamType.PATH; } - public Object getCachedValue() { - return cachedValue; + public Object getValue() { + return lateBinding.get(); } - public void setCachedValue(Object cachedValue) { - this.cachedValue = cachedValue; + public void setLateBinding(ILateBindingValue lateBinding) { + this.lateBinding = lateBinding; } public Path getPath() { diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/json/Append.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/json/Append.java index 805226e4..ed39d4a8 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/function/json/Append.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/json/Append.java @@ -22,7 +22,7 @@ public class Append implements PathFunction { for (Parameter param : parameters) { if (jsonProvider.isArray(model)) { int len = jsonProvider.length(model); - jsonProvider.setArrayIndex(model, len, param.getCachedValue()); + jsonProvider.setArrayIndex(model, len, param.getValue()); } } } diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/ILateBindingValue.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/ILateBindingValue.java new file mode 100644 index 00000000..0d0ed403 --- /dev/null +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/ILateBindingValue.java @@ -0,0 +1,10 @@ +package com.jayway.jsonpath.internal.function.latebinding; + +/** + * Obtain the late binding value at runtime rather than storing the value in the cache thus trashing the cache + * + * Created by mattg on 3/27/17. + */ +public interface ILateBindingValue { + Object get(); +} diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/JsonLateBindingValue.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/JsonLateBindingValue.java new file mode 100644 index 00000000..ef09725e --- /dev/null +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/JsonLateBindingValue.java @@ -0,0 +1,22 @@ +package com.jayway.jsonpath.internal.function.latebinding; + +import com.jayway.jsonpath.internal.function.Parameter; +import com.jayway.jsonpath.spi.json.JsonProvider; + +/** + * Created by mattg on 3/27/17. + */ +public class JsonLateBindingValue implements ILateBindingValue { + private final JsonProvider jsonProvider; + private final Parameter jsonParameter; + + public JsonLateBindingValue(JsonProvider jsonProvider, Parameter jsonParameter) { + this.jsonProvider = jsonProvider; + this.jsonParameter = jsonParameter; + } + + @Override + public Object get() { + return jsonProvider.parse(jsonParameter.getJson()); + } +} diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/PathLateBindingValue.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/PathLateBindingValue.java new file mode 100644 index 00000000..58c70018 --- /dev/null +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/latebinding/PathLateBindingValue.java @@ -0,0 +1,33 @@ +package com.jayway.jsonpath.internal.function.latebinding; + +import com.jayway.jsonpath.Configuration; +import com.jayway.jsonpath.internal.Path; +import com.jayway.jsonpath.internal.function.ParamType; + +/** + * Defines the contract for late bindings, provides document state and enough context to perform the evaluation at a later + * date such that we can operate on a dynamically changing value. + * + * Acts like a lambda function with references, but since we're supporting JDK 6+, we're left doing this... + * + * Created by mattg on 3/27/17. + */ +public class PathLateBindingValue implements ILateBindingValue { + private final Path path; + private final Object rootDocument; + private final Configuration configuration; + + public PathLateBindingValue(ParamType type, Path path, Object rootDocument, Configuration configuration) { + this.path = path; + this.rootDocument = rootDocument; + this.configuration = configuration; + } + + /** + * Evaluate the field type + * @return + */ + public Object get() { + return path.evaluate(rootDocument, rootDocument, configuration).getValue(); + } +} diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/numeric/AbstractAggregation.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/numeric/AbstractAggregation.java index 78d9da99..1215f434 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/function/numeric/AbstractAggregation.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/numeric/AbstractAggregation.java @@ -49,8 +49,8 @@ public abstract class AbstractAggregation implements PathFunction { } if (parameters != null) { for (Parameter param : parameters) { - if (param.getCachedValue() instanceof Number) { - Number value = (Number)param.getCachedValue(); + if (param.getValue() instanceof Number) { + Number value = (Number)param.getValue(); count++; next(value); } diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/function/text/Concatenate.java b/json-path/src/main/java/com/jayway/jsonpath/internal/function/text/Concatenate.java index 6c2dc1c4..19689edf 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/function/text/Concatenate.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/function/text/Concatenate.java @@ -27,8 +27,8 @@ public class Concatenate implements PathFunction { } if (parameters != null) { for (Parameter param : parameters) { - if (param.getCachedValue() != null) { - result.append(param.getCachedValue().toString()); + if (param.getValue() != null) { + result.append(param.getValue().toString()); } } } diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/path/FunctionPathToken.java b/json-path/src/main/java/com/jayway/jsonpath/internal/path/FunctionPathToken.java index 8e9055ed..af8d073d 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/path/FunctionPathToken.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/path/FunctionPathToken.java @@ -4,6 +4,8 @@ import com.jayway.jsonpath.internal.PathRef; import com.jayway.jsonpath.internal.function.Parameter; import com.jayway.jsonpath.internal.function.PathFunction; import com.jayway.jsonpath.internal.function.PathFunctionFactory; +import com.jayway.jsonpath.internal.function.latebinding.JsonLateBindingValue; +import com.jayway.jsonpath.internal.function.latebinding.PathLateBindingValue; import java.util.List; @@ -49,11 +51,13 @@ public class FunctionPathToken extends PathToken { if (!param.hasEvaluated()) { switch (param.getType()) { case PATH: - param.setCachedValue(param.getPath().evaluate(ctx.rootDocument(), ctx.rootDocument(), ctx.configuration()).getValue()); + //param.setCachedValue(param.getPath().evaluate(ctx.rootDocument(), ctx.rootDocument(), ctx.configuration()).getValue()); + param.setLateBinding(new PathLateBindingValue(param.getType(), param.getPath(), ctx.rootDocument(), ctx.configuration())); param.setEvaluated(true); break; case JSON: - param.setCachedValue(ctx.configuration().jsonProvider().parse(param.getJson())); +// ctx.configuration().jsonProvider().parse(param.getJson()) + param.setLateBinding(new JsonLateBindingValue(ctx.configuration().jsonProvider(), param)); param.setEvaluated(true); break; } diff --git a/json-path/src/test/java/com/jayway/jsonpath/internal/function/Issue234.java b/json-path/src/test/java/com/jayway/jsonpath/internal/function/Issue234.java new file mode 100644 index 00000000..17e1e54a --- /dev/null +++ b/json-path/src/test/java/com/jayway/jsonpath/internal/function/Issue234.java @@ -0,0 +1,39 @@ +package com.jayway.jsonpath.internal.function; + +import com.jayway.jsonpath.JsonPath; +import org.assertj.core.util.Maps; +import org.junit.Test; + +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * TDD for Issue #234 + * + * Verifies the use-case where-in the function path expression is cached and re-used but the JsonPath includes a function + * whose arguments are then dependent upon state that changes externally from the internal Cache.getCache state. The + * prior implementation had a bug where-in the parameter values were cached -- the present implementation (as of Issue #234) + * now uses a late binding approach to eval the function parameters. Cache invalidation isn't an option given the need + * for nested function calls. + * + * Once this bug is fixed, most of the concern then centers around the need to ensure nested functions are processed + * correctly. + * + * @see NestedFunctionTest for examples of where that is performed. + * + * Created by mattg on 3/27/17. + */ +public class Issue234 { + + @Test + public void testIssue234() { + Map context = Maps.newHashMap(); + context.put("key", "first"); + Object value = JsonPath.read(context, "concat(\"/\", $.key)"); + assertThat(value).isEqualTo("/first"); + context.put("key", "second"); + value = JsonPath.read(context, "concat(\"/\", $.key)"); + assertThat(value).isEqualTo("/second"); + } +}