You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/09/01 09:25:30 UTC
[GitHub] [kafka] rk3rn3r opened a new pull request #11287: KAFKA-13256 Fix possible NPE when ConfigDef renders RST or HTML and ConfigKey.documentation is unset/NULL
rk3rn3r opened a new pull request #11287:
URL: https://github.com/apache/kafka/pull/11287
closes https://issues.apache.org/jira/browse/KAFKA-13256
### Committer Checklist (excluded from commit message)
- [x] Verify design and implementation
- [x] Verify test coverage and CI build status
- [x] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rk3rn3r commented on a change in pull request #11287: KAFKA-13256 Fix possible NPE when ConfigDef renders RST or HTML and ConfigKey.documentation is unset/NULL
Posted by GitBox <gi...@apache.org>.
rk3rn3r commented on a change in pull request #11287:
URL: https://github.com/apache/kafka/pull/11287#discussion_r705252513
##########
File path: clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java
##########
@@ -1372,11 +1372,13 @@ public String toEnrichedRst() {
*/
private void getConfigKeyRst(ConfigKey key, StringBuilder b) {
b.append("``").append(key.name).append("``").append("\n");
- for (String docLine : key.documentation.split("\n")) {
- if (docLine.length() == 0) {
- continue;
+ if (null != key.documentation) {
+ for (String docLine : key.documentation.split("\n")) {
+ if (docLine.length() == 0) {
+ continue;
+ }
+ b.append(" ").append(docLine).append("\n\n");
Review comment:
Hello @mimaison! Thanks for your review!
I applied the changes and added some code for it to the test cases.
I also discovered an issue with the RST output by adding the test!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a change in pull request #11287: KAFKA-13256 Fix possible NPE when ConfigDef renders RST or HTML and ConfigKey.documentation is unset/NULL
Posted by GitBox <gi...@apache.org>.
mimaison commented on a change in pull request #11287:
URL: https://github.com/apache/kafka/pull/11287#discussion_r705140109
##########
File path: clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java
##########
@@ -1372,11 +1372,13 @@ public String toEnrichedRst() {
*/
private void getConfigKeyRst(ConfigKey key, StringBuilder b) {
b.append("``").append(key.name).append("``").append("\n");
- for (String docLine : key.documentation.split("\n")) {
- if (docLine.length() == 0) {
- continue;
+ if (null != key.documentation) {
Review comment:
Nit: we typically put `null` on the right hand side in the Kafka codebase. Same below
##########
File path: clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java
##########
@@ -1372,11 +1372,13 @@ public String toEnrichedRst() {
*/
private void getConfigKeyRst(ConfigKey key, StringBuilder b) {
b.append("``").append(key.name).append("``").append("\n");
- for (String docLine : key.documentation.split("\n")) {
- if (docLine.length() == 0) {
- continue;
+ if (null != key.documentation) {
+ for (String docLine : key.documentation.split("\n")) {
+ if (docLine.length() == 0) {
+ continue;
+ }
+ b.append(" ").append(docLine).append("\n\n");
Review comment:
It seems `toHTLM()` is not tested but there's an existing test for `toEnrichedRst()` in `ConfigDefTest`. Can we add a config entry with a `null` documentation there?
What about something like:
```
diff --git clients/src/test/java/org/apache/kafka/common/config/ConfigDefTest.java clients/src/test/java/org/apache/kafka/common/config/ConfigDefTest.java
index f8239e5b7e..5a4655db14 100644
--- clients/src/test/java/org/apache/kafka/common/config/ConfigDefTest.java
+++ clients/src/test/java/org/apache/kafka/common/config/ConfigDefTest.java
@@ -531,9 +531,15 @@ public class ConfigDefTest {
"Group Two", 1, Width.NONE, "..", Collections.<String>emptyList())
.define("opt1.of.group2", Type.BOOLEAN, false, Importance.HIGH, "Doc doc doc doc doc.",
"Group Two", 0, Width.NONE, "..", Collections.singletonList("some.option"))
- .define("poor.opt", Type.STRING, "foo", Importance.HIGH, "Doc doc doc doc.");
+ .define("poor.opt", Type.STRING, "foo", Importance.HIGH, "Doc doc doc doc.")
+ .define("null doc", Type.STRING, "foo", Importance.HIGH, null);
final String expectedRst = "" +
+ "``null doc``\n" +
+ " * Type: string\n" +
+ " * Default: foo\n" +
+ " * Importance: high\n" +
+ "\n" +
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rk3rn3r commented on a change in pull request #11287: KAFKA-13256 Fix possible NPE when ConfigDef renders RST or HTML and ConfigKey.documentation is unset/NULL
Posted by GitBox <gi...@apache.org>.
rk3rn3r commented on a change in pull request #11287:
URL: https://github.com/apache/kafka/pull/11287#discussion_r705251404
##########
File path: clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java
##########
@@ -1372,11 +1372,13 @@ public String toEnrichedRst() {
*/
private void getConfigKeyRst(ConfigKey key, StringBuilder b) {
b.append("``").append(key.name).append("``").append("\n");
- for (String docLine : key.documentation.split("\n")) {
- if (docLine.length() == 0) {
- continue;
+ if (null != key.documentation) {
Review comment:
applied
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #11287: KAFKA-13256 Fix possible NPE when ConfigDef renders RST or HTML and ConfigKey.documentation is unset/NULL
Posted by GitBox <gi...@apache.org>.
mimaison merged pull request #11287:
URL: https://github.com/apache/kafka/pull/11287
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org