calcite-avatica icon indicating copy to clipboard operation
calcite-avatica copied to clipboard

[CALCITE-6590] Use reflection to handle Java SecurityManager deprecation in Avatica

Open stoty opened this issue 1 year ago • 2 comments

Also bump ByteBuddy version to 1.15.1

stoty avatar Sep 26 '24 18:09 stoty

This is the alternate patch using reflection.

stoty avatar Sep 26 '24 18:09 stoty

Have you been able to check this @julianhyde ?

stoty avatar Oct 08 '24 13:10 stoty

The approach here is directly lifted from Jetty (pruning some unused functions)

stoty avatar Jan 16 '25 13:01 stoty

rebased on main

stoty avatar Jan 16 '25 13:01 stoty

I realized my suggestions for SecurityUtils were getting a little involved so I coded up the result and PRed here: https://github.com/stoty/calcite-avatica/pull/1

chrisdennis avatar Jan 24 '25 19:01 chrisdennis

I have applied your PR, and made some minor changes. These are in different commits. Please check them @chrisdennis .

stoty avatar Jan 25 '25 10:01 stoty

I have changed the name back to sneakyThrow().

Since we're going for perfection, we may also want to reconsider callAs().

Subject callAs() throws Exceptions, and the fallback path now also throws exceptions (wrapped in PrivilegedActionException).

We could change it to something like :

  public static <T> T callAs(Subject subject, Callable<T> action) throws Exception {
    try {
      return (T) CALL_AS.invoke(subject, action);
    } catch (PrivilegedActionException e) {
      e.getException();
    } catch (Exception e) {
      throw e;
    } catch (Throwable t) {
      throw sneakyThrow(t);
    }
  }

stoty avatar Jan 27 '25 15:01 stoty

Since we're going for perfection, we may also want to reconsider callAs().

Apologies for triggering some (all?) of this bike-shedding.

Subject callAs() throws Exceptions, and the fallback path now also throws exceptions (wrapped in PrivilegedActionException).

I'm confused, Subject.callAs(...) throws CompletionException if the action throws, which is what the current code does, unwrapping any PrivilegedActionException and rewrapping it in case we're backed by the old doAs(...) method. The only tweak I'd make to the current code is that the Exception catch and rethrow is redundant given the sneakyThrow(t) call.

chrisdennis avatar Jan 27 '25 15:01 chrisdennis

I'm confused, Subject.callAs(...) throws CompletionException if the action throws, which is what the current code does, unwrapping any PrivilegedActionException and rewrapping it in case we're backed by the old doAs(...) method. The only tweak I'd make to the current code is that the Exception catch and rethrow is redundant given the sneakyThrow(t) call.

You are right.

However, since Subject.callAs() explicitly throws CompletionException, shouldn't SecurityUtils.callAs() also explictly throw CompletionException ?

stoty avatar Jan 27 '25 18:01 stoty

However, since Subject.callAs() explicitly throws CompletionException, shouldn't SecurityUtils.callAs() also explictly throw CompletionException ?

Yes, that would make sense.

chrisdennis avatar Jan 27 '25 18:01 chrisdennis

This is as polished as it gets now. Can you do a final review @zabetak ?

stoty avatar Jan 28 '25 05:01 stoty

Merged manually. (Maybe we could enable squash merges...)

stoty avatar Jan 28 '25 11:01 stoty

Thanks for getting this in @stoty, @chrisdennis and @zabetak!

F21 avatar Jan 28 '25 11:01 F21