k-rus commented on code in PR #4038:
URL: https://github.com/apache/cassandra/pull/4038#discussion_r2061246845
##########
src/java/org/apache/cassandra/schema/TableMetadata.java:
##########
@@ -602,11 +606,16 @@ public boolean dependsOn(Function function)
public void validate()
{
- if (!isNameValid(keyspace))
- except("Keyspace name must not be empty, more than %s characters
long, or contain non-alphanumeric-underscore characters (got \"%s\")",
SchemaConstants.NAME_LENGTH, keyspace);
+ if (!isValidKeyspaceName(keyspace))
+ except("Keyspace name must not be empty, more than %s characters
long, or contain non-alphanumeric-underscore characters (got \"%s\")",
NAME_LENGTH, keyspace);
Review Comment:
> what I would prefer to do is to move "except" (its guts) into
"isValidKeyspaceName". If we always throw when we check, why not to move
throwing to that method?
@smiklosovic
Thank you for the suggestion. I investigated to throw from inside
`isValidKeyspaceName`, however, the callers throw different exception classes:
`ConfigurationException` and `InvalidRequestException`. One solution can be to
pass the exception class, but it doesn't look to be simple in Java 11: requires
to add a supplier interface. Let me know if you know a better solution.
>SchemaConstants should contain this whole helper logic. We would just call
it from there here. We would make TableMetadata easier to read.
At this moment I feel strongly to keep the methods in `KeyspaceMetadata` and
`TableMetadata` and not move into `SchemaConstants`:
- To me the logic is specific, e.g., table name validation is not relevant
for keyspace or index name validations, while `SchemaConstants` is common
utility to me. In such case, may be `TABLE_NAME_LENGTH` should be moved out
from `SchemaConstants`.
- The version of this patch for 4.x will contain more complex logic for
validating table name length.
- Index name length validation will be fixed by CASSANDRA-20445 and it will
contain complex logic.
--
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
