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

[Fix-16573][alert] SQL task alert sending failed #16595

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

saowu
Copy link
Contributor

@saowu saowu commented Sep 7, 2024

Purpose of the pull request

fix #16573

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@@ -118,7 +119,7 @@ public static String formatContent(AlertData alertData) {
for (Map map : list) {
for (Entry<String, Object> entry : (Iterable<Entry<String, Object>>) map.entrySet()) {
String key = entry.getKey();
String value = entry.getValue().toString();
String value = Objects.nonNull(entry.getValue()) ? entry.getValue().toString() : null;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think entry.getValue() will get npe. Can you provide the reproduce steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
The json field value is null.

Copy link
Member

Choose a reason for hiding this comment

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

Where did this parameter other comes from? I can't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This parameter is used to verify npe and does not exist.
#16573
The sql query results are sent via Feishu, and the field value in the query results is null

Copy link
Member

Choose a reason for hiding this comment

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

I see. We should do the data convertion in org.apache.dolphinscheduler.plugin.task.sql.SqlTask. This ensures that other alarm types are normal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Is the field value in the result processed here empty?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Is the second one better?

@SbloodyS SbloodyS added this to the 3.3.0 milestone Sep 7, 2024
@SbloodyS SbloodyS changed the title [Fix-16573][alert] The json field value is null [Fix-16573][alert] SQL task alert sending failed Sep 9, 2024
@github-actions github-actions bot removed the test label Sep 10, 2024
Copy link

sonarcloud bot commented Sep 11, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

Comment on lines +258 to +266
resultJSONArray.forEach(row -> {
row.fields().forEachRemaining(field -> {
if (field.getValue() instanceof NullNode) {
field.setValue(new TextNode("null"));
}
});
});
String sendResult = resultJSONArray.isEmpty() ? JSONUtils.toJsonString(generateEmptyRow(resultSet))
: JSONUtils.toJsonString(resultJSONArray);
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 some unit-test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to write a unit test for this piece.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started all the projects and tested them with simulated data.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how to write a unit test for this piece.

You can take a look at other UT.

I started all the projects and tested them with simulated data.

Testing is used to ensure the normal operation in the future, and the current normal operation cannot guarantee that the future will not be affected by other factors.

@SbloodyS SbloodyS added the bug Something isn't working label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [alert] SQL task query result sends null exception
3 participants