Skip to content

Enforcing procedure name args only #10545

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
kokosing merged 2 commits into from
Sep 23, 2022
Merged

Enforcing procedure name args only #10545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9da623
Allow enforcing only named arguments for procedure calls
Jan 11, 2022
4c1cc99
Use named arguments only in flush_metadata_cache procedure
Jan 11, 2022
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
3 changes: 3 additions & 0 deletions core/trino-main/src/main/java/io/trino/execution/CallTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ public ListenableFuture<Void> execute(
Predicate<CallArgument> hasName = argument -> argument.getName().isPresent();
boolean anyNamed = call.getArguments().stream().anyMatch(hasName);
boolean allNamed = call.getArguments().stream().allMatch(hasName);
if (!allNamed && procedure.requiresNamedArguments()) {
throw semanticException(INVALID_ARGUMENTS, call, "Only named arguments are allowed for this procedure");
}
if (anyNamed && !allNamed) {
throw semanticException(INVALID_ARGUMENTS, call, "Named and positional arguments cannot be mixed");
}
Expand Down
12 changes: 12 additions & 0 deletions core/trino-spi/src/main/java/io/trino/spi/procedure/Procedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,21 @@ public class Procedure
private final String schema;
private final String name;
private final List<Argument> arguments;
private final boolean requireNamedArguments;
private final MethodHandle methodHandle;

public Procedure(String schema, String name, List<Argument> arguments, MethodHandle methodHandle)
{
this(schema, name, arguments, methodHandle, false);
}

public Procedure(String schema, String name, List<Argument> arguments, MethodHandle methodHandle, boolean requireNamedArguments)
{
this.schema = checkNotNullOrEmpty(schema, "schema").toLowerCase(ENGLISH);
this.name = checkNotNullOrEmpty(name, "name").toLowerCase(ENGLISH);
this.arguments = List.copyOf(requireNonNull(arguments, "arguments is null"));
this.methodHandle = requireNonNull(methodHandle, "methodHandle is null");
this.requireNamedArguments = requireNamedArguments;

Set<String> names = new HashSet<>();
for (Argument argument : arguments) {
Expand Down Expand Up @@ -85,6 +92,11 @@ public MethodHandle getMethodHandle()
return methodHandle;
}

public boolean requiresNamedArguments()
{
return requireNamedArguments;
}

@Override
public String toString()
{
Expand Down
11 changes: 4 additions & 7 deletions .../main/java/io/trino/plugin/hive/metastore/procedure/FlushHiveMetastoreCacheProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,13 @@ public class FlushHiveMetastoreCacheProcedure

static {
try {
FLUSH_HIVE_METASTORE_CACHE = lookup().unreflect(FlushHiveMetastoreCacheProcedure.class.getMethod("flushMetadataCache", String.class, String.class, String.class, List.class, List.class));
FLUSH_HIVE_METASTORE_CACHE = lookup().unreflect(FlushHiveMetastoreCacheProcedure.class.getMethod("flushMetadataCache", String.class, String.class, List.class, List.class));
}
catch (ReflectiveOperationException e) {
throw new AssertionError(e);
}
}

private static final String FAKE_PARAM_DEFAULT_VALUE = "procedure should only be invoked with named parameters";

private final Optional<CachingHiveMetastore> cachingHiveMetastore;

@Inject
Expand All @@ -84,17 +82,16 @@ public Procedure get()
"system",
PROCEDURE_NAME,
ImmutableList.of(
new Procedure.Argument("$FAKE_FIRST_PARAMETER", VARCHAR, false, FAKE_PARAM_DEFAULT_VALUE),
new Procedure.Argument(PARAM_SCHEMA_NAME, VARCHAR, false, null),
new Procedure.Argument(PARAM_TABLE_NAME, VARCHAR, false, null),
new Procedure.Argument(PARAM_PARTITION_COLUMN, new ArrayType(VARCHAR), false, null),
new Procedure.Argument(PARAM_PARTITION_VALUE, new ArrayType(VARCHAR), false, null)),
FLUSH_HIVE_METASTORE_CACHE.bindTo(this));
FLUSH_HIVE_METASTORE_CACHE.bindTo(this),
true);
}

public void flushMetadataCache(String fakeParam, String schemaName, String tableName, List<String> partitionColumn, List<String> partitionValue)
public void flushMetadataCache(String schemaName, String tableName, List<String> partitionColumn, List<String> partitionValue)
{
checkState(FAKE_PARAM_DEFAULT_VALUE.equals(fakeParam), "Procedure should only be invoked with named parameters. " + PROCEDURE_USAGE_EXAMPLES);
Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Sep 22, 2022

Choose a reason for hiding this comment

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

Wow, that was a clever workaround. Glad that it's gone :D

try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(getClass().getClassLoader())) {
doFlushMetadataCache(
Optional.ofNullable(schemaName),
Expand Down
9 changes: 4 additions & 5 deletions ...st/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastoreWithQueryRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,16 +143,15 @@ public void testFlushHiveMetastoreCacheProcedureCallable()
@Test
public void testIllegalFlushHiveMetastoreCacheProcedureCalls()
{
String illegalParameterMessage = "Illegal parameter set passed. ";
String validUsageExample = "Valid usages:\n - 'flush_metadata_cache()'\n - flush_metadata_cache(schema_name => ..., table_name => ..., partition_column => ARRAY['...'], partition_value => ARRAY['...'])";
String illegalParameterMessage = "Illegal parameter set passed. Valid usages:\n - 'flush_metadata_cache()'\n - flush_metadata_cache(schema_name => ..., table_name => ..., partition_column => ARRAY['...'], partition_value => ARRAY['...'])";

assertThatThrownBy(() -> getQueryRunner().execute("CALL system.flush_metadata_cache('dummy_schema')"))
.hasMessage("Procedure should only be invoked with named parameters. " + validUsageExample);
.hasMessageContaining("Only named arguments are allowed for this procedure");

assertThatThrownBy(() -> getQueryRunner().execute("CALL system.flush_metadata_cache(schema_name => 'dummy_schema')"))
.hasMessage(illegalParameterMessage + validUsageExample);
.hasMessage(illegalParameterMessage);
assertThatThrownBy(() -> getQueryRunner().execute("CALL system.flush_metadata_cache(schema_name => 'dummy_schema', table_name => 'dummy_table')"))
.hasMessage(illegalParameterMessage + validUsageExample);
.hasMessage(illegalParameterMessage);

assertThatThrownBy(() -> getQueryRunner().execute("CALL system.flush_metadata_cache(schema_name => 'dummy_schema', table_name => 'dummy_table', partition_column => ARRAY['dummy_partition'])"))
.hasMessage("Parameters partition_column and partition_value should have same length");
Expand Down