smiklosovic commented on code in PR #4038:
URL: https://github.com/apache/cassandra/pull/4038#discussion_r2041584307
##########
src/java/org/apache/cassandra/schema/SchemaConstants.java:
##########
@@ -75,14 +77,54 @@ public final class SchemaConstants
*/
public static final int NAME_LENGTH = 48;
+ /**
+ * Longest acceptable file name. Longer names lead to too long file name
error.
+ */
+ public static final int FILENAME_LENGTH = 255;
+
+ /**
+ * Longest acceptable table name, so it can be used in a directory
+ * name constructed with a suffix of a table id and a separator.
+ */
+ public static final int TABLE_NAME_LENGTH = FILENAME_LENGTH - 32 - 1;
+
// 59adb24e-f3cd-3e02-97f0-5b395827453f
public static final UUID emptyVersion;
public static final List<String> LEGACY_AUTH_TABLES =
Arrays.asList("credentials", "users", "permissions");
+ /**
+ * Validates that a name is valid to be used in files. Assumes that the
name length
+ * should fit the default, {@link #NAME_LENGTH}.
+ * See {@link #isValidName(String, int)} for more details.
+ *
+ * @param name the name to check
+ * @return whether the name is safe for use in file paths and file names
+ */
public static boolean isValidName(String name)
{
- return name != null && !name.isEmpty() && name.length() <= NAME_LENGTH
&& PATTERN_WORD_CHARS.matcher(name).matches();
+ return isValidName(name, NAME_LENGTH);
+ }
+
+ /**
+ * Names such as keyspace, table, index names are used in file paths and
file names,
+ * so, they need to be safe for the use there, i.e., short enough and
+ * containing only alphanumeric characters and underscores.
+ * Allows to provide the length of names, since it varies for different
database objects,
+ * especially, they were not historically controlled for {@link
#NAME_LENGTH}, see,
+ * e.g., CASSANDRA-20389.
+ * There are cases when the length cannot be controlled by a single value.
In such case
+ * the length validation is skipped if the given length is smaller than 1.
+ *
+ * @param name the name to check
+ * @param maxLength max acceptable length for the given name. For the
cases when it cannot
+ * be limited, 0 or negative number should be supplied.
Review Comment:
@k-rus
_It don't see that it gives any advantage._
Because then you do not need to do
`&& (maxLength <= 0 || name.length() <= maxLength);`
but just
`&& (name.length() <= maxLength);`
because `name.length()` will be always < when `maxLength` is
`Integer.MAX_VALUE`
Also, who would ever call that method with `maxLength <= 0`, meaning
`maxLength` being `0`? That does not make sense to call like that.
`Integer.MAX_VALUE` is more "idiomatic" in such a way that we practically do
not care how long it will be, it might be so long, up to `Integer.MAX_VALUE`,
that we effectively do not care.
Asserting that `maxLength > 0` at the beginning of this method is just
enough imho.
What you are doing is that you say "and put there 0 or negative if you don't
care about the length". What is suggested is that you still stay working with
the logic of "how long I want to have it at maximum" and you do not care so you
put there insanely large number.
The latter case just parses better in the head because we are putting here
special numbers, 0, -1 just to model something which is perfectly covered by
`Integer.MAX_VALUE`. With your solution, people need to know that "we do not
care equals to negative or 0". They don't need to know this. Using
Integer.MAX_VALUE completely circumvent this.
I would also move `name.length() <= maxLength` _before_
`PATTERN_WORD_CHARS.matcher(name).matches()`.
It does not make sense to evaluate pattern when length is bigger than
allowed and checking the length of a string is presumably faster than executing
pattern matching.
Also, I think that `!name.isEmpty()` is not enough. What if `name` is `
` - meaning e.g. 5 spaces? Then `name.isEmpty()` is false but ` ` is hardly
a valid file name so you would execute pattern matching unnecessarily. You
should use `!name.isBlank()` instead which is there from Java 11.
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
