[Fix-16573][alert] SQL task alert sending failed #16595
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
I don't think entry.getValue() will get npe. Can you provide the reproduce steps?
There was a problem hiding this comment.
The json field value is null.
There was a problem hiding this comment.
Where did this parameter other comes from? I can't find it.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I see. We should do the data convertion in org.apache.dolphinscheduler.plugin.task.sql.SqlTask. This ensures that other alarm types are normal.
There was a problem hiding this comment.
Is the field value in the result processed here empty?
There was a problem hiding this comment.
Is the second one better?
SbloodyS
changed the title
|
Please retry analysis of this Pull-Request directly on SonarCloud |
| 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); |
There was a problem hiding this comment.
Please add some unit-test for this.
There was a problem hiding this comment.
I don't know how to write a unit test for this piece.
There was a problem hiding this comment.
I started all the projects and tested them with simulated data.
There was a problem hiding this comment.
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.
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