This is an automated email from the ASF dual-hosted git repository.
abhishekrb pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new e4cdbca23cb make planner errors be user persona (#17437)
e4cdbca23cb is described below
commit e4cdbca23cb9b008a69bfd2831ee2d749e8a2757
Author: Tom <[email protected]>
AuthorDate: Mon Nov 4 10:48:35 2024 -0800
make planner errors be user persona (#17437)
Change the persona for errors within the planner from Admin to User. The
ADMIN persona is meant to be "a persona who is interacting with admin APIs and
understands Druid query concepts". This isn't an admin API, it's a query API.
Low quality error messages being returned to the correct audience is better
than hiding all error messages.
The errors that can be returned back can be user solvable, and other times
requires a druid expert. But the errors do not leak information that should
only be seen by more expert/privileged personas.
The original ADMIN persona showed some reticence to tag low-quality error
messages with a USER persona. but it really does seem user-directed to me so
USER to me would make sense.
---
.../src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java | 4 ++--
.../java/org/apache/druid/sql/calcite/planner/QueryHandler.java | 7 ++++---
.../java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java | 2 +-
sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java | 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git
a/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java
b/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java
index b18e14b04f2..2918391c0be 100644
---
a/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java
+++
b/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java
@@ -511,7 +511,7 @@ public class MSQFaultsTest extends MSQTestBase
+ "PARTITIONED BY ALL TIME")
.setExpectedValidationErrorMatcher(
new DruidExceptionMatcher(
- DruidException.Persona.ADMIN,
+ DruidException.Persona.USER,
DruidException.Category.INVALID_INPUT,
"general"
).expectMessageContains(
@@ -533,7 +533,7 @@ public class MSQFaultsTest extends MSQTestBase
)
.setExpectedValidationErrorMatcher(
new DruidExceptionMatcher(
- DruidException.Persona.ADMIN,
+ DruidException.Persona.USER,
DruidException.Category.INVALID_INPUT,
"general"
).expectMessageContains(
diff --git
a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java
b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java
index b19e83e040e..2fca51e8dc0 100644
--- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java
+++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java
@@ -678,9 +678,10 @@ public abstract class QueryHandler extends
SqlStatementHandler.BaseStatementHand
.build(exception, "Unhandled Query Planning Failure,
see broker logs for details");
} else {
// Planning errors are more like hints: it isn't guaranteed that the
planning error is actually what went wrong.
- // For this reason, we consider these as targetting a more expert
persona, i.e. the admin instead of the actual
- // user.
- throw DruidException.forPersona(DruidException.Persona.ADMIN)
+ // Even though the errors could be targetted to a more expert persona
the errors aren't leaking any privileged
+ // information about the cluster that an admin might care about. The
errors that are user resolvable are worth
+ // the potential confusion that a user might face with one that requires
an expert persona.
+ throw DruidException.forPersona(DruidException.Persona.USER)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build(
exception,
diff --git
a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
index 94bc3b5c2eb..9c74d4bbffc 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
+++ b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java
@@ -690,7 +690,7 @@ public class BaseCalciteQueryTest extends CalciteTestBase
if (testBuilder().isDecoupledMode()) {
return new DruidExceptionMatcher(Persona.USER, Category.INVALID_INPUT,
"invalidInput");
} else {
- return new DruidExceptionMatcher(Persona.ADMIN, Category.INVALID_INPUT,
"general");
+ return new DruidExceptionMatcher(Persona.USER, Category.INVALID_INPUT,
"general");
}
}
diff --git a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java
b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java
index 665d5873b46..b906fe73178 100644
--- a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java
+++ b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java
@@ -1386,7 +1386,7 @@ public class SqlResourceTest extends CalciteTestBase
validateErrorResponse(
exception,
"general",
- DruidException.Persona.ADMIN,
+ DruidException.Persona.USER,
DruidException.Category.INVALID_INPUT,
"Query could not be planned. A possible reason is "
+ "[SQL query requires ordering a table by non-time column [[dim1]],
which is not supported.]"
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
