Skip to content
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

[JENKINS-30002] Improve Util.isOverridden #1804

Merged
merged 1 commit into from Oct 5, 2015

Conversation

8 participants
@amuniz
Copy link
Member

commented Aug 18, 2015

@reviewbybees

This comment has been minimized.

Copy link

commented Aug 18, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -33,6 +34,7 @@
import hudson.util.QuotedStringTokenizer;
import hudson.util.VariableResolver;
import hudson.util.jna.WinIOException;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 18, 2015

Member

please avoid such unrelated changes

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 18, 2015

Member

Thanks for fixing it!

} catch (NoSuchMethodException e) {
throw new AssertionError(e);
}
}

private static Method getMethod(@Nonnull Class clazz, @Nonnull String methodName, @Nonnull Class... types) throws NoSuchMethodException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 18, 2015

Member

Seems it can return null if an exception happens and superclass is null. The method should be marked as CheckForNull

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 18, 2015

Author Member

It can not return null, see the last if block.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 18, 2015

Member

Sorry, I've missed the obvious code block :(
From my POW, it would be better not to throw runtime exceptions. Null might be better here, because the calling method should be able the case.

Currently we have a runtime exception, which may be thrown away from the new isOverriden() method and which has no documentation or such runtime exception. This behavior is very hazardous for a widely used method.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

Sorry, I'm putting a 🐛 on it, because I need more clarification.

It smells that the code of getMethod() breaks the encapsulation. Seems it may return a private method of the superclass with the requested signature, which is wrong even if does not impact isOverriden(). Could you clarify it? Probably, it would be enough to rename a method to a less generic one (or write Javadocs)

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

@oleg-nenashev you are right, it breaks encapsulation, but it's exactly what we want in this case, that's why we are avoiding to use the original Class.getMethod which only search for public overrides.

See the test code for a clear example. Current isOverridden does not work properly for protected overridden methods.

(New lines in imports section removed).

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

@oleg-nenashev BTW did you read the JIRA issue? All this is already explained there.

@amuniz amuniz force-pushed the amuniz:JENKINS-30002 branch from d4569e6 to 818bc2a Aug 18, 2015

@olivergondza

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

I agree with Oleg that this should work for public, protected and package protected methods, though not for private. It is not overriding by definition and it makes no sense for any use case I can imagine.

@tfennelly

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

@amuniz yeah, not sure I get why we'd include private methods? I think the PR would probably be fine if it included a !Modifier.isPrivate(method.getModifiers()) check to exclude private methods.

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

Ok, it makes sense.
Fixed.

@amuniz amuniz force-pushed the amuniz:JENKINS-30002 branch 2 times, most recently from 8c5cb78 to f6c2e02 Aug 18, 2015

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

@amuniz
And WDYT about the runtime exception?

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

@oleg-nenashev I'm keeping the original behavior. Perhaps there is some code out there relying on this, so I'd prefer to keep it as is. Don't you agree?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

@amuniz
Yes, I agree. It would be great to just add the AssertionError to Javadoc
Should we also add a check for statics? They are not overridable, but I don't know how this reflection magic works for them.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

I think it would suffice to throw an IllegalArgumentException if the method is not either public or protected, or if it is static or final, or if the “derived” class is not assignable to the parent class.

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

@jglick I think that would break backward compatibility, If someone is catching AssertionError for some reason, that catch will never be reached anymore.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

Should also check the comment in org.jvnet.tiger_types.Types.isOverriding (no clue where source is—last seen on java.net) regarding overrides of generic methods:

public abstract class Base<T>
    public void setX(T t) {}
    public T getX() {return null;}
}
public class Derived extends Base<Integer> {
    public void setX(Integer i) {}
    public Integer getX() {return 0;}
}

Here both methods are overridden in Derived. I think the current implementation handles that, but it would be worth testing.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

If someone is catching AssertionError for some reason…

…then they should be punished!

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2015

All fixed.

* Checks if the public method defined on the base type with the given arguments
* are overridden in the given derived type.
* Checks if the method defined on the base type with the given arguments
* is overridden in the given derived type.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 18, 2015

Member

I still think @throws would be useful here

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

🐝 with a minor comment

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2015

@reviewbybees

This comment has been minimized.

Copy link

commented Aug 19, 2015

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@tfennelly

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

🐝

Nice job @amuniz

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2015

Would be something additional required at my side to have this merged?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 3, 2015

I'm OK with the merge, but it would be better to squash PRs. There are people who are against squashing, but as a past maintainer of an internal Jenkins fork I feel strongly about it.

@tfennelly

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

@oleg-nenashev yeah, I agree with you on squashing. The vast majority of the history in a PR is just irritating noise; adding annotations, NPE checks and other such.

The merge commit argument (etc) is fine, but I think the PR owner should be allowed to squash the PR before final commit if they want. In fact, is this even a "rule" that the community has agreed upon?

On other OSS projects I have contributed to, you'd get your balls handed to you in a jar if you pushed a multi-commit PR upstream. In fact, I know some that hate merge commits i.e. they want the upstream commit history to read like a book as much as possible.

@amuniz amuniz force-pushed the amuniz:JENKINS-30002 branch from 858a1fd to 89a0050 Oct 5, 2015

@amuniz

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2015

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

Thanks @amuniz . I'll merge it when the test passes

@tfennelly Yes, my position is same. Probably I'll start a thread on jenkinsci-dev and propose such rule.

@tfennelly

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

@oleg-nenashev or propose/confirm that there is no such rule i.e. it's the PR owner choice.

oleg-nenashev added a commit that referenced this pull request Oct 5, 2015

Merge pull request #1804 from amuniz/JENKINS-30002
[JENKINS-30002] Improve Util.isOverridden

@oleg-nenashev oleg-nenashev merged commit 6adbb33 into jenkinsci:master Oct 5, 2015

1 check passed

Jenkins This pull request looks good
Details

oleg-nenashev added a commit that referenced this pull request Oct 5, 2015

tfennelly added a commit to tfennelly/jenkins that referenced this pull request Oct 5, 2015

Merge branch 'master' into plugin-manager-dependants
* master: (58 commits)
  Changelog: Replace the PR reference by the JIRA issue reference
  Noting jenkinsci#1818
  Noting jenkinsci#1804
  [JENKINS-30002] Improve Util.isOverridden
  Noting jenkinsci#1842
  [FIXED JENKINS-30777] this concludes the fix
  [JENKINS-30777] also allow slaves to decorate logger
  [JENKINS-30777] Generalized the signature to work with Run, not just AbstractBuild.
  Diamond operator
  [FIXED JENKINS-29876] CheckForNull job in ReverseBuildTrigger
  Noting JENKINS-30084 in changelog
  [JENKINS-30084] remove extra space
  [JENKINS-30084] address feedbacks
  [JENKINS-30084] indent back
  [JENKINS-30084] fixing test
  [JENKINS-30084] fixing test
  [JENKINS-30084] enhancing test case
  [JENKINS-30084] test added to make sure a flyweight task can be blocked at last minute
  [JENKINS-30084] fix regression when flyweight task is blocked by upstream/downstream project
  [JENKINS-30084] some more polish
  ...
@jglick

This comment has been minimized.

Copy link
Member

commented Oct 19, 2015

Please, please do not squash commits inside a PR. If you insist on producing a squashed commit for the master branch to close the PR, that is one thing (which I disagree with), but it does not require destroying history in the PR.

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

@jglick please please don't merge master into PR before merging it to master.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

@jglick @tfennelly
I agree that we don't want to request squashing from contributors to save their time. BTW merging of unsquashed commits to the master of a big OSS project is a really bad thing IMO. I'm not going to do it.

BTW I've switched to this script, which performs the things semi-automatically. So no I can squash commits from others w/o significant efforts.

@amuniz amuniz deleted the amuniz:JENKINS-30002 branch Oct 20, 2015

@stephenc

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

and @oleg-nenashev forced his personal opinion on squashing commits when merging to master
sad panda

@jglick

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

I agree that we don't want to request squashing from contributors to save their time.

No, my concern was rather about destroying the history of activity recorded in the PR. A script like yours at least leaves the PR intact, so we have the information available so long as we host on GitHub, even if information is destroyed in the official project history.

don't merge master into PR before merging it to master

No idea what that refers to.

@stephenc

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

don't merge master into PR before merging it to master

No idea what that refers to.

The only guess I have is 955eb72 but that wasn't you anyway!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

@stephenc

and @oleg-nenashev forced his personal opinion on squashing commits when merging to master

I didn't force anything. My statement was:

I'm OK with the merge, but it would be better to squash PRs. There are people who are against squashing, but as a past maintainer of an internal Jenkins fork I feel strongly about it.

I just provided my opinion. I didn't block the PR by -1 or bug, so anybody else was able to merge the PR if he thinks differently. It's my right not to merge PRs when I'm not 100% happy, but I don't require other contributors or maintainers to follow the squash rule outside plugins I maintain.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 20, 2015

@jglick

No, my concern was rather about destroying the history of activity recorded in the PR. A script like yours at least leaves the PR intact, so we have the information available so long as we host on GitHub, even if information is destroyed in the official project history.

I'm not against multi-commit changes when these commits are really reasonable on the atomic level (E.g. "Added new API method" + "Added unit tests for new API" + "Migrated stuff to the new API"). I'm against dozens of commits happening during the PR polishing: typo fixes, minor regressions, etc. I doubt this info will be ever required, but it makes the analysis very difficult when you want to investigate the issue without checking out the entire repo.

Probably keeping full histories in PRs is a convenient approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.