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

Adding support for custom path functions #286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jakeswenson
Copy link

Adding a configuration value for a path function factory

Adding a configuration value for a path function factory
public MapJsonPathFunctionFactory(
final Map<String, Class<? extends PathFunction>> pathFunctionMap) {

this.pathFunctionMap = pathFunctionMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you clone the list here so that modifications to the original map don't affect the factory.

public PathFunction newFunction(final String name) throws InvalidPathException {
Class functionClazz = pathFunctionMap.get(name);
if(functionClazz == null){
throw new InvalidPathException("Function with name: " + name + " does not exists.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "does not exist."

@@ -64,16 +64,8 @@
*
* @throws InvalidPathException
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should deprecate the whole class

@@ -25,11 +25,11 @@
*/
public class PathFunctionFactory {

public static final Map<String, Class> FUNCTIONS;
public static final Map<String, Class<? extends PathFunction>> FUNCTIONS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move the value/initialization to the Configuration class and just keep a reference here?

@kallestenflo
Copy link
Contributor

We need to come up with a SPI to prevent use of internal classes. 3.0 feature?

@jochenberger
Copy link
Contributor

LGTM, but I'd love to see some tests and maybe an addition to the README file with a code snippet that registers a new function?

@@ -0,0 +1,24 @@
package com.jayway.jsonpath.internal.function;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be moved to a package within com.jayway.jsonpath.spi, along with the implementation class.

@jochenberger
Copy link
Contributor

I see no harm in adding this in a minor release if it doesn't break any existing API, but maybe we should give it some more thought and not add it to 2.3.0 (which I hope will be ready pretty soon :-) ).

}

@Override
public PathFunction newFunction(final String name) throws InvalidPathException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I question whether PathFunction as an interface is an appropriate expression for a user facing function feature - there's some nuance to know in what context the function is being invoked that is internal state of the JsonPath library. Breaking apart the types of PathFunctions (aggregations vs straight calculations) might be a good way to tease apart the interface into something more meaningful for the end-user.

Also, does the function need to take parameters -- can a sub-select be a parameter to the function - expressing that in a more general way such that the consumer doesn't need to grok the internals of JsonPath would be ideal.

Certainly keep this method as all interfaces/implementations are a PathFunction, but I think you might want to introduce additional factory methods for aggregation vs straight calculation and then abstract away the nuance of accepting parameters.

@cristph
Copy link

cristph commented Jan 21, 2022

so this PR break down halfway ?

@ooooooook
Copy link

I use reflection to force add custom path functions. The implementation is not elegant, but it works.

Here is an example below:

pom.xml

<dependency>
    <groupId>com.jayway.jsonpath</groupId>
    <artifactId>json-path</artifactId>
    <version>2.8.0</version>
</dependency>

code:

import com.google.common.collect.ImmutableSet;
import com.jayway.jsonpath.*;
import com.jayway.jsonpath.internal.EvaluationContext;
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.spi.json.JacksonJsonProvider;
import com.jayway.jsonpath.spi.json.JsonProvider;
import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider;
import com.jayway.jsonpath.spi.mapper.MappingProvider;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.reflect.FieldUtils;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Set;

public class JsonPathConfig {

    static {
        initJsonPathConfig();
        addCustomFunctions();
    }

    private static void addCustomFunctions() {
        try {
            // force access unmodifiableMap!!
            @SuppressWarnings({"unchecked", "rawtypes"})
            Map<String, Class> m = (Map<String, Class>) FieldUtils.readField(PathFunctionFactory.FUNCTIONS, "m", true);
            m.put("copyN", CopyNFunction.class);
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }
    }

    private static void initJsonPathConfig() {
        // https://github.com/json-path/JsonPath#jsonprovider-spi
        Configuration.setDefaults(new Configuration.Defaults() {
            private final JsonProvider jsonProvider = new JacksonJsonProvider();
            private final MappingProvider mappingProvider = new JacksonMappingProvider();

            @Override
            public JsonProvider jsonProvider() {
                return jsonProvider;
            }

            @Override
            public MappingProvider mappingProvider() {
                return mappingProvider;
            }

            @Override
            public Set<Option> options() {
                return ImmutableSet.of(Option.DEFAULT_PATH_LEAF_TO_NULL);
            }
        });
    }

    public static class CopyNFunction implements PathFunction {

        @Override
        public Object invoke(String currentPath, PathRef parent, Object model, EvaluationContext ctx, List<Parameter> parameters) {
            if (parameters == null || parameters.size() != 1) {
                throw new InvalidPathException("copyN function accept 1 parameter, current: " + CollectionUtils.size(parameters));
            }
            Object parameter = parameters.get(0).getValue();
            if (!(parameter instanceof Number)) {
                throw new InvalidPathException("copyN function parameter type should be Number, current: " + parameter.getClass());
            }

            int size = ((Number) parameter).intValue();
            List<Object> list = new ArrayList<>();
            for (int i = 0; i < size; i++) {
                list.add(model);
            }
            return list;
        }

    }

    public static void main(String[] args) {
        String content = "{ \"foo\": \"bar\" }";
        DocumentContext context = JsonPath.parse(content);
        Object obj = context.read("$.foo.copyN(3)");
        System.out.println(obj);
    }

}

the JsonPath $.foo.copyN(3) output [bar, bar, bar]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants