You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org> on 2023/03/31 09:08:29 UTC

[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #14004: Errors take 3

github-code-scanning[bot] commented on code in PR #14004:
URL: https://github.com/apache/druid/pull/14004#discussion_r1154227559


##########
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java:
##########
@@ -249,60 +254,66 @@
   protected AbstractAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
   {
     return new DruidAvaticaJsonHandler(
-            druidMeta,
-            new DruidNode("dummy", "dummy", false, 1, null, true, false),
-            new AvaticaMonitor()
+        druidMeta,
+        new DruidNode("dummy", "dummy", false, 1, null, true, false),
+        new AvaticaMonitor()
     );
   }
 
   @Before
   public void setUp() throws Exception
   {
-    walker = CalciteTests.createMockWalker(conglomerate, temporaryFolder.newFolder());
     final DruidSchemaCatalog rootSchema = makeRootSchema();
     testRequestLogger = new TestRequestLogger();
 
     injector = new CoreInjectorBuilder(new StartupInjectorBuilder().build())
-        .addModule(binder -> {
-            binder.bindConstant().annotatedWith(Names.named("serviceName")).to("test");
-            binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0);
-            binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1);
-            binder.bind(AuthenticatorMapper.class).toInstance(CalciteTests.TEST_AUTHENTICATOR_MAPPER);
-            binder.bind(AuthorizerMapper.class).toInstance(CalciteTests.TEST_AUTHORIZER_MAPPER);
-            binder.bind(Escalator.class).toInstance(CalciteTests.TEST_AUTHENTICATOR_ESCALATOR);
-            binder.bind(RequestLogger.class).toInstance(testRequestLogger);
-            binder.bind(DruidSchemaCatalog.class).toInstance(rootSchema);
-            for (NamedSchema schema : rootSchema.getNamedSchemas().values()) {
-              Multibinder.newSetBinder(binder, NamedSchema.class).addBinding().toInstance(schema);
+        .addModule(
+            binder -> {
+              binder.bindConstant().annotatedWith(Names.named("serviceName")).to("test");
+              binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0);
+              binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1);
+              binder.bind(AuthenticatorMapper.class).toInstance(CalciteTests.TEST_AUTHENTICATOR_MAPPER);
+              binder.bind(AuthorizerMapper.class).toInstance(CalciteTests.TEST_AUTHORIZER_MAPPER);
+              binder.bind(Escalator.class).toInstance(CalciteTests.TEST_AUTHENTICATOR_ESCALATOR);
+              binder.bind(RequestLogger.class).toInstance(testRequestLogger);
+              binder.bind(DruidSchemaCatalog.class).toInstance(rootSchema);
+              for (NamedSchema schema : rootSchema.getNamedSchemas().values()) {
+                Multibinder.newSetBinder(binder, NamedSchema.class).addBinding().toInstance(schema);
+              }
+              binder.bind(QueryLifecycleFactory.class)
+                    .toInstance(CalciteTests.createMockQueryLifecycleFactory(walker, conglomerate));
+              binder.bind(DruidOperatorTable.class).toInstance(operatorTable);
+              binder.bind(ExprMacroTable.class).toInstance(macroTable);
+              binder.bind(PlannerConfig.class).toInstance(plannerConfig);
+              binder.bind(String.class)
+                    .annotatedWith(DruidSchemaName.class)
+                    .toInstance(CalciteTests.DRUID_SCHEMA_NAME);
+              binder.bind(AvaticaServerConfig.class).toInstance(AVATICA_CONFIG);
+              binder.bind(ServiceEmitter.class).to(NoopServiceEmitter.class);
+              binder.bind(QuerySchedulerProvider.class).in(LazySingleton.class);
+              binder.bind(QueryScheduler.class)
+                    .toProvider(QuerySchedulerProvider.class)
+                    .in(LazySingleton.class);
+              binder.install(new SqlModule.SqlStatementFactoryModule());
+              binder.bind(new TypeLiteral<Supplier<DefaultQueryConfig>>()
+              {
+              }).toInstance(Suppliers.ofInstance(new DefaultQueryConfig(ImmutableMap.of())));
+              binder.bind(CalciteRulesManager.class).toInstance(new CalciteRulesManager(ImmutableSet.of()));
+              binder.bind(JoinableFactoryWrapper.class).toInstance(CalciteTests.createJoinableFactoryWrapper());
+              binder.bind(CatalogResolver.class).toInstance(CatalogResolver.NULL_RESOLVER);
             }
-            binder.bind(QueryLifecycleFactory.class)
-                  .toInstance(CalciteTests.createMockQueryLifecycleFactory(walker, conglomerate));
-            binder.bind(DruidOperatorTable.class).toInstance(operatorTable);
-            binder.bind(ExprMacroTable.class).toInstance(macroTable);
-            binder.bind(PlannerConfig.class).toInstance(plannerConfig);
-            binder.bind(String.class)
-                  .annotatedWith(DruidSchemaName.class)
-                  .toInstance(CalciteTests.DRUID_SCHEMA_NAME);
-            binder.bind(AvaticaServerConfig.class).toInstance(AVATICA_CONFIG);
-            binder.bind(ServiceEmitter.class).to(NoopServiceEmitter.class);
-            binder.bind(QuerySchedulerProvider.class).in(LazySingleton.class);
-            binder.bind(QueryScheduler.class)
-                  .toProvider(QuerySchedulerProvider.class)
-                  .in(LazySingleton.class);
-            binder.install(new SqlModule.SqlStatementFactoryModule());
-            binder.bind(new TypeLiteral<Supplier<DefaultQueryConfig>>(){}).toInstance(Suppliers.ofInstance(new DefaultQueryConfig(ImmutableMap.of())));
-            binder.bind(CalciteRulesManager.class).toInstance(new CalciteRulesManager(ImmutableSet.of()));
-            binder.bind(JoinableFactoryWrapper.class).toInstance(CalciteTests.createJoinableFactoryWrapper());
-            binder.bind(CatalogResolver.class).toInstance(CatalogResolver.NULL_RESOLVER);
-          }
-         )
+        )
         .build();
 
     DruidMeta druidMeta = injector.getInstance(DruidMeta.class);
     server = new ServerWrapper(druidMeta);
     client = server.getUserConnection();
     superuserClient = server.getConnection(CalciteTests.TEST_SUPERUSER_NAME, "druid");
