You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by no...@apache.org on 2006/10/15 13:50:28 UTC
svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james:
transport/mailets/AbstractRedirect.java
transport/mailets/AbstractVirtualUserTable.java
transport/mailets/DSNBounce.java util/Misc.java
Author: norman
Date: Sun Oct 15 04:50:23 2006
New Revision: 464156
URL: http://svn.apache.org/viewvc?view=rev&rev=464156
Log:
Move duplicate code to util class.
Added:
james/server/trunk/src/java/org/apache/james/util/Misc.java (with props)
Modified:
james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractRedirect.java
james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractVirtualUserTable.java
james/server/trunk/src/java/org/apache/james/transport/mailets/DSNBounce.java
Modified: james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractRedirect.java
URL: http://svn.apache.org/viewvc/james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractRedirect.java?view=diff&rev=464156&r1=464155&r2=464156
==============================================================================
--- james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractRedirect.java (original)
+++ james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractRedirect.java Sun Oct 15 04:50:23 2006
@@ -46,6 +46,7 @@
import org.apache.mailet.dates.RFC822DateFormat;
import org.apache.james.core.MailImpl;
import org.apache.james.core.MimeMessageUtil;
+import org.apache.james.util.Misc;
import org.apache.mailet.GenericMailet;
import org.apache.mailet.Mail;
@@ -977,7 +978,7 @@
boolean keepMessageId = false;
// duplicates the Mail object, to be able to modify the new mail keeping the original untouched
- MailImpl newMail = new MailImpl(originalMail,newName(originalMail));
+ MailImpl newMail = new MailImpl(originalMail,Misc.newName(originalMail,random));
try {
// We don't need to use the original Remote Address and Host,
// and doing so would likely cause a loop with spam detecting
@@ -1082,42 +1083,6 @@
private static final java.util.Random random = new java.util.Random(); // Used to generate new mail names
- /**
- * Create a unique new primary key name.
- *
- * @param mail the mail to use as the basis for the new mail name
- * @return a new name
- */
- private String newName(Mail mail) throws MessagingException {
- String oldName = mail.getName();
-
- // Checking if the original mail name is too long, perhaps because of a
- // loop caused by a configuration error.
- // it could cause a "null pointer exception" in AvalonMailRepository much
- // harder to understand.
- if (oldName.length() > 76) {
- int count = 0;
- int index = 0;
- while ((index = oldName.indexOf('!', index + 1)) >= 0) {
- count++;
- }
- // It looks like a configuration loop. It's better to stop.
- if (count > 7) {
- throw new MessagingException("Unable to create a new message name: too long."
- + " Possible loop in config.xml.");
- }
- else {
- oldName = oldName.substring(0, 76);
- }
- }
-
- StringBuffer nameBuffer =
- new StringBuffer(64)
- .append(oldName)
- .append("-!")
- .append(random.nextInt(1048576));
- return nameBuffer.toString();
- }
/**
* A private method to convert types from string to int.
Modified: james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractVirtualUserTable.java
URL: http://svn.apache.org/viewvc/james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractVirtualUserTable.java?view=diff&rev=464156&r1=464155&r2=464156
==============================================================================
--- james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractVirtualUserTable.java (original)
+++ james/server/trunk/src/java/org/apache/james/transport/mailets/AbstractVirtualUserTable.java Sun Oct 15 04:50:23 2006
@@ -22,6 +22,7 @@
package org.apache.james.transport.mailets;
import org.apache.james.core.MailImpl;
+import org.apache.james.util.Misc;
import org.apache.james.util.VirtualUserTableUtil;
import org.apache.mailet.GenericMailet;
import org.apache.mailet.Mail;
@@ -162,7 +163,7 @@
// getMailetContext().sendMail(mail.getSender(), recipientsToAddForward, mail.getMessage());
// duplicates the Mail object, to be able to modify the new mail keeping the original untouched
- MailImpl newMail = new MailImpl(mail,newName(mail));
+ MailImpl newMail = new MailImpl(mail,Misc.newName(mail,random));
try {
try {
newMail.setRemoteAddr(java.net.InetAddress.getLocalHost().getHostAddress());
@@ -238,39 +239,4 @@
private static final java.util.Random random = new java.util.Random(); // Used to generate new mail names
- /**
- * Create a unique new primary key name.
- *
- * @param mail the mail to use as the basis for the new mail name
- * @return a new name
- */
- private String newName(Mail mail) throws MessagingException {
- String oldName = mail.getName();
-
- // Checking if the original mail name is too long, perhaps because of a
- // loop caused by a configuration error.
- // it could cause a "null pointer exception" in AvalonMailRepository much
- // harder to understand.
- if (oldName.length() > 76) {
- int count = 0;
- int index = 0;
- while ((index = oldName.indexOf('!', index + 1)) >= 0) {
- count++;
- }
- // It looks like a configuration loop. It's better to stop.
- if (count > 7) {
- throw new MessagingException("Unable to create a new message name: too long. Possible loop in config.xml.");
- }
- else {
- oldName = oldName.substring(0, 76);
- }
- }
-
- StringBuffer nameBuffer =
- new StringBuffer(64)
- .append(oldName)
- .append("-!")
- .append(random.nextInt(1048576));
- return nameBuffer.toString();
- }
}
Modified: james/server/trunk/src/java/org/apache/james/transport/mailets/DSNBounce.java
URL: http://svn.apache.org/viewvc/james/server/trunk/src/java/org/apache/james/transport/mailets/DSNBounce.java?view=diff&rev=464156&r1=464155&r2=464156
==============================================================================
--- james/server/trunk/src/java/org/apache/james/transport/mailets/DSNBounce.java (original)
+++ james/server/trunk/src/java/org/apache/james/transport/mailets/DSNBounce.java Sun Oct 15 04:50:23 2006
@@ -23,6 +23,7 @@
import org.apache.james.Constants;
import org.apache.james.core.MailImpl;
+import org.apache.james.util.Misc;
import org.apache.james.util.mail.MimeMultipartReport;
import org.apache.james.util.mail.dsn.DSNStatus;
import org.apache.mailet.Mail;
@@ -150,7 +151,7 @@
// duplicates the Mail object, to be able to modify the new mail keeping the original untouched
- MailImpl newMail = new MailImpl(originalMail,newName(originalMail));
+ MailImpl newMail = new MailImpl(originalMail,Misc.newName(originalMail,random));
try {
// We don't need to use the original Remote Address and Host,
// and doing so would likely cause a loop with spam detecting
@@ -559,44 +560,6 @@
return ex1.getMessage().trim();
}
}
-
- /**
- * Create a unique new primary key name.
- *
- * @param mail the mail to use as the basis for the new mail name
- * @return a new name
- */
- protected String newName(Mail mail) throws MessagingException {
- String oldName = mail.getName();
-
- // Checking if the original mail name is too long, perhaps because of a
- // loop caused by a configuration error.
- // it could cause a "null pointer exception" in AvalonMailRepository much
- // harder to understand.
- if (oldName.length() > 76) {
- int count = 0;
- int index = 0;
- while ((index = oldName.indexOf('!', index + 1)) >= 0) {
- count++;
- }
- // It looks like a configuration loop. It's better to stop.
- if (count > 7) {
- throw new MessagingException("Unable to create a new message name: too long."
- + " Possible loop in config.xml.");
- }
- else {
- oldName = oldName.substring(0, 76);
- }
- }
-
- StringBuffer nameBuffer =
- new StringBuffer(64)
- .append(oldName)
- .append("-!")
- .append(random.nextInt(1048576));
- return nameBuffer.toString();
- }
-
public String getMailetInfo() {
Added: james/server/trunk/src/java/org/apache/james/util/Misc.java
URL: http://svn.apache.org/viewvc/james/server/trunk/src/java/org/apache/james/util/Misc.java?view=auto&rev=464156
==============================================================================
--- james/server/trunk/src/java/org/apache/james/util/Misc.java (added)
+++ james/server/trunk/src/java/org/apache/james/util/Misc.java Sun Oct 15 04:50:23 2006
@@ -0,0 +1,72 @@
+package org.apache.james.util;
+
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+
+import java.util.Random;
+
+import javax.mail.MessagingException;
+
+import org.apache.mailet.Mail;
+
+// Not sure about the name yet
+// TODO: Rename
+public class Misc {
+
+ private Misc() {}
+
+
+ /**
+ * Create a unique new primary key name for the given MailObject.
+ *
+ * @param mail the mail to use as the basis for the new mail name
+ * @return a new name
+ */
+ public static String newName(Mail mail,Random random) throws MessagingException {
+ String oldName = mail.getName();
+
+ // Checking if the original mail name is too long, perhaps because of a
+ // loop caused by a configuration error.
+ // it could cause a "null pointer exception" in AvalonMailRepository much
+ // harder to understand.
+ if (oldName.length() > 76) {
+ int count = 0;
+ int index = 0;
+ while ((index = oldName.indexOf('!', index + 1)) >= 0) {
+ count++;
+ }
+ // It looks like a configuration loop. It's better to stop.
+ if (count > 7) {
+ throw new MessagingException("Unable to create a new message name: too long."
+ + " Possible loop in config.xml.");
+ }
+ else {
+ oldName = oldName.substring(0, 76);
+ }
+ }
+
+ StringBuffer nameBuffer =
+ new StringBuffer(64)
+ .append(oldName)
+ .append("-!")
+ .append(random.nextInt(1048576));
+ return nameBuffer.toString();
+ }
+}
Propchange: james/server/trunk/src/java/org/apache/james/util/Misc.java
------------------------------------------------------------------------------
svn:eol-style = native
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james:
transport/mailets/AbstractRedirect.java transport/mailets/AbstractVirtualUserTable.java
transport/mailets/DSNBounce.java util/Misc.java
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> Stefano Bagnara wrote:
>
>> Noel J. Bergman wrote:
>>> Stefano Bagnara wrote:
>>>> We don't use the duplicate because we can't assume the originalMail is a
>>>> MailImpl as the contract is that we receive a Mail object.
>>> Well, then duplicate(...) is pretty useless. :-)
>> I agree, but I left it for backward compatibility: people may have code
>> calling that method ;-) Maybe we should mark it as deprecated as
>> duplicate and duplicate(newName) are only called by tests.
>
> Should we consider adding duplicate to the Mailet API? I had originally drafted a contructor based approach in my e-mail, and removed it since we have the same code already in duplicate().
If we could find a way to avoid casting and creating MailImpl objects
all around I would be happy to do this, but unfortunately I think that
currently adding the duplicate method would be not enough because we
need the specific MailImpl class around so we can only be sure we have
it if we use the new MailImpl(Mail original) alternative.
In future we should try to fix this, but we should also add methods to
split a Mail object between recipients, to alias some of the recipients
and so on. Not sure if we should add this to Mail or to MailetContext
but we'll eventually need them (e.g: to correctly support DSN).
I would leave things as is by now.
> At the moment, I don't have a strong preference.
>
>>> MailImpl(Mail) should not create a new name.
>
>> Why not?
>
>> I think it is safer to generate a new name by default: if you want to
>> generate a new mail and keep the same name you can do
>> new MailImpl(orig, orig.getName()).
>
>> I prefer the solution I just committed (where the default behaviour is
>> to generate a new name) because I think it is less error prone
>
> I agree that it is less error prone. I suppose that the question is whether the caller would expect that they'll get a new name as a side-effect. Perhaps. <<shrug>>
>
> --- Noel
Maybe others should say what they think: personally when I see a
constructor getting an object as parameter I don't assume it will
duplicate the object as is: in this case it does not even take the same
object but an interface implemented by the object so I would not expect
anything without reading the javadocs.
Maybe others follow another workflow, I've not a strong opinion on this.
+1 to the "current" solution
-0 to change the default constructor to not change the name and add
anothr constructor with a boolean to change the name.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james: transport/mailets/AbstractRedirect.java transport/mailets/AbstractVirtualUserTable.java transport/mailets/DSNBounce.java util/Misc.java
Posted by "Noel J. Bergman" <no...@devtech.com>.
Stefano Bagnara wrote:
> Noel J. Bergman wrote:
> > Stefano Bagnara wrote:
> > > We don't use the duplicate because we can't assume the originalMail is a
> > > MailImpl as the contract is that we receive a Mail object.
> > Well, then duplicate(...) is pretty useless. :-)
> I agree, but I left it for backward compatibility: people may have code
> calling that method ;-) Maybe we should mark it as deprecated as
> duplicate and duplicate(newName) are only called by tests.
Should we consider adding duplicate to the Mailet API? I had originally drafted a contructor based approach in my e-mail, and removed it since we have the same code already in duplicate().
At the moment, I don't have a strong preference.
> > MailImpl(Mail) should not create a new name.
> Why not?
> I think it is safer to generate a new name by default: if you want to
> generate a new mail and keep the same name you can do
> new MailImpl(orig, orig.getName()).
> I prefer the solution I just committed (where the default behaviour is
> to generate a new name) because I think it is less error prone
I agree that it is less error prone. I suppose that the question is whether the caller would expect that they'll get a new name as a side-effect. Perhaps. <<shrug>>
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james:
transport/mailets/AbstractRedirect.java transport/mailets/AbstractVirtualUserTable.java
transport/mailets/DSNBounce.java util/Misc.java
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> Stefano Bagnara wrote:
>
>> Considering that the newName method is currently called only during the
>> creation of a new MailImpl object I think that a third solution would
>> be better.
>
> This is just using a constructor instead of the duplicate() call. But OK.
>> We don't use the duplicate because we can't assume the originalMail is a
>> MailImpl as the contract is that we receive a Mail object.
>
> Well, then duplicate(...) is pretty useless. :-)
I agree, but I left it for backward compatibility: people may have code
calling that method ;-)
Maybe we should mark it as deprecated as duplicate and
duplicate(newName) are only called by tests.
>> Add a constructor to MailImpl:
>> public MailImpl(Mail original) that simply create a newName
>> and then calls this(original, newName(original));
>
> MailImpl(Mail) should not create a new name. If we want to use the contructor approach, go with the same pattern:
Why not?
I think it is safer to generate a new name by default: if you want to
generate a new mail and keep the same name you can do new MailImpl(orig,
orig.getName()).
> MailImpl(Mail orig) { this(orig, false) }
> MailImpl(Mail orig, boolean newName) { ... }
>
> Work for you?
>
> --- Noel
I prefer the solution I just committed (where the default behaviour is
to generate a new name) because I think it is less error prone than the
boolean value.
My idea is supported by the fact that currently we only need the
constructor that generate a new name and we would never use the default
one (that could lead to problems because you start having 2 Mail objects
possibly different and sharing the same key).
What others thinks?
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james: transport/mailets/AbstractRedirect.java transport/mailets/AbstractVirtualUserTable.java transport/mailets/DSNBounce.java util/Misc.java
Posted by "Noel J. Bergman" <no...@devtech.com>.
Stefano Bagnara wrote:
> Considering that the newName method is currently called only during the
> creation of a new MailImpl object I think that a third solution would
> be better.
This is just using a constructor instead of the duplicate() call. But OK.
> We don't use the duplicate because we can't assume the originalMail is a
> MailImpl as the contract is that we receive a Mail object.
Well, then duplicate(...) is pretty useless. :-)
> Add a constructor to MailImpl:
> public MailImpl(Mail original) that simply create a newName
> and then calls this(original, newName(original));
MailImpl(Mail) should not create a new name. If we want to use the contructor approach, go with the same pattern:
MailImpl(Mail orig) { this(orig, false) }
MailImpl(Mail orig, boolean newName) { ... }
Work for you?
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james:
transport/mailets/AbstractRedirect.java transport/mailets/AbstractVirtualUserTable.java
transport/mailets/DSNBounce.java util/Misc.java
Posted by Stefano Bagnara <ap...@bago.org>.
Considering that the newName method is currently called only during the
creation of a new MailImpl object I think that a third solution would be
better.
Add a constructor to MailImpl:
public MailImpl(Mail original) that simply create a newName and then
calls this(original, newName(original));
We don't use the duplicate because we can't assume the originalMail is a
MailImpl as the contract is that we receive a Mail object.
Stefano
Noel J. Bergman wrote:
> I really don't like this change.
>
> I'd suggest moving newName to MailImpl, and hiding the random there, too. Then you could revert the calling code to the way it was, except for changing from newName(originalMail) to MailImpl.newName(originalMail).
>
> But even better, we already have
>
> public Mail duplicate() { ... }
> public Mail duplicate(String newName) { .. }
>
> we could add:
>
> public Mail duplicate(boolean newName) {...}
>
> and call that, rather than the constructor directly:
>
> - MailImpl newMail = new MailImpl(originalMail,newName(originalMail));
> + MailImpl newMail = (MailImpl) originalMail.duplicate(true);
>
> --- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: svn commit: r464156 - in /james/server/trunk/src/java/org/apache/james: transport/mailets/AbstractRedirect.java transport/mailets/AbstractVirtualUserTable.java transport/mailets/DSNBounce.java util/Misc.java
Posted by "Noel J. Bergman" <no...@devtech.com>.
I really don't like this change.
I'd suggest moving newName to MailImpl, and hiding the random there, too. Then you could revert the calling code to the way it was, except for changing from newName(originalMail) to MailImpl.newName(originalMail).
But even better, we already have
public Mail duplicate() { ... }
public Mail duplicate(String newName) { .. }
we could add:
public Mail duplicate(boolean newName) {...}
and call that, rather than the constructor directly:
- MailImpl newMail = new MailImpl(originalMail,newName(originalMail));
+ MailImpl newMail = (MailImpl) originalMail.duplicate(true);
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org