Skip to content

KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… #9851

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

Merged
merged 12 commits into from
Apr 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions clients/src/main/java/org/apache/kafka/common/internals/Topic.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import java.util.Collections;
import java.util.Set;
import java.util.function.Consumer;

public class Topic {

Expand All @@ -34,15 +35,21 @@ public class Topic {
private static final int MAX_NAME_LENGTH = 249;

public static void validate(String topic) {
if (topic.isEmpty())
throw new InvalidTopicException("Topic name is illegal, it can't be empty");
if (topic.equals(".") || topic.equals(".."))
throw new InvalidTopicException("Topic name cannot be \".\" or \"..\"");
if (topic.length() > MAX_NAME_LENGTH)
throw new InvalidTopicException("Topic name is illegal, it can't be longer than " + MAX_NAME_LENGTH +
" characters, topic name: " + topic);
if (!containsValidPattern(topic))
throw new InvalidTopicException("Topic name \"" + topic + "\" is illegal, it contains a character other than " +
validate(topic, "Topic name", message -> {
throw new InvalidTopicException(message);
});
}

public static void validate(String name, String logPrefix, Consumer<String> throwableConsumer) {
if (name.isEmpty())
throwableConsumer.accept(logPrefix + " is illegal, it can't be empty");
if (".".equals(name) || "..".equals(name))
throwableConsumer.accept(logPrefix + " cannot be \".\" or \"..\"");
if (name.length() > MAX_NAME_LENGTH)
throwableConsumer.accept(logPrefix + " is illegal, it can't be longer than " + MAX_NAME_LENGTH +
" characters, " + logPrefix + ": " + name);
if (!containsValidPattern(name))
throwableConsumer.accept(logPrefix + " \"" + name + "\" is illegal, it contains a character other than " +
"ASCII alphanumerics, '.', '_' and '-'");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import org.apache.kafka.common.errors.InvalidConfigurationException;
import org.apache.kafka.common.errors.UnsupportedVersionException;
import org.apache.kafka.common.internals.Topic;
import org.apache.kafka.common.message.JoinGroupRequestData;
import org.apache.kafka.common.message.JoinGroupResponseData;
import org.apache.kafka.common.protocol.ApiKeys;
Expand Down Expand Up @@ -59,38 +60,14 @@ public String toString() {
public static final int UNKNOWN_GENERATION_ID = -1;
public static final String UNKNOWN_PROTOCOL_NAME = "";

private static final int MAX_GROUP_INSTANCE_ID_LENGTH = 249;

/**
* Ported from class Topic in {@link org.apache.kafka.common.internals} to restrict the charset for
* static member id.
*/
public static void validateGroupInstanceId(String id) {
if (id.equals(""))
throw new InvalidConfigurationException("Group instance id must be non-empty string");
Copy link
Member

Choose a reason for hiding this comment

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

The exception is changed from InvalidConfigurationException to InvalidTopicException. It results in different Errors so I feel it would be better to keep throwing InvalidConfigurationException for this check

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 modified code thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @chia7712
thanks

Copy link
Member

Choose a reason for hiding this comment

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

the following check have similar issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chia7712
Would it be okay to explain a little more about what follow check means?

Copy link
Member

Choose a reason for hiding this comment

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

sorry for unclear comment :(

My point was error type thrown by following checks was changed also.

Copy link
Contributor Author

@highluck highluck Mar 5, 2021

Choose a reason for hiding this comment

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

@chia7712
thanks
I modified code!
I think everything will be fine if I modify it like this. How about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @chia7712
thanks!

if (id.equals(".") || id.equals(".."))
throw new InvalidConfigurationException("Group instance id cannot be \".\" or \"..\"");
if (id.length() > MAX_GROUP_INSTANCE_ID_LENGTH)
throw new InvalidConfigurationException("Group instance id can't be longer than " + MAX_GROUP_INSTANCE_ID_LENGTH +
" characters: " + id);
if (!containsValidPattern(id))
throw new InvalidConfigurationException("Group instance id \"" + id + "\" is illegal, it contains a character other than " +
"ASCII alphanumerics, '.', '_' and '-'");
}

/**
* Valid characters for Consumer group.instance.id are the ASCII alphanumerics, '.', '_', and '-'
*/
static boolean containsValidPattern(String topic) {
for (int i = 0; i < topic.length(); ++i) {
char c = topic.charAt(i);

boolean validChar = (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || c == '.' ||
c == '_' || c == '-';
if (!validChar)
return false;
}
return true;
Topic.validate(id, "Group instance id", message -> {
throw new InvalidConfigurationException(message);
});
}

public JoinGroupRequest(JoinGroupRequestData data, short version) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.util.Arrays;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.fail;

Expand Down Expand Up @@ -58,17 +57,6 @@ public void shouldThrowOnInvalidGroupInstanceIds() {
}
}
}

@Test
public void shouldRecognizeInvalidCharactersInGroupInstanceIds() {
char[] invalidChars = {'/', '\\', ',', '\u0000', ':', '"', '\'', ';', '*', '?', ' ', '\t', '\r', '\n', '='};

for (char c : invalidChars) {
String instanceId = "Is " + c + "illegal";
assertFalse(JoinGroupRequest.containsValidPattern(instanceId));
}
}

@Test
public void testRequestVersionCompatibilityFailBuild() {
assertThrows(UnsupportedVersionException.class, () -> new JoinGroupRequest.Builder(
Expand Down