-    clientNoTrailingSlash = DriverManager.getConnection(StringUtils.maybeRemoveTrailingSlash(server.url), CalciteTests.TEST_SUPERUSER_NAME, "druid");
+    clientNoTrailingSlash = DriverManager.getConnection(
+        StringUtils.maybeRemoveTrailingSlash(server.url),
+        CalciteTests.TEST_SUPERUSER_NAME,
+        "druid"

Review Comment:
   ## Hard-coded credential in API call
   
   Hard-coded value flows to [sensitive API call](1).
   
   [Show more details](https://github.com/apache/druid/security/code-scanning/4562)



##########
server/src/main/java/org/apache/druid/server/QueryResultPusher.java:
##########
@@ -197,58 +208,47 @@
   @Nullable
   private Response handleQueryException(ResultsWriter resultsWriter, QueryException e)
   {
-    if (accumulator != null && accumulator.isInitialized()) {
-      // We already started sending a response when we got the error message.  In this case we just give up
-      // and hope that the partial stream generates a meaningful failure message for our client.  We could consider
-      // also throwing the exception body into the response to make it easier for the client to choke if it manages
-      // to parse a meaningful object out, but that's potentially an API change so we leave that as an exercise for
-      // the future.
+    return handleDruidException(resultsWriter, DruidException.fromFailure(new QueryExceptionCompat(e)));
+  }
 
+  private Response handleDruidException(ResultsWriter resultsWriter, DruidException e)
+  {
+    if (resultsWriter != null) {
       resultsWriter.recordFailure(e);
-
-      // This case is always a failure because the error happened mid-stream of sending results back.  Therefore,
-      // we do not believe that the response stream was actually usable
       counter.incrementFailed();
-      return null;
+
+      if (accumulator != null && accumulator.isInitialized()) {
+        // We already started sending a response when we got the error message.  In this case we just give up
+        // and hope that the partial stream generates a meaningful failure message for our client.  We could consider
+        // also throwing the exception body into the response to make it easier for the client to choke if it manages
+        // to parse a meaningful object out, but that's potentially an API change so we leave that as an exercise for
+        // the future.
+        return null;
+      }
     }
 
-    final QueryException.FailType failType = e.getFailType();
-    switch (failType) {
-      case USER_ERROR:
+    switch (e.getCategory()) {

Review Comment:
   ## Missing enum case in switch
   
   Switch statement does not have a case for [DEFENSIVE](1).
   
   [Show more details](https://github.com/apache/druid/security/code-scanning/4508)



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org