eventmesh icon indicating copy to clipboard operation
eventmesh copied to clipboard

[ISSUE #4646] Add condition for transforming non-data values

Open pmupkin opened this issue 1 year ago • 19 comments

Fixes #4646

From a design perspective, the transformer needs to avoid converting non-data fields that users may attempt to transform. The original code allowed users to transform fields in the EventMesh; restrictions have been added. If users utilize non-data fields, an exception will be thrown.

pmupkin avatar Dec 11 '23 15:12 pmupkin

@pmupkin please fix the ci check error, and associate this pr with an issue first. https://eventmesh.apache.org/community/contribute/contribute this doc will help you fix the ci check error.

xwm1992 avatar Dec 12 '23 02:12 xwm1992

https://github.com/apache/eventmesh/blob/fdd87969e34e6e831a420aeb9cfbc6776ecaeec7/eventmesh-transformer/src/main/java/org/apache/eventmesh/transformer/JsonPathParser.java#L43-L61 I didn't understand the logic here, could you explain it? The code in line 44 uses Jackson to convert JSON format strings into JsonNode, which seems to have nothing to do with JSON path expressions.

pandaapo avatar Dec 12 '23 11:12 pandaapo

https://github.com/apache/eventmesh/blob/fdd87969e34e6e831a420aeb9cfbc6776ecaeec7/eventmesh-transformer/src/main/java/org/apache/eventmesh/transformer/JsonPathParser.java#L43-L61

I didn't understand the logic here, could you explain it? The code in line 44 uses Jackson to convert JSON format strings into JsonNode, which seems to have nothing to do with JSON path expressions.

Originally, the tools for serializing JSON (Jackson) and extracting JSON (JsonPath) were both placed within a single class. Looking at it now, the naming indeed can be confusing. It might be beneficial to reorganize them separately in the future.

pmupkin avatar Dec 12 '23 14:12 pmupkin

@pmupkin

Originally, the tools for serializing JSON (Jackson) and extracting JSON (JsonPath) were both placed within a single class.

I don't think it's a big problem to put them both in JsonPathUtils, because some methods do use Jackson and JsonPath at the same time.

我觉得将它们都放到JsonPathUtils中问题不大,因为有些方法确实同时用到Jackson和JsonPath。

Looking at it now, the naming indeed can be confusing. It might be beneficial to reorganize them separately in the future.

I don't think it's a confusing naming issue right now. JsonPathUtils#parseStrict() cannot handle Json path expressions, it only converts JSON strings into JsonNode, so if (valueText.startsWith("$") && !valueText.startsWith("$.data")) you add in this PR seems unnecessary.

More seriously, because the JsonPathUtils.parseStrict() function converts JSON strings to JsonNode, the data stored in the JsonPathParser's variablesList property does not have any JSON path. It makes JsonPathParser#match() method ineffective as it cannot retrieve any JSON paths from variablesList property, which are exactly what it needs.

I don't know if I misunderstood anything, I look forward to your reply.

我觉得现在不是困惑的命名问题。JsonPathUtils#parseStrict()就处理不了Json路径表达式,它只是将json字符串转成JsonNode,所以您在该PR中加上if (valueText.startsWith("$") && !valueText.startsWith("$.data")) 似乎是没必要的。 更严重的是,因为JsonPathUtils.parseStrict()功能是将json字符串转成JsonNode,所以JsonPathParservariablesList属性保存的数据中就没有json路径。这样JsonPathParser#match()方法就失效了,因为该方法从variablesList属性拿不到任何json路径,但这恰恰是它需要的。 我不知道我有没有哪里理解错了,期待您的回复。

pandaapo avatar Dec 12 '23 14:12 pandaapo

@pmupkin

Originally, the tools for serializing JSON (Jackson) and extracting JSON (JsonPath) were both placed within a single class.

I don't think it's a big problem to put them both in JsonPathUtils, because some methods do use Jackson and JsonPath at the same time.

我觉得将它们都放到JsonPathUtils中问题不大,因为有些方法确实同时用到Jackson和JsonPath。

Looking at it now, the naming indeed can be confusing. It might be beneficial to reorganize them separately in the future.

I don't think it's a confusing naming issue right now. JsonPathUtils#parseStrict() cannot handle Json path expressions, it only converts JSON strings into JsonNode, so if (valueText.startsWith("$") && !valueText.startsWith("$.data")) you add in this PR seems unnecessary.

More seriously, because the JsonPathUtils.parseStrict() function converts JSON strings to JsonNode, the data stored in the JsonPathParser's variablesList property does not have any JSON path. It makes JsonPathParser#match() method ineffective as it cannot retrieve any JSON paths from variablesList property, which are exactly what it needs.

I don't know if I misunderstood anything, I look forward to your reply.

我觉得现在不是困惑的命名问题。JsonPathUtils#parseStrict()就处理不了Json路径表达式,它只是将json字符串转成JsonNode,所以您在该PR中加上if (valueText.startsWith("$") && !valueText.startsWith("$.data")) 似乎是没必要的。 更严重的是,因为JsonPathUtils.parseStrict()功能是将json字符串转成JsonNode,所以JsonPathParservariablesList属性保存的数据中就没有json路径。这样JsonPathParser#match()方法就失效了,因为该方法从variablesList属性拿不到任何json路径,但这恰恰是它需要的。 我不知道我有没有哪里理解错了,期待您的回复。

The variablesList holds pairs of data from a JSON. Put simply, the initialization of JsonPathParser serves the following purposes:

  1. Checking strict adherence to JSON format.
  2. Serializing JSON for easy access to its keys and values.
  3. Storing the keys and values in variablesList. Ultimately, what gets stored in variablesList are the string representations of JSON keys and values.

The match function, on the other hand, is used to parse CloudEvents messages using JSONPath.

pmupkin avatar Dec 12 '23 15:12 pmupkin

The match function, on the other hand, is used to parse CloudEvents messages using JSONPath.

https://github.com/apache/eventmesh/blob/fdd87969e34e6e831a420aeb9cfbc6776ecaeec7/eventmesh-transformer/src/main/java/org/apache/eventmesh/transformer/JsonPathParser.java#L74-L83 @pmupkin This is the logic of match. The code in line 82 , it retrieves the content specified in the first parameter JSON based on the second parameter, JSON path. The second parameter, JSON path, is obtained from variablesList property of JsonPathParser. You also said that this attribute stores JSON keys and values without a JSON path, does this mean that the match method is invalid?

这是match的逻辑,在第82行代码,是根据第二个参数即JSON path,获取第一个参数JSON中指定的内容。第二个参数JSON path正是从JsonPathParservariablesList属性中获取。您也说该属性保存的是JSON键和值,没有JSON path,所以是不是意味着match方法失效了?

pandaapo avatar Dec 12 '23 15:12 pandaapo

The match function, on the other hand, is used to parse CloudEvents messages using JSONPath.

https://github.com/apache/eventmesh/blob/fdd87969e34e6e831a420aeb9cfbc6776ecaeec7/eventmesh-transformer/src/main/java/org/apache/eventmesh/transformer/JsonPathParser.java#L74-L83

@pmupkin This is the logic of match. The code in line 82 , it retrieves the content specified in the first parameter JSON based on the second parameter, JSON path. The second parameter, JSON path, is obtained from variablesList property of JsonPathParser. You also said that this attribute stores JSON keys and values without a JSON path, does this mean that the match method is invalid? 这是match的逻辑,在第82行代码,是根据第二个参数即JSON path,获取第一个参数JSON中指定的内容。第二个参数JSON path正是从JsonPathParservariablesList属性中获取。您也说该属性保存的是JSON键和值,没有JSON path,所以是不是意味着match方法失效了?

Regarding the code at line 82, for instance, if a value in the provided JSON format by the user is $.data.time, the JsonPath tool will be used to match corresponding values in CloudEvents. Could you provide an example illustrating the issue you mentioned?

pmupkin avatar Dec 12 '23 15:12 pmupkin

if a value in the provided JSON format by the user is $.data.time, ... Could you provide an example illustrating the issue you mentioned?

@pmupkin I'm a bit confused. Shouldn't the JSON format be like the following: {"a": "1", "b": {"c": "2"}, "d": []}? How is $.data.time in JSON format? Isn't it a JSON Path expression? Do you mean there is a value like $.data.time in JSON?

我有些困惑。JSON格式不应该是像这样吗{"a": "1", "b": {"c": "2"}, "d": []}$.data.time这怎么是JSON格式呢,这不是JSON Path表达式吗?难道您的意思是在JSON中存在$.data.time这样的value?

pandaapo avatar Dec 12 '23 15:12 pandaapo

$.data.time

if a value in the provided JSON format by the user is $.data.time, ... Could you provide an example illustrating the issue you mentioned?

@pmupkin I'm a bit confused. Shouldn't the JSON format be like the following: {"a": "1", "b": {"c": "2"}, "d": []}? How is $.data.time in JSON format? Isn't it a JSON Path expression? Do you mean there is a value like $.data.time in JSON?

我有些困惑。JSON格式不应该是像这样吗{"a": "1", "b": {"c": "2"}, "d": []}$.data.time这怎么是JSON格式呢,这不是JSON Path表达式吗?难道您的意思是在JSON中存在$.data.time这样的value?

Here's a fact: $.data.time is stored as a string value,like "{"time":"$.data.time"}" and ultimately, the $.data.time extracted can be used for JsonPath matching. That's my understanding.

pmupkin avatar Dec 12 '23 16:12 pmupkin

Here's a fact: $.data.time is stored as a string value,like "{"time":"$.data.time"}" and ultimately, the $.data.time extracted can be used for JsonPath matching. That's my understanding.

@pmupkin Could you provide one or more complete examples of Transformer rule writing? Then based on them, I will study again the code related to Transformer in your PR #4365 and the code in PR #4622. Thank you.

Addtionally, please pay attention to the first review comment above https://github.com/apache/eventmesh/pull/4637#issuecomment-1851215780.

您能否提供一个或多个完整的Transformer规则编写示例?然后我根据它们,再学习一遍您的PR #4365中有关Transformer的代码,以及PR #4622的代码。谢谢! 另外,请您注意一下上面第一条review意见https://github.com/apache/eventmesh/pull/4637#issuecomment-1851215780 。

pandaapo avatar Dec 13 '23 00:12 pandaapo

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (24873dd) 17.28% compared to head (f9cd804) 17.40%. Report is 16 commits behind head on master.

Files Patch % Lines
...ventmesh/connector/lark/sink/ImServiceHandler.java 64.28% 24 Missing and 1 partial :warning:
...nnector/lark/sink/connector/LarkSinkConnector.java 38.46% 8 Missing :warning:
...g/apache/eventmesh/transformer/JsonPathParser.java 66.66% 1 Missing and 1 partial :warning:
...onnector/lark/sink/config/SinkConnectorConfig.java 80.00% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4637      +/-   ##
============================================
+ Coverage     17.28%   17.40%   +0.12%     
- Complexity     1741     1762      +21     
============================================
  Files           792      797       +5     
  Lines         29697    29798     +101     
  Branches       2567     2579      +12     
============================================
+ Hits           5132     5187      +55     
- Misses        24090    24130      +40     
- Partials        475      481       +6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 18 '23 04:12 codecov[bot]

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

github-actions[bot] avatar Apr 06 '24 18:04 github-actions[bot]

Codecov Report

Attention: Patch coverage is 39.49447% with 766 lines in your changes missing coverage. Please review.

Project coverage is 17.40%. Comparing base (1600c6b) to head (f9cd804). Report is 4691 commits behind head on master.

Files with missing lines Patch % Lines
...mesh/connector/jdbc/connection/JdbcConnection.java 0.00% 137 Missing :warning:
...va/org/apache/eventmesh/common/utils/LogUtils.java 5.68% 80 Missing and 3 partials :warning:
...g/apache/eventmesh/common/utils/JsonPathUtils.java 0.00% 54 Missing :warning:
...tor/jdbc/connection/mysql/MysqlJdbcConnection.java 0.00% 43 Missing :warning:
...dingtalk/sink/connector/DingDingSinkConnector.java 65.38% 24 Missing and 3 partials :warning:
...pache/eventmesh/connector/jdbc/CatalogChanges.java 0.00% 25 Missing :warning:
...ava/org/apache/eventmesh/connector/jdbc/Field.java 0.00% 24 Missing :warning:
...ava/org/apache/eventmesh/common/ThreadWrapper.java 59.64% 16 Missing and 7 partials :warning:
...g/apache/eventmesh/connector/jdbc/DataChanges.java 0.00% 22 Missing :warning:
...a/org/apache/eventmesh/connector/jdbc/Payload.java 0.00% 21 Missing :warning:
... and 71 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4637      +/-   ##
============================================
+ Coverage     16.93%   17.40%   +0.47%     
- Complexity     1413     1762     +349     
============================================
  Files           589      797     +208     
  Lines         25786    29798    +4012     
  Branches       2397     2579     +182     
============================================
+ Hits           4367     5187     +820     
- Misses        20983    24130    +3147     
- Partials        436      481      +45     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Apr 06 '24 18:04 codecov-commenter

May I ask that is this PR ready for review?

Pil0tXia avatar Jun 12 '24 18:06 Pil0tXia

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

github-actions[bot] avatar Aug 16 '24 18:08 github-actions[bot]

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

github-actions[bot] avatar Oct 16 '24 18:10 github-actions[bot]

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

github-actions[bot] avatar Dec 16 '24 18:12 github-actions[bot]

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

github-actions[bot] avatar Apr 14 '25 18:04 github-actions[bot]

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

github-actions[bot] avatar Jun 14 '25 18:06 github-actions[bot]