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

[Feature][Connector-V2][HTTP] Use json-path parsing #3510

Merged
merged 22 commits into from
Nov 30, 2022

Conversation

liugddx
Copy link
Member

@liugddx liugddx commented Nov 22, 2022

close #3500

Purpose of this pull request

Check list

@liugddx liugddx marked this pull request as draft November 22, 2022 14:29
@liugddx liugddx closed this Nov 23, 2022
@liugddx liugddx reopened this Nov 23, 2022
@liugddx liugddx marked this pull request as ready for review November 27, 2022 12:47
@liugddx
Copy link
Member Author

liugddx commented Nov 27, 2022

@hailin0 @Hisoka-X PTAL


### new version

- Increase jsonpath parsing.
Copy link
Member

Choose a reason for hiding this comment

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

Add PR link.

Copy link
Member

@EricJoy2048 EricJoy2048 Nov 28, 2022

Choose a reason for hiding this comment

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

Please refer to the format of other changed logs. [Feature/Bugfix/Improve][XXX]

@@ -124,6 +125,18 @@ connector will generate data as the following:

the schema fields of upstream data

### json_field [Config]

The Wildcards for jsonpath like
Copy link
Member

Choose a reason for hiding this comment

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

Add example json, and how to use json_field with example json, like parse data in list. And if we have json_field, do we need config schema or not? The answer of these question should be found in document.

@Hisoka-X
Copy link
Member

By the way, this is a good feature.

@EricJoy2048
Copy link
Member

We have an issue discuss about this #3573. Can we wait for the discussion to finish before processing this pr?

}
}
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Please add real response json example in here.

# Conflicts:
#	seatunnel-connectors-v2/connector-http/connector-http-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/http/config/HttpConfig.java
#	seatunnel-connectors-v2/connector-http/connector-http-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/http/source/HttpSource.java
@TyrantLucifer TyrantLucifer added the don't merge There needs to be a specific reason in the PR, and it cannot be merged for the time being. label Nov 30, 2022
@liugddx
Copy link
Member Author

liugddx commented Nov 30, 2022

Please pending this pull request. Waiting for #3573 finished and decied this pull request is or not merged. cc @Hisoka-X @EricJoy2048 @liugddx

Hi, we have communicated about this issue. If you have other good suggestions, you can also put forward them. @Hisoka-X @EricJoy2048 @TaoZex

@liugddx
Copy link
Member Author

liugddx commented Nov 30, 2022

Please pending this pull request. Waiting for #3573 finished and decied this pull request is or not merged. cc @Hisoka-X @EricJoy2048 @liugddx

Hi, we have communicated about this issue. If you have other good suggestions, you can also put forward them. @Hisoka-X @EricJoy2048 @TaoZex

@TyrantLucifer

@TyrantLucifer TyrantLucifer removed the don't merge There needs to be a specific reason in the PR, and it cannot be merged for the time being. label Nov 30, 2022
List<?> result0 = results.get(0);
List<?> result = results.get(i);
if (result0.size() != result.size()) {
throw new SeaTunnelException(
Copy link
Member

Choose a reason for hiding this comment

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

Use HttpConnectorException

private void initJsonPath(JsonField jsonField) {
jsonPaths = new JsonPath[jsonField.getFields().size()];
final int[] index = {0};
jsonField.getFields().forEach((key, value) -> {
Copy link
Member

Choose a reason for hiding this comment

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

Use entrySet is better, final int[] index = {0}; make the code not clear

EricJoy2048
EricJoy2048 previously approved these changes Nov 30, 2022
Copy link
Member

@EricJoy2048 EricJoy2048 left a comment

Choose a reason for hiding this comment

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

This is a very good feature. Thanks for you contribution.

Comment on lines 20 to +25
| name | type | required | default value |
| --------------------------- | ------ | -------- | ------------- |
| url | String | Yes | - |
| schema | Config | No | - |
| schema.fields | Config | No | - |
| json_field | Config | No | - |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add content_json

Copy link
Member Author

Choose a reason for hiding this comment

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

Please add content_json

Thanks for your code review.

add content_json
Comment on lines +165 to +170
List<List<String>> datas = new ArrayList<>();
for (int i = 0; i < results.size(); i++) {
List<String> result = results.get(i);
if (i == 0) {
for (Object o : result) {
String val = o == null ? null : o.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

The code on line 170 is probably not needed because result is of type List<String>.

Comment on lines +177 to +178
Object o = result.get(j);
String val = o == null ? null : o.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes there is null data, which needs to be converted to a null string.It is necessary to do so

Copy link
Contributor

@TaoZex TaoZex left a comment

Choose a reason for hiding this comment

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

+1

@Hisoka-X Hisoka-X merged commit 1807eb6 into apache:dev Nov 30, 2022
@liugddx liugddx deleted the json-path-parsing branch December 1, 2022 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][Connector-V2][HTTP] Use json-path parsing
5 participants