[JENKINS-75556] Fix ClassCastException and ClassNotFoundException in client builder

[strangelookingnerd]

Follow-up for #69 that fixes https://issues.jenkins.io/browse/JENKINS-75556

@MarkEWaite this makes #77 obsolete

Testing done

Manually verified Test connection in System Configuration works as expected.

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[MarkEWaite]

Thanks very much!

[Puffy1215]

I had run into this bug with my testing of the plugin as well. Did you take a look into this solution on the jenkins docs for class loading issues? https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/#context-class-loaders

I was able to get this builder to work using the code below. Your implementation has the upside of being much simpler to follow, but I wasn't sure if it came with any other considerations to consider.

Thread t = Thread.currentThread();
ClassLoader orig = t.getContextClassLoader();
t.setContextClassLoader(apiProxyClass.getClassLoader());
try {
         GiteeApiProxy apiProxy = ((ResteasyWebTarget) builder
                    // ... builder functions
                    .build().target(url))
                    .proxyBuilder(apiProxyClass)
                    .classloader(t.getContextClassLoader())
                    .build();

          return new ResteasyGiteeClient(url, apiProxy, pullRequestIdProvider);
       } finally {
            t.setContextClassLoader(orig);
       }
}

[MarkEWaite]

@Puffy1215 have you tested the implementation in this pull request? I'm more interested in delivering a fix that works than in changing implementation from one form to another

[strangelookingnerd]

@Puffy1215 Yeah, I tried a similar approach as you describe at first but did not really like fiddeling with the classloader.

[Puffy1215]

Understood, I was interested in the thought process. I am perfectly comfortable with your solution as it is easier to understand.

I have tried your fixes and have found no issues.

[MarkEWaite]

I have tried your fixes and have found no issues.

That is great news. I will merge and release this pull request.

[Puffy1215]

Do we still need this class? I cant find any reference to getHttpEngine being used in the codebase, and it is marked as deprecated. In either case it is confusing here that our private static class shares the name with the abstract class ResteasyClientBuilder.

I think it would be better if we renamed this class (ResteasyClientBuilderWithHttpCreds?), or moved the functionality to a private function. Open to thoughts. Perhaps this could be a separate issue.

[strangelookingnerd]

I can just assume that it had its justification before #69. There was a large upgrade to resteasy and the justification could just have vanished by migrating the codebase to a newver version.

I'd recommend following that up in a separate PR in order to deliver a fix as #78 (comment) already points out.

[Puffy1215]

Agreed. I can open an issue with this. I'll need to investigate some older commits but I believe the deprecated function has not been used, even previously, for a long time.