Skip to content

Invoke toJson forcibly even if DataSourceImpl is not public #26

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Invoke toJson forcibly even if DataSourceImpl is not public #26

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 57 additions & 14 deletions src/main/java/org/embulk/util/config/Compat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,23 @@ private static Optional<String> callToJsonIfAvailable(final DataSource source) {
return Optional.empty();
}

final Object jsonStringObject;
final Object jsonStringObject = invokeToJson(source, toJson);

if (jsonStringObject == null) {
throw new NullPointerException("org.embulk.config.DataSource#toJson() returned null.");
}
if (!(jsonStringObject instanceof String)) {
throw new ClassCastException(
"org.embulk.config.DataSource#toJson() returned not a String: "
+ jsonStringObject.getClass().getCanonicalName());
}

return Optional.of((String) jsonStringObject);
}

private static Object invokeToJson(final DataSource source, final Method toJson) {
try {
jsonStringObject = toJson.invoke(source);
return toJson.invoke(source);
} catch (final InvocationTargetException ex) {
final Throwable targetException = ex.getTargetException();
if (targetException instanceof RuntimeException) {
Expand All @@ -113,21 +127,50 @@ private static Optional<String> callToJsonIfAvailable(final DataSource source) {
}
throw new IllegalStateException("DataSource(Impl)#toJson() threw unexpected Exception.", targetException);
} catch (final IllegalAccessException ex) {
logger.debug("DataSource(Impl)#toJson is not accessible unexpectedly. DataSource: {}, toJson: {}, ",
source.getClass(), toJson);
throw new IllegalStateException("DataSource(Impl)#toJson() is not accessible.", ex);
// "org.embulk.util.config.DataSourceImpl" fo embulk-util-config 0.1.4 or earlier was package-private,
// then invoking "toJson" of "org.embulk.util.config.DataSourceImpl" caused IllegalAccessException.
// https://github.com/embulk/embulk-util-config/pull/20
//
// Pass-through to the next step to setAccessible(true) for the "toJson" Method instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: I assumed a pass-through here means there's a possibility of another type of exception is thrown but could be resolved with the "accessible = true" hack?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought is,

If toJson.invoke(source); throws a type of Exception:

  1. If the Exception is InvocationTargetException: all rethrown to fail explicittly.
  2. If the Exception is IllegalAccessException: passing-through intentionally.
  3. Otherwise: thrown-up to fail.

And those are intended and expected. The case 1 and case 3 should fail immediately. Only the case 2 should pass-through to the next step.

Then, only in the case 2, the accessible flag should be set back false by toJson.setAccessible(false); in finally.

}

if (jsonStringObject == null) {
throw new NullPointerException("DataSource(Impl)#toJson() returned null.");
}
if (!(jsonStringObject instanceof String)) {
throw new ClassCastException(
"DataSource(Impl)#toJson() returned not a String: "
+ jsonStringObject.getClass().getCanonicalName());
}
logger.warn("DataSource(Impl)#toJson is not accessible unexpectedly. DataSource class: {}, toJson method: {}",
source.getClass(), toJson);
logger.warn("The plugin or another plugin is estimated to be using embulk-util-config:0.1.4 or earlier.");
logger.warn("To workaround it, trying to invoke toJson forcibly by Method#setAccessible(true).");

return Optional.of((String) jsonStringObject);
synchronized (toJson) {
try {
toJson.setAccessible(true);
} catch (final SecurityException ex) {
throw new IllegalStateException(
"Method#setAccessible(true) is forbidden for thie method DataSource(Impl)#toJson. "
+ "DataSource class: " + source.getClass().toString()
+ "toJson method: " + toJson.toString(),
ex);
}

try {
return toJson.invoke(source);
} catch (final InvocationTargetException ex) {
final Throwable targetException = ex.getTargetException();
if (targetException instanceof RuntimeException) {
throw (RuntimeException) targetException;
}
if (targetException instanceof Error) {
throw (Error) targetException;
}
throw new IllegalStateException("DataSource#toJson() threw unexpected Exception.", targetException);
} catch (final IllegalAccessException ex) {
throw new IllegalStateException(
"DataSource(Impl)#toJson is not accessible even after Method#setAccessible(true). "
+ "DataSource class: " + source.getClass().toString()
+ "toJson method: " + toJson.toString(),
ex);
} finally {
toJson.setAccessible(false);
}
}
Comment on lines +142 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: If we narrow the scope of toJson by obtaining a new local variable Method toJson = getToJsonMethod(source), is there a need for synchronizing and restore the accessibility here? In case there's no need, do you still consider it potentially dangerous?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it can still be dangerous because:

  • Method#setAccessible is modifying the "method".
  • The toJson instance, which is retrieved by Class#getMethod("toJson"), is expected to be the same instance if another Class#getMethod("toJson") is called in another thread.

Yeah, we have to consider a lot of possibility with this type of hacks -- then, as I mentioned, I'm not very positive to merge this.

}

private static ObjectNode callGetObjectNodeAndRebuildIfAvailable(final DataSource source, final ObjectMapper mapper) {
Expand Down