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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pmupkin
Copy link
Contributor

@pmupkin pmupkin commented Dec 11, 2023

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.

@xwm1992
Copy link
Contributor

xwm1992 commented Dec 12, 2023

@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.

@pandaapo
Copy link
Member

public JsonPathParser(String jsonPathString) {
JsonNode jsonObject = JsonPathUtils.parseStrict(jsonPathString);
Iterator<Map.Entry<String, JsonNode>> fields = jsonObject.fields();
while (fields.hasNext()) {
Map.Entry<String, JsonNode> entry = fields.next();
String name = entry.getKey();
JsonNode valueNode = entry.getValue();
if (!valueNode.isValueNode()) {
throw new TransformException("invalid config: " + jsonPathString);
}
String valueText = valueNode.asText();
if (valueText.startsWith("$") && !valueText.startsWith("$.data")) {
throw new TransformException("invalid config: unsupported value " + jsonPathString);
}
variablesList.add(new Variable(name, valueText));

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.

@pmupkin
Copy link
Contributor Author

pmupkin commented Dec 12, 2023

public JsonPathParser(String jsonPathString) {
JsonNode jsonObject = JsonPathUtils.parseStrict(jsonPathString);
Iterator<Map.Entry<String, JsonNode>> fields = jsonObject.fields();
while (fields.hasNext()) {
Map.Entry<String, JsonNode> entry = fields.next();
String name = entry.getKey();
JsonNode valueNode = entry.getValue();
if (!valueNode.isValueNode()) {
throw new TransformException("invalid config: " + jsonPathString);
}
String valueText = valueNode.asText();
if (valueText.startsWith("$") && !valueText.startsWith("$.data")) {
throw new TransformException("invalid config: unsupported value " + jsonPathString);
}
variablesList.add(new Variable(name, valueText));

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.

@pandaapo
Copy link
Member

pandaapo commented Dec 12, 2023

@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路径,但这恰恰是它需要的。
我不知道我有没有哪里理解错了,期待您的回复。

@pmupkin
Copy link
Contributor Author

pmupkin commented Dec 12, 2023

@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.

@pandaapo
Copy link
Member

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

public List<Variable> match(String json) throws JsonProcessingException {
if (json == null || json.isEmpty()) {
return new ArrayList<>();
}
List<Variable> variableList = new ArrayList<>(variablesList.size());
for (Variable element : variablesList) {
if (JsonPathUtils.isValidAndDefinite(element.getValue())) {
String res = JsonPathUtils.matchJsonPathValueWithString(json, element.getValue());
Variable variable = new Variable(element.getName(), res);

@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方法失效了?

@pmupkin
Copy link
Contributor Author

pmupkin commented Dec 12, 2023

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

public List<Variable> match(String json) throws JsonProcessingException {
if (json == null || json.isEmpty()) {
return new ArrayList<>();
}
List<Variable> variableList = new ArrayList<>(variablesList.size());
for (Variable element : variablesList) {
if (JsonPathUtils.isValidAndDefinite(element.getValue())) {
String res = JsonPathUtils.matchJsonPathValueWithString(json, element.getValue());
Variable variable = new Variable(element.getName(), res);

@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?

@pandaapo
Copy link
Member

pandaapo commented Dec 12, 2023

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?

@pmupkin
Copy link
Contributor Author

pmupkin commented Dec 12, 2023

$.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.

@pandaapo
Copy link
Member

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 #4637 (comment).

您能否提供一个或多个完整的Transformer规则编写示例?然后我根据它们,再学习一遍您的PR #4365中有关Transformer的代码,以及PR #4622的代码。谢谢!
另外,请您注意一下上面第一条review意见#4637 (comment)

@pmupkin pmupkin changed the title Add condition for transforming non-data values [ISSUE #4646] Add condition for transforming non-data values Dec 14, 2023
Copy link

codecov bot commented Dec 18, 2023

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 ⚠️
...nnector/lark/sink/connector/LarkSinkConnector.java 38.46% 8 Missing ⚠️
...g/apache/eventmesh/transformer/JsonPathParser.java 66.66% 1 Missing and 1 partial ⚠️
...onnector/lark/sink/config/SinkConnectorConfig.java 80.00% 1 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Apr 6, 2024

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 github-actions bot added the Stale label Apr 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 61.70213% with 36 lines in your changes missing coverage. Please review.

Project coverage is 17.40%. Comparing base (24873dd) to head (f9cd804).
Report is 181 commits behind head on master.

Files Patch % Lines
...ventmesh/connector/lark/sink/ImServiceHandler.java 64.28% 24 Missing and 1 partial ⚠️
...nnector/lark/sink/connector/LarkSinkConnector.java 38.46% 8 Missing ⚠️
...g/apache/eventmesh/transformer/JsonPathParser.java 66.66% 1 Missing and 1 partial ⚠️
...onnector/lark/sink/config/SinkConnectorConfig.java 80.00% 1 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot removed the Stale label Apr 7, 2024
@pmupkin pmupkin closed this Apr 29, 2024
@pmupkin pmupkin deleted the f--t branch April 29, 2024 11:59
@pmupkin pmupkin restored the f--t branch June 9, 2024 08:08
@pmupkin pmupkin reopened this Jun 9, 2024
@Pil0tXia
Copy link
Member

May I ask that is this PR ready for review?

@Pil0tXia Pil0tXia added the waiting for contributor PR is awaiting the contributor's response to the review for further evaluation. label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for contributor PR is awaiting the contributor's response to the review for further evaluation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Adding a constraint in the transformer.
5 participants