https://github.com/jenkinsci/git-plugin/pull/15
One of the issues I've noticed with the Git plug-in for Hudson (Git Hudson plug-in v1.1.4) is that the wrong email address is used for committers. The email address used is the person's full name, rather than the email address listed on the git commit (i.e. John Smith@myhost.com) This issue was reported on Hudson's site but there was no resolution (in spite it being an open ticket since August 2010), so I decided to look into it.
The main error message that gets reported is "Illegal whitespace in address in string". What appears to be happening is that Hudson is using the fullname of the GitHub committer for the email prefix address (i.e. John Smith@myhostcom.com) instead of the Git committer/author address:
Incorrect mail address used for sending email notifications
I managed to trace the issue to GitChangeSet.java from the Git plug-in and the Hudson Mailer.java files. The quick short-term fix patch is simply to invoke user.addProperty() regardless of whether user.getProperty(Mailer.UserProperty.class) has already been set. You can review the diff from this Github fork:
https://github.com/rogerhu/Hudson-GIT-plugin/commit/f4421e866563586ee8573cf7757946c90e5873c3
You can recompile the Hudson plug-in by downloading Maven2, changing GitChangeSet.java, and doing a "mvn build". You then shutdown Hudson, copy the target/git.hpi into your plugins/ dir, rename/remove the current expanded plugin dir, and create a .pinned file to inform Hudson not to replace the .hpi build when you reset:
http://wiki.hudson-ci.org/display/HUDSON/Plugin+tutorial
Deploying a custom build of a core plugin Stop Hudson Copy the custom HPI file to $HUDSON_HOME/plugins Remove the previously expanded plugin directory Create an empty file calledThe following diff worked for me. The old code in the Git plug-in attempted to check to see if the Mailer.UserProperty.class had already been set. The problem with this approach is that this class is set to null when a User object is first initialized..hpi.pinned - e.g. maven-plugin.hpi.pinned Start Hudson
diff --git a/src/main/java/hudson/plugins/git/GitChangeSet.java b/src/main/java/hudson/plugins/git/GitChangeSet.java
index 7fb0327..3e9455f 100644
--- a/src/main/java/hudson/plugins/git/GitChangeSet.java
+++ b/src/main/java/hudson/plugins/git/GitChangeSet.java
@@ -232,13 +232,16 @@ public class GitChangeSet extends ChangeLogSet.Entry {
User user = User.get(csAuthor, true);
- // set email address for user if needed
- if (fixEmpty(csAuthorEmail) != null && user.getProperty(Mailer.UserProperty.class) == null) {
- try {
- user.addProperty(new Mailer.UserProperty(csAuthorEmail));
- } catch (IOException e) {
- // ignore error
- }
+ // won't work because it creates a recursive loop
+ // String adrs = fixEmpty(user.getProperty(Mailer.UserProperty.class).getAddress());
+
+ // set email address for user -- removes the default one stored in Users table (null)
+ if (fixEmpty(csAuthorEmail) != null /* && adrs == null */) {
+ try {
+ user.addProperty(new Mailer.UserProperty(csAuthorEmail)); // addProperty() will overwrite the existing property
+ } catch(IOException e) {
+ // ignore error
+ }
}
return user;
The problem is that when the User object is initialized inside Hudson, it also calls a load() function, which attempts to initialize all UserProperty extensions, including the one defined by the Mailer UserProperty:
private User(String id, String fullName) {
this.id = id;
this.fullName = fullName;
load();
}
.
.
.
// allocate default instances if needed.
// doing so after load makes sure that newly added user properties do get reflected
for (UserPropertyDescriptor d : UserProperty.all()) {
if(getProperty(d.clazz)==null) {
UserProperty up = d.newInstance(this);
if(up!=null)
properties.add(up);
}
}
By invoking the addProperty(Mailer.UserProperty(csAuthorEmail)) directly, Hudson will first attempt to look for an identical matching Mailer.UserProperty(), remove it, and replace it with this one. Therefore, this patch will cause the e-mail address of the Git committer to take the highest precedence. We can see how this works by looking at the addProperty() function in core/src/main/java/hudson/model/User.java:
/**
* Updates the user object by adding a property.
*/
public synchronized void addProperty(UserProperty p) throws IOException {
UserProperty old = getProperty(p.getClass());
List ps = new ArrayList(properties);
if(old!=null)
ps.remove(old);
ps.add(p);
p.setUser(this);
properties = ps;
save();
}
More background about the issue: If a default suffix (i.e. myhost.com) is used in the Hudson configuration, then Hudson will attempt to use the full name and append the @myhost.com, causing the javax.mail package to throw an exception. The code that handles all this resolve this info is the getAddress() function of the Mailer.java, which first tries to see if the private e-mail variable is set before attempting to look elsewhere:tasks/Mailer.java
@Exported
public String getAddress() {
if(emailAddress!=null)
return emailAddress;
// try the inference logic
return MailAddressResolver.resolve(user);
}
Since emailAddress is null, the MailAdderessResolver() tries to do a bunch of guesses (i.e. parse the string to see if there is an '@') before it settles on using the full-name + default hostname suffix. If you don't have a User explicitly defined in your Hudson configuration (i.e. a config.xml inside your users/ dir, or the user listed within the Manage Users screen), Hudson will set the default e-mail address to be null when the User object is initialized. When this happens, the Git plug-in will check to see if the user already has a Mailer.UserProperty. Since it does, the if check for the Hudson Git will fail and the Git committer's e-mail address will never be added.
What's the long term fix? The issue with the patch above is that if a User is explicitly defined in Hudson and is a Git committer, then the Git committer's email will always win. What if we want to keep it such that the User defined as a Hudson user is still has the highest priority?
The issue is the getAddress() function within the core/src/main/java/hudson/tasks/Mailer.java file, which denotes the emailAddress variable as a private final variable. Therefore, emailAddress can only be set once (it will be set to null), and it cannot be modified by outside instantiations.
public static class UserProperty extends hudson.model.UserProperty {
/**
* The user's e-mail address.
* Null to leave it to default.
*/
private final String emailAddress;
public UserProperty(String emailAddress) {
this.emailAddress = emailAddress;
}
@Exported
public String getAddress() {
if(emailAddress!=null)
return emailAddress;
// try the inference logic
return MailAddressResolver.resolve(user);
}
@Extension
public static final class DescriptorImpl extends UserPropertyDescriptor {
public String getDisplayName() {
return Messages.Mailer_UserProperty_DisplayName();
}
public UserProperty newInstance(User user) {
return new UserProperty(null);
}
@Override
public UserProperty newInstance(StaplerRequest req, JSONObject formData) throws FormException {
return new UserProperty(req.getParameter("email.address"));
}
}
}
If we can change emailAddress not to be final and/or private, then we could modify the existing emailAddress or perform checks whether emailAddress is null before overwriting the Git committer email address.
FYI -- calling getAddress() unfortunately has the side-effect of calling MailAddressResolver.resolve, which will attempt to call getProjects() which will in turn eventually call getAddress() again. So either we make the emailAddress public or we need to expose a different function that explicitly returns back the emailAddress without calling MailAddressResolver.resolve(). A fix to make emailAddress public or a function to set the UserProperty only when emailAddress is null would then allow us to use Hudson-created accounts as the last word in e-mail commit notify messages.
Note: Hudson depends on source-code management plug-ins to have a function called getAuthor(), which is the main function at the heart of this issue. When the mailer wants to look for the culprits who broke the code (getCulprits), it outsources the getAuthor() function to the plug-in, which in turn can add a Mailer.UserProperty() to the User object for use in mail notifications. Unfortunately there seems to have been an unexpected consequence in allowing null Mailer.UserProperty's to be instantiated by the User constructor. Since I could find no other source-code management plugin for Hudson that implemented anything more complex than User.get() calls, it may be why this issue was not caught earlier.
No comments:
Post a Comment