-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
#2241 fix bug: when running with a fat jar, need to read entries with openConnection #2952
Conversation
… openConnection when running in IDE or a thin jar, it is ok, when running with a fat jar in server, new JarFile can not open and read the entries.
Welcome @wkclz! |
Hrm, do you have a pointer to documentation about why this works this way? I definitely don't want this library to be opening URLs over the network, so is there a way to restrict this to file based entries in the fat jar? |
Also, you need to sign the CLA. |
Running Yaml.load("yaml String"), in "io.kubernetes.client.util.Yaml:553" Make a break point to trace the code, ModelMapper.initModelMap() is in the static, it will scan and cache classes to classesByGVK or preBuiltClassesByGVK from "io.kubernetes.client.openapi.models". If try to get the jar like this: ((JarURLConnection) packageURL.openConnection()).getJarFile(), it will be OK. I have try it by overwrite this class. In this issue #2241, I output the stack of the problem. CLA is ok now. |
jarFileName = jarFileName.substring(5, jarFileName.indexOf("!")); | ||
logger.info("Loading classes from jar {}", jarFileName); | ||
try (JarFile jf = new JarFile(jarFileName)) { | ||
logger.info("Loading classes from jar {}", packageURL.getFile()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an explicit check for packageURL.startsWith("jar:")
here?
Ok, this approach works for me, I asked for a single explicit check for the Is there a way that we can add a unit test for this? |
In a fat jar, packageURL start with "nested:", not a fat jar, it start with "jar:".
Running unit test with |
packageURL is not jar or nested, it would be return
/lgtm I'm fine to merge this w/o a test given the complexity of creating one. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, wkclz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
re import URLDecoder
Looks like this isn't working right in our e2e tests, it can't find the class for nodes any more. |
fixed, and tested e2e tests |
Fixes: #1659 |
jf = new JarFile(jarFileName); | ||
} | ||
if (jf == null) { | ||
logger.error("Loading classes from jar with error packageURL: {}", jarFileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.error("Loading classes from jar with error packageURL: {}", jarFileName); | |
logger.error("Loading classes from jar with error packageURL: {}", packageURL); |
nit: looks like we might want to dump the packageURL here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same with packageURL
@yue9944882 I'll wait for your lgtm on this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
In a fat jar, packageURL start with "nested:", not a fat jar, it start with "jar:". (cherry picked from commit ec7e7b9)
…eturn packageURL is not jar or nested, it would be return (cherry picked from commit 88ac227)
re import URLDecoder (cherry picked from commit de71d98)
…release-19 Cherry Pick #2952 To Release 19
When running in IDE or a thin jar, it is ok.
When running with a fat jar in server, new JarFile can not open and read the entries.