Skip to content

Add more null checks to TableIdentifier #2703

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.

Sign up for GitHub

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
rdblue merged 1 commit into from
Jun 17, 2021
Merged

Add more null checks to TableIdentifier #2703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion api/src/main/java/org/apache/iceberg/catalog/TableIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class TableIdentifier {
private final String name;

public static TableIdentifier of(String... names) {
Preconditions.checkArgument(names != null, "Cannot create table identifier from null array");
Copy link
Contributor

@jackye1995 jackye1995 Jun 17, 2021

Choose a reason for hiding this comment

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

I don't think null check is needed for java varargs. The names will be empty array if no input is given. I verified it locally:

private void met(String... strs) {
    System.out.println(Arrays.toString(strs));
  }

met();

The result is []

Copy link
Contributor

@rdblue rdblue Jun 17, 2021

Choose a reason for hiding this comment

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

The problem isn't when this is used as a varargs call. It is that this actually creates of(String[] names) in the class file. So you can call it directly with a String array:

String[] levels = null;
return Namespace.of(levels); // fails

Copy link
Contributor

@jackye1995 jackye1995 Jun 17, 2021

Choose a reason for hiding this comment

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

Ah I see, never thought about this use case, thanks!

Copy link
Contributor Author

@nastra nastra Jun 18, 2021

Choose a reason for hiding this comment

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

there's also a test that verifies this behavior: https://github.com/apache/iceberg/pull/2703/files#diff-7d459f8dfa23098a11664c5f2399f8a8a6c27d72bfcf4384e9bb580fdaff0963R72

Preconditions.checkArgument(names.length > 0, "Cannot create table identifier without a table name");
return new TableIdentifier(Namespace.of(Arrays.copyOf(names, names.length - 1)), names[names.length - 1]);
}
Expand All @@ -45,12 +46,14 @@ public static TableIdentifier of(Namespace namespace, String name) {
}

public static TableIdentifier parse(String identifier) {
Preconditions.checkArgument(identifier != null, "Cannot parse table identifier: null");
Iterable<String> parts = DOT.split(identifier);
return TableIdentifier.of(Iterables.toArray(parts, String.class));
}

private TableIdentifier(Namespace namespace, String name) {
Preconditions.checkArgument(name != null && !name.isEmpty(), "Invalid table name %s", name);
Preconditions.checkArgument(name != null && !name.isEmpty(), "Invalid table name: null or empty");
Preconditions.checkArgument(namespace != null, "Invalid Namespace: null");
this.namespace = namespace;
this.name = name;
}
Expand Down
27 changes: 27 additions & 0 deletions api/src/test/java/org/apache/iceberg/catalog/TestTableIdentifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.apache.iceberg.catalog;

import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -54,4 +55,30 @@ public void testToLowerCase() {
TableIdentifier.of("Catalog", "dB", "TBL").toLowerCase(),
TableIdentifier.of("catalog", "db", "tbl"));
}

@Test
public void testInvalidTableName() {
Assertions.assertThatThrownBy(() -> TableIdentifier.of(Namespace.empty(), ""))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid table name: null or empty");

Assertions.assertThatThrownBy(() -> TableIdentifier.of(Namespace.empty(), null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid table name: null or empty");
}

@Test
public void testNulls() {
Assertions.assertThatThrownBy(() -> TableIdentifier.of((String[]) null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot create table identifier from null array");

Assertions.assertThatThrownBy(() -> TableIdentifier.parse(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse table identifier: null");

Assertions.assertThatThrownBy(() -> TableIdentifier.of(null, "name"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid Namespace: null");
}
}