You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/06/14 02:37:45 UTC
[GitHub] [commons-configuration] darkma773r opened a new pull request, #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
darkma773r opened a new pull request, #186:
URL: https://github.com/apache/commons-configuration/pull/186
Contains the changes from the previously closed #107, rebased on master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r897506536
##########
src/test/java/org/apache/commons/configuration2/TestDataConfiguration.java:
##########
@@ -839,7 +842,7 @@ public void testConversionException() throws Exception {
}
try {
- conf.get(Class.forName("javax.mail.internet.InternetAddress"), "key1");
+ conf.get(Class.forName("jakarta.mail.internet.InternetAddress"), "key1");
Review Comment:
This can simply be a direct class reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r896731746
##########
src/test/java/org/apache/commons/configuration2/TestDataConfiguration.java:
##########
@@ -73,10 +73,13 @@ private static Date expectedDate() throws ParseException {
/**
* Create an instance of InternetAddress. This trick is necessary to compile and run the test with Java 1.3 and the
- * javamail-1.4 which is not compatible with Java 1.3
+ * javamail-1.4 which is not compatible with Java 1.3.
+ *
+ * <p>javamail-2.0 had a namespace change, moving javax.mail.* to jakarta.mail.*. This test verifies if we have
+ * javax.mail.* in the classpath before trying the Jakarta classes.</p>
*/
private Object createInternetAddress(final String email) throws Exception {
- final Class<?> cls = Class.forName("javax.mail.internet.InternetAddress");
+ final Class<?> cls = Class.forName("jakarta.mail.internet.InternetAddress");
Review Comment:
Reuse the constant you created above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r897508540
##########
src/main/java/org/apache/commons/configuration2/convert/PropertyConverter.java:
##########
@@ -607,12 +612,22 @@ static Object toInternetAddress(final Object value) throws ConversionException {
if (value.getClass().getName().equals(INTERNET_ADDRESS_CLASSNAME)) {
return value;
}
+ if (value.getClass().getName().equals(INTERNET_ADDRESS_CLASSNAME_JAKARTA)) {
+ return value;
+ }
if (!(value instanceof String)) {
throw new ConversionException("The value " + value + " can't be converted to a InternetAddress");
}
try {
- final Constructor<?> ctor = Class.forName(INTERNET_ADDRESS_CLASSNAME).getConstructor(String.class);
- return ctor.newInstance(value);
+ try {
Review Comment:
Good point. I suppose the scenario might be like this:
- A user has both mailapi version on the classpath.
- They call `convert` passing in the javax InternetAddress class.
- The converter sees that some type of InternetAddress has been requested and attempts to load the jakarta version, which is found.
- A new jakarta InternetAddress is returned, which causes a class cast exception when being assigned to the javax InternetAddress variable.
I've provided a fix for this in #188, where we only attempt to load and cast to the actual type given. Let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r897504422
##########
src/test/java/org/apache/commons/configuration2/TestDataConfiguration.java:
##########
@@ -73,10 +73,13 @@ private static Date expectedDate() throws ParseException {
/**
* Create an instance of InternetAddress. This trick is necessary to compile and run the test with Java 1.3 and the
- * javamail-1.4 which is not compatible with Java 1.3
+ * javamail-1.4 which is not compatible with Java 1.3.
+ *
+ * <p>javamail-2.0 had a namespace change, moving javax.mail.* to jakarta.mail.*. This test verifies if we have
+ * javax.mail.* in the classpath before trying the Jakarta classes.</p>
*/
private Object createInternetAddress(final String email) throws Exception {
- final Class<?> cls = Class.forName("javax.mail.internet.InternetAddress");
+ final Class<?> cls = Class.forName("jakarta.mail.internet.InternetAddress");
Review Comment:
This entire method was actually not needed anymore. There is no longer any reason to not instantiate `InternetAddress` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r commented on PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#issuecomment-1155952801
@garydgregory, thanks for the feedback! Please take a look at #188 and let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] JasmineXieCCD commented on pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by "JasmineXieCCD (via GitHub)" <gi...@apache.org>.
JasmineXieCCD commented on PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#issuecomment-1420136277
May I know what's the new release version and target date to support jakarta namespace change for servlet-api (javax.servlet)
Such as File:
https://github.com/apache/commons-configuration/blob/master/src/main/java/org/apache/commons/configuration2/web/ServletConfiguration.java
https://github.com/apache/commons-configuration/blob/master/src/main/java/org/apache/commons/configuration2/web/ServletContextConfiguration.java
https://github.com/apache/commons-configuration/blob/master/src/main/java/org/apache/commons/configuration2/web/ServletFilterConfiguration.java
https://github.com/apache/commons-configuration/blob/master/src/main/java/org/apache/commons/configuration2/web/ServletRequestConfiguration.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r896733283
##########
src/main/java/org/apache/commons/configuration2/convert/PropertyConverter.java:
##########
@@ -607,12 +612,22 @@ static Object toInternetAddress(final Object value) throws ConversionException {
if (value.getClass().getName().equals(INTERNET_ADDRESS_CLASSNAME)) {
return value;
}
+ if (value.getClass().getName().equals(INTERNET_ADDRESS_CLASSNAME_JAKARTA)) {
+ return value;
+ }
if (!(value instanceof String)) {
throw new ConversionException("The value " + value + " can't be converted to a InternetAddress");
}
try {
- final Constructor<?> ctor = Class.forName(INTERNET_ADDRESS_CLASSNAME).getConstructor(String.class);
- return ctor.newInstance(value);
+ try {
Review Comment:
So this WILL cause a class cast exception? How is that ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r merged pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r merged PR #186:
URL: https://github.com/apache/commons-configuration/pull/186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r897504090
##########
src/main/java/org/apache/commons/configuration2/convert/PropertyConverter.java:
##########
@@ -70,6 +70,9 @@ public final class PropertyConverter {
/** The fully qualified name of {@code javax.mail.internet.InternetAddress} */
private static final String INTERNET_ADDRESS_CLASSNAME = "javax.mail.internet.InternetAddress";
+ /** The fully qualified name of {@code jakarta.mail.internet.InternetAddress} */
Review Comment:
Good idea. See #188
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r896730985
##########
src/main/java/org/apache/commons/configuration2/convert/PropertyConverter.java:
##########
@@ -70,6 +70,9 @@ public final class PropertyConverter {
/** The fully qualified name of {@code javax.mail.internet.InternetAddress} */
private static final String INTERNET_ADDRESS_CLASSNAME = "javax.mail.internet.InternetAddress";
+ /** The fully qualified name of {@code jakarta.mail.internet.InternetAddress} */
Review Comment:
Maybe rename the other constant to have a JAVAX postfix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on a diff in pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#discussion_r896732187
##########
src/test/java/org/apache/commons/configuration2/TestDataConfiguration.java:
##########
@@ -839,7 +842,7 @@ public void testConversionException() throws Exception {
}
try {
- conf.get(Class.forName("javax.mail.internet.InternetAddress"), "key1");
+ conf.get(Class.forName("jakarta.mail.internet.InternetAddress"), "key1");
Review Comment:
Another magic string, reuse the constant...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by GitBox <gi...@apache.org>.
darkma773r commented on PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#issuecomment-1154653481
> (pending CI build)
>
> Thanks!
Thanks, @kinow! I think you win the award for fastest-ever approval of a PR :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on pull request #186: [CONFIGURATION-813]: Support both javax and jakarta namespace in mailapi
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #186:
URL: https://github.com/apache/commons-configuration/pull/186#issuecomment-1420720289
There is no set plan ATM, we need to consider if this change requires a major version change and all that goes along with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org