Introduce json path commons lib #11680

cgardens · 2022-04-02T18:56:12Z

What

With our heavy use of JSON, having a way query into JSON objects is valuable. The main case it has come up recently is being able to convert paths in JSONSchema into queries that we can use to query into configuration objects. For example if the json schema is something like:

"connectionSpecification": {
    "type": "object",
    "properties": {
      "username": {
        "type": "string"
      },
      "password": {
        "type": "string",
        "airbyte_secret": true
      }
    }
  }

We want to be able to say something like, give me all of the paths for a potential configuration object that matches this schema that is a secret. In this case it is just "$.password". We would then use that information to perform queries or replacements on the actual configuration object itself. e.g. { "username": "charles", "password": "hunter2" } => { "username": "charles", "password": "********" }. This gets more complicated as you have more complex JSON objects with nesting, arrays, etc. Right now we are pretty haphazard in solving this problem and have multiple implementations of how try to handle it. Introducing this will help consolidate it.

In order to do this we need some query language for json. The JSONPath specification does exactly that. More information about the specification can be found here. For those familiar with jq, JSONPath will be most recognizable as pretty much "that DSL that jq uses".

This PR introduces a java implementation of that specification (repository) called JsonPath. The goal of this PR is to introduce this library in such a way that JSONPath can be easily used by our developers without them having to be experts on that underlying library. Thus, I'm introducing common helper functions. I will be using these directly in subsequent PR for secrets improvements, but for now, I'm doing these as a separate PR for clarity.

Recommended reading order

JsonPaths.java
JsonPathsTest.java

cgardens · 2022-04-02T18:58:00Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+ * return a single value (see: {@link JsonPaths#getSingleValue(JsonNode, String)}). These should
+ * only be used if it is not possible for a query to return more than one value.
+ */
+public class JsonPaths {


I could see an argument for keeping this package private. I am not sure if this will mostly be used internally for the Jsons and JsonsSchemas helpers in the commons package versus actually being used in application code. Leaning a little towards starting package private and then opening up if it would be useful.

certainly seems easier to make open later than it would be to pull back into the private side

cgardens · 2022-04-02T18:59:28Z

airbyte-commons/src/test/java/io/airbyte/commons/json/JsonPathsTest.java

+
+class JsonPathsTest {
+
+  private static final String JSON = """


java does this now? I missed this. <3 java 15.

cgardens · 2022-04-02T18:59:55Z

airbyte-commons/src/test/java/io/airbyte/commons/json/JsonPathsTest.java

+    });
+  }
+
+  // todo (cgardens) - this behavior is a little unintuitive, but based on the docs, there's not an


not sure how intuitive this is to others. was a bit weird for me but inclined to leave it as be.

cgardens · 2022-04-02T19:00:28Z

build.gradle

@@ -272,6 +272,7 @@ subprojects {
        implementation 'com.fasterxml.jackson.core:jackson-annotations'
        implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml'
        implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310'
+        implementation 'com.jayway.jsonpath:json-path:2.7.0'


if we make this package private then we might actually just put it in the commons dependencies instead of the deps in the root of the project so that only commons has access to it.

benmoriceau · 2022-04-05T15:31:17Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+   *
+   * @param jsonPath - path to validate
+   */
+  public static void assertIsSingleReturnQuery(final String jsonPath) {


I haven't been through the entire review but don't we want to do that on the arrays?

Please ignore, I have seen where it is being use.

benmoriceau · 2022-04-05T15:36:13Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+    return getInternal(GET_PATHS_CONFIGURATION, json, jsonPath)
+        .stream()
+        .map(JsonNode::asText)
+        .collect(Collectors.toList());


nit, we can use toList() in java 17

benmoriceau · 2022-04-05T15:37:17Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+   * @param jsonPath - path into the json object. must be in the format of JSONPath.
+   * @return all paths that are present that match the input query
+   */
+  public static List<String> getPaths(final JsonNode json, final String jsonPath) {


Nit: Should we enforce it being a Set? The path should be uniq

yeah. good point.

actually upon further thought i think the list helps avoid bugs due to not determinitic ordering behavior. added to the java docs to explain it.

benmoriceau · 2022-04-05T15:47:57Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+   * @param jsonPath - path into the json object. must be in the format of JSONPath.
+   * @param replacement - a json node to replace the current value at the jsonPath
+   */
+  public static JsonNode replaceAtJsonNodeLoud(final JsonNode json, final String jsonPath, final JsonNode replacement) {


I don't understand the Loud suffix here, shouldn't we throw an exception if the path doesn't exist?

@benmoriceau maybe i am letting knowledge of how we are going to use this creep in. a common case is that we are going to get all the possible paths are are secrets from the jsonschema object. we are then going to see if each of those paths exist in the config object. if they do not exist then we simply want to do nothing (i.e. the oneOf case).

at that point we need a version of the method that fails loudly if the path doesn't exist and one that simply does nothing if the path doesn't exist. i think both are important and useful helpers. happy to adjust the naming convention. my intuition is the "loud" versions will be used less frequently than the none loud ones, which is why i chose to make that version more verbose.

benmoriceau · 2022-04-05T15:49:05Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+    try {
+      return replaceAtJsonNodeLoud(json, jsonPath, replacement);
+    } catch (final PathNotFoundException e) {
+      LOGGER.debug("Path not found", e);


Why is it a DEBUG log? Shouldn't we set it at least as a warn? It seems like a miss-usage of the library.

benmoriceau

Mainly nit comments.

benmoriceau · 2022-04-05T15:50:07Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+        }
+      }
+    } catch (final PathNotFoundException e) {
+      LOGGER.debug("Path not found", e);


Same comment than the previous warn.

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

benmoriceau · 2022-04-05T15:58:07Z

airbyte-commons/src/test/java/io/airbyte/commons/json/JsonPathsTest.java

+
+  @Test
+  void testGetValues() {
+    assertEquals(List.of(0, 1, 2), JsonPaths.getValues(JSON_NODE, LIST_ALL_QUERY).stream().map(JsonNode::asInt).collect(Collectors.toList()));


Nit: AssertJ can provide a slightly easier syntax, for example:

List<Integer> test = new ArrayList<>(); Assertions.assertThat(test).map(e -> e.toString()) .contains("test", "test");

supertopher · 2022-04-05T16:05:25Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+/**
+ * JSONPath is specification for querying JSON objects. More information about the specification can
+ * be found here: https://goessner.net/articles/JsonPath/. For those familiar with jq, JSONPath will
+ * be most recognizable as "that DSL that jq uses".


this is a helpful TLDR ☝️

supertopher

LGTM.

nits don't feel blocking to me

supertopher · 2022-04-05T16:07:21Z

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

+ * return a single value (see: {@link JsonPaths#getSingleValue(JsonNode, String)}). These should
+ * only be used if it is not possible for a query to return more than one value.
+ */
+public class JsonPaths {


certainly seems easier to make open later than it would be to pull back into the private side

airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPaths.java

…s.java Co-authored-by: Topher Lubaway <[email protected]>

cgardens added 3 commits April 1, 2022 12:15

add JsonPaths class; needs tests

40dffa5

add tests

0397dc6

copy clean up

e5ce9ae

cgardens commented Apr 2, 2022

View reviewed changes

cgardens temporarily deployed to more-secrets April 2, 2022 18:58 Inactive

cgardens temporarily deployed to more-secrets April 2, 2022 18:59 Inactive

cgardens commented Apr 2, 2022

View reviewed changes

cgardens requested review from benmoriceau, lmossman and supertopher April 2, 2022 19:02

benmoriceau reviewed Apr 5, 2022

View reviewed changes

supertopher reviewed Apr 5, 2022

View reviewed changes

supertopher approved these changes Apr 5, 2022

View reviewed changes

cgardens mentioned this pull request Apr 7, 2022

Java JSONSchema Traversal #9948

Closed

benmoriceau mentioned this pull request Apr 7, 2022

Extract the secret's json path from a config schema #11823

Closed

Update airbyte-commons/src/main/java/io/airbyte/commons/json/JsonPath…

e40372b

…s.java Co-authored-by: Topher Lubaway <[email protected]>

cgardens temporarily deployed to more-secrets April 8, 2022 16:51 Inactive

code review feedback

5703131

cgardens temporarily deployed to more-secrets April 8, 2022 17:51 Inactive

cgardens merged commit f512208 into master Apr 8, 2022

cgardens deleted the cgardens/json-path branch April 8, 2022 19:03

This was referenced Apr 8, 2022

Bump Airbyte version from 0.35.65-alpha to 0.35.66-alpha #11851

Closed

Bump Airbyte version from 0.35.65-alpha to 0.35.66-alpha #11862

Closed

Bump Airbyte version from 0.35.65-alpha to 0.35.66-alpha #11920

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce json path commons lib #11680

Introduce json path commons lib #11680

cgardens commented Apr 2, 2022 •

edited

Loading

cgardens Apr 2, 2022

supertopher Apr 5, 2022

cgardens Apr 2, 2022

cgardens Apr 2, 2022

cgardens Apr 2, 2022

benmoriceau Apr 5, 2022

benmoriceau Apr 5, 2022

benmoriceau Apr 5, 2022

benmoriceau Apr 5, 2022

cgardens Apr 8, 2022

cgardens Apr 8, 2022

benmoriceau Apr 5, 2022

cgardens Apr 8, 2022

benmoriceau Apr 5, 2022

benmoriceau left a comment

benmoriceau Apr 5, 2022

benmoriceau Apr 5, 2022

supertopher Apr 5, 2022

supertopher left a comment

supertopher Apr 5, 2022

Introduce json path commons lib #11680

Introduce json path commons lib #11680

Conversation

cgardens commented Apr 2, 2022 • edited Loading

What

Recommended reading order

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoriceau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supertopher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens commented Apr 2, 2022 •

edited

Loading