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

The context type parameter on ConsoleAnnotatorFactory did not work #3662

Merged
merged 3 commits into from Oct 5, 2018

Conversation

2 participants
@jglick
Member

jglick commented Sep 28, 2018

Whatever @kohsuke was attempting to do with type parameters in c6acb5b, it did not work, and c5f1ed5 did not fix it. Reproducible by amending jenkinsci/timestamper-plugin#24 with

diff --git a/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotator.java b/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotator.java
index e7a08b8..1d75b0c 100644
--- a/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotator.java
+++ b/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotator.java
@@ -46,7 +46,7 @@ import hudson.plugins.timestamper.io.TimestampsReader;
  *
  * @author Steven G. Brown
  */
-public final class TimestampAnnotator extends ConsoleAnnotator<Object> {
+public final class TimestampAnnotator extends ConsoleAnnotator<Run<?, ?>> {
 
   private static final long serialVersionUID = 1L;
 
@@ -69,9 +69,7 @@ public final class TimestampAnnotator extends ConsoleAnnotator<Object> {
 
   /** {@inheritDoc} */
   @Override
-  public ConsoleAnnotator<Object> annotate(Object context, MarkupText text) {
-    Run<?, ?> build = (Run<?, ?>) context;
-
+  public ConsoleAnnotator<Run<?, ?>> annotate(Run<?, ?> build, MarkupText text) {
     try {
       if (timestampsReader == null) {
         ConsoleLogParser.Result logPosition = logParser.seek(build);
diff --git a/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotatorFactory3.java b/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotatorFactory3.java
index 1b686ff..64dfc2c 100644
--- a/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotatorFactory3.java
+++ b/src/main/java/hudson/plugins/timestamper/annotator/TimestampAnnotatorFactory3.java
@@ -41,15 +41,11 @@ import jenkins.YesNoMaybe;
  * @author Steven G. Brown
  */
 @Extension(dynamicLoadable = YesNoMaybe.YES)
-public final class TimestampAnnotatorFactory3 extends ConsoleAnnotatorFactory<Object> {
+public final class TimestampAnnotatorFactory3 extends ConsoleAnnotatorFactory<Run<?, ?>> {
 
   /** {@inheritDoc} */
   @Override
-  public ConsoleAnnotator<Object> newInstance(Object context) {
-    if (!(context instanceof Run<?, ?>)) {
-      return null; // something else
-    }
-    Run<?, ?> build = (Run<?, ?>) context;
+  public ConsoleAnnotator<Run<?, ?>> newInstance(Run<?, ?> build) {
     if (TimestampNote.useTimestampNotes(build)) {
         return null; // not using this system
     }

which throws a ClassCastException against current cores but works with this patch.

Proposed changelog entries

  • ConsoleAnnotatorFactory mishandled its type parameter, effectively forcing all implementations to use Object or rawtypes.
@@ -113,7 +113,7 @@ protected void setContentType(StaplerResponse rsp) {
rsp.setContentType(isHtml() ? "text/html;charset=UTF-8" : "text/plain;charset=UTF-8");
}
private ConsoleAnnotator createAnnotator(StaplerRequest req) throws IOException {
private ConsoleAnnotator<T> createAnnotator(StaplerRequest req) throws IOException {

This comment has been minimized.

@jglick

jglick Sep 28, 2018

Member

Most of the patch is correcting or suppressing rawtypes/unchecked warnings. Will call out the two actual fixes explicitly.

@jglick

jglick Sep 28, 2018

Member

Most of the patch is correcting or suppressing rawtypes/unchecked warnings. Will call out the two actual fixes explicitly.

@@ -135,7 +135,7 @@ private ConsoleAnnotator createAnnotator(StaplerRequest req) throws IOException
throw new IOException(e);
}
// start from scratch
return ConsoleAnnotator.initial(context==null ? null : context.getClass());
return ConsoleAnnotator.initial(context);

This comment has been minimized.

@jglick

jglick Sep 28, 2018

Member

This made no sense. context is a T. initial takes a T. So why were we passing a Class<T>?

@jglick

jglick Sep 28, 2018

Member

This made no sense. context is a T. initial takes a T. So why were we passing a Class<T>?

public Class type() {
Type type = Types.getBaseClass(getClass(), ConsoleAnnotator.class);
public Class<?> type() {
Type type = Types.getBaseClass(getClass(), ConsoleAnnotatorFactory.class);

This comment has been minimized.

@jglick

jglick Sep 28, 2018

Member

Uh, this is a ConsoleAnnotatorFactory, not a ConsoleAnnotator, so type was always null, causing this method to always return Object.class, causing _for to always call newInstance, resulting in the ClassCastException.

@jglick

jglick Sep 28, 2018

Member

Uh, this is a ConsoleAnnotatorFactory, not a ConsoleAnnotator, so type was always null, causing this method to always return Object.class, causing _for to always call newInstance, resulting in the ClassCastException.

jglick added some commits Oct 5, 2018

@jglick jglick merged commit f32c0ba into jenkinsci:master Oct 5, 2018

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@jglick jglick deleted the jglick:ConsoleAnnotatorFactory branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment