[Digester] enums in ConvertUtils.register

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[Digester] enums in ConvertUtils.register

Jon Steelman-4
I just tried to register a Java 1.5 enum class, but Digester seems not
to work with it. No exceptions are thrown during
ConvertUtils.register(myConverter, SomeEnum.class) or during a parsing
match of digester.parse. I'm using Digester 1.7

Thanks,
Jon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Jon Steelman-4
So, my question is should ConvertUtils.register(Converter converter,
java.lang.Class clazz) be able to accept and use an enum class as the
clazz parameter?

-----Original Message-----
From: Jon Steelman
Sent: Tuesday, June 14, 2005 8:33 PM
To: [hidden email]
Subject: [Digester] enums in ConvertUtils.register

I just tried to register a Java 1.5 enum class, but Digester seems not
to work with it. No exceptions are thrown during
ConvertUtils.register(myConverter, SomeEnum.class) or during a parsing
match of digester.parse. I'm using Digester 1.7


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [Digester] enums in ConvertUtils.register

Simon Kitching
In reply to this post by Jon Steelman-4
On Tue, 2005-06-14 at 20:32 -0400, Jon Steelman wrote:
> I just tried to register a Java 1.5 enum class, but Digester seems not
> to work with it. No exceptions are thrown during
> ConvertUtils.register(myConverter, SomeEnum.class) or during a parsing
> match of digester.parse. I'm using Digester 1.7

I would hope it works; it seems a reasonable thing to do. However
java1.5 enums are fairly cutting-edge so there may be some issue.

Unfortunately, as far as I am aware, you are the first to try using
ConvertUtils with enums so you're probably going to have to figure out
what the issue is yourself. I would suggest first writing a simple test
using ConvertUtils directly; if that doesn't work then the problem is
not Digester.

Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Jon Steelman-4
In reply to this post by Jon Steelman-4
Okay, my bad. I tested ConvertUtils directly and it works as does
Digester.

However, it brings up the point that ConvertUtils.convert(String, Class)
should automatically handle the case of the Class is an enum by using
the enum's built in valueOf(String) method so that a custom Converter is
not required. Is this the forum to make that suggestion?

Jon

-----Original Message-----
From: Simon Kitching [mailto:[hidden email]]
Sent: Wednesday, June 15, 2005 12:11 AM
To: Jakarta Commons Users List
Subject: Re: [Digester] enums in ConvertUtils.register

On Tue, 2005-06-14 at 20:32 -0400, Jon Steelman wrote:
> I just tried to register a Java 1.5 enum class, but Digester seems not
> to work with it. No exceptions are thrown during
> ConvertUtils.register(myConverter, SomeEnum.class) or during a parsing
> match of digester.parse. I'm using Digester 1.7

I would hope it works; it seems a reasonable thing to do. However
java1.5 enums are fairly cutting-edge so there may be some issue.

Unfortunately, as far as I am aware, you are the first to try using
ConvertUtils with enums so you're probably going to have to figure out
what the issue is yourself. I would suggest first writing a simple test
using ConvertUtils directly; if that doesn't work then the problem is
not Digester.

Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Simon Kitching
On Wed, 2005-06-15 at 03:19 -0400, Jon Steelman wrote:
> Okay, my bad. I tested ConvertUtils directly and it works as does
> Digester.
>
> However, it brings up the point that ConvertUtils.convert(String, Class)
> should automatically handle the case of the Class is an enum by using
> the enum's built in valueOf(String) method so that a custom Converter is
> not required. Is this the forum to make that suggestion?

Yes, this is definitely the right place to discuss things like this.

The problem is that if we add that code to beanutils then beanutils will
no longer work in pre-1.5 jvms. Offhand I can't think of a reasonable
way to support 1.5 enums without breaking earlier java versions. And
having separate versions of beanutils for the two java platforms would
be a major nuisance.

Any ideas how this could be implemented?

Regards,

Simon



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Jon Steelman-4
In reply to this post by Jon Steelman-4
I wrote the following code that works on jdk1.5 *and* I expect to be
backwards compatible to jdk1.1:

public Object checkForEnum(String stringValue, Class clazz) {
    Class superclass = clazz.getSuperclass();
    System.out.println("superClass = " + superclass);
    if (superclass.getName().equals("java.lang.Enum")) {
        // Only makes it here if the jdk is 1.5+ and class is an enum
        Method valueOf = null;
        try {
            valueOf = clazz.getMethod("valueOf",new Class[]
{String.class});
        }
        catch (NoSuchMethodException e) {
            // Should never get here! All enums have this dynamic method
            e.printStackTrace();
        }
        Object enm = null;
        try {
            enm = valueOf.invoke(null,new Object[]{stringValue});
        }
        catch (IllegalAccessException e) {
            // Should never get here as the method is always public.
            e.printStackTrace();
        }
        catch (InvocationTargetException e) {
            // Should never get here as the target object is
                // null thanks to method being static/
            e.printStackTrace();
        }
        // IllegalArgumentException is possible when the
        return enm;
    }
}

Can it be incorporated?

Thanks,
Jon

-----Original Message-----
From: Simon Kitching [mailto:[hidden email]]
Sent: Wednesday, June 15, 2005 3:45 AM
To: Jakarta Commons Users List
Subject: RE: [Digester] enums in ConvertUtils.register

On Wed, 2005-06-15 at 03:19 -0400, Jon Steelman wrote:
> However, it brings up the point that ConvertUtils.convert(String,
Class)
> should automatically handle the case of the Class is an enum by using
> the enum's built in valueOf(String) method so that a custom Converter
is
> not required. Is this the forum to make that suggestion?

Yes, this is definitely the right place to discuss things like this.

The problem is that if we add that code to beanutils then beanutils will
no longer work in pre-1.5 jvms. Offhand I can't think of a reasonable
way to support 1.5 enums without breaking earlier java versions. And
having separate versions of beanutils for the two java platforms would
be a major nuisance.

Any ideas how this could be implemented?

Regards,

Simon



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Simon Kitching
On Wed, 2005-06-15 at 04:30 -0400, Jon Steelman wrote:
> I wrote the following code that works on jdk1.5 *and* I expect to be
> backwards compatible to jdk1.1:

That was quick!

This looks reasonable to me. The reflection is of course slow, but for
non-enum targets, the cost is fairly low.

Could you create a bugzilla enhancement entry for this? I will then try
to commit it within the next few days.

There are a few issues that need to be addressed, though:
* Is clazz always going to be a direct subclass of java.lang.Enum?
  I don't think you can subclass enums, but haven't looked into this.
  And what about when the enum is in a nested class, etc?
* Need to handle the error cases better. As you say, these should
  never occur - but never is a dangerous word in the software
  world :) Actually - can an enum be private? What happens in that case?

I guess I need to learn more about java1.5 enums!

Thanks very much for the patch.

Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Simon Kitching
On Wed, 2005-06-15 at 20:58 +1200, Simon Kitching wrote:

> On Wed, 2005-06-15 at 04:30 -0400, Jon Steelman wrote:
> > I wrote the following code that works on jdk1.5 *and* I expect to be
> > backwards compatible to jdk1.1:
>
> That was quick!
>
> This looks reasonable to me. The reflection is of course slow, but for
> non-enum targets, the cost is fairly low.
>
> Could you create a bugzilla enhancement entry for this? I will then try
> to commit it within the next few days.

Oh - and if you can add a unit test or two that would be appreciated. If
you don't supply one then I will need to write them, and it is your
requested feature...

If you are willing to write unit tests but need help I'm happy to answer
any questions.

Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Jon Steelman-4
In reply to this post by Jon Steelman-4
Right, indirect subclassing of java.lang.Enum is not allowed so any enum
is an end-of-the-line, but I'll triple check that with the spec. I'll
explore the listed issues fully, see if I can come up with any others,
provide some unit testing (is JUnit the preferred way?), and create a
bugzilla enhancement entry. How's that for service? ;-)  As this is my
first week using Digester & BeanUtils, where would this code go between
Digester & BeanUtils and with what class?

Thanks,
Jon

-----Original Message-----
From: Simon Kitching [mailto:[hidden email]]
Sent: Wednesday, June 15, 2005 4:59 AM
To: Jakarta Commons Users List
Subject: RE: [Digester] enums in ConvertUtils.register

On Wed, 2005-06-15 at 04:30 -0400, Jon Steelman wrote:
> I wrote the following code that works on jdk1.5 *and* I expect to be
> backwards compatible to jdk1.1:

That was quick!

This looks reasonable to me. The reflection is of course slow, but for
non-enum targets, the cost is fairly low.

Could you create a bugzilla enhancement entry for this? I will then try
to commit it within the next few days.

There are a few issues that need to be addressed, though:
* Is clazz always going to be a direct subclass of java.lang.Enum?
  I don't think you can subclass enums, but haven't looked into this.
  And what about when the enum is in a nested class, etc?
* Need to handle the error cases better. As you say, these should
  never occur - but never is a dangerous word in the software
  world :) Actually - can an enum be private? What happens in that case?

I guess I need to learn more about java1.5 enums!

Thanks very much for the patch.

Regards,

Simon


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: [Digester] enums in ConvertUtils.register

Simon Kitching
Hi Jon,

On Wed, 2005-06-15 at 08:27 -0400, Jon Steelman wrote:
> Right, indirect subclassing of java.lang.Enum is not allowed so any enum
> is an end-of-the-line, but I'll triple check that with the spec. I'll
> explore the listed issues fully, see if I can come up with any others,
> provide some unit testing (is JUnit the preferred way?), and create a
> bugzilla enhancement entry. How's that for service? ;-)  As this is my
> first week using Digester & BeanUtils, where would this code go between
> Digester & BeanUtils and with what class?

That would certainly get you top service on any future support you need
for beanutils and digester :-). And a listing in the contributors
section if you wish.

This functionality definitely belongs in beanutils. Method
  org.apache.commons.beanutils.ConvertUtilsBean.convert(String, Class)
needs to be updated to check for Enum if the initial lookup of converter
based on target class type fails, and before defaulting to
lookup(String.class).

How about creating

  // dummy class to represent Enum.class; we can't use Enum.class
  // directly as that won't work in pre-1.5
  public class EnumClass {}

and

  public class EnumConverter implements Converter {
    ... the reflection stuff
  }

then updating ConvertUtilsBean.deregister() to add an EnumConverter to
the list of converters by default:

  register(EnumClass.class, new EnumConverter())

then in convert(String, class)

  converter = lookup(clazz)
  if (converter == null) {
    if (clazz is enum) {
      converter = lookup(EnumClass.class);
    }
  }
  if converter == null) {
    converter = lookup(String.class)
  }
  return converter.convert(clazz, value);

Note that ConvertUtilsBean.deregister() is poorly named; it actually
sets the set of registered converters back to the default set.

I don't think we can get away without the hack in method convert, as we
have to test the superclass of the specified class which isn't normal
for convertutils. But this way everything else follows the existing
style. And users can delete or replace the EnumConverter in the
converter list if they wish.

I can't see any backwards compatibility issues here; any custom enum
converters the user has registered would continue to work because we try
a lookup on the specific enum class first. The only time the new code
would be invoked is when the conversion would have produced a String
instead of the requested Enum, and that surely would have produced an
exception anyway. So I think we're ok on that aspect.

Possibly the deregister() method could avoid adding an EnumConverter to
the list at all on pre-1.5 jvms, but I don't see it doing any harm.

And the EnumConverter can throw ConversionException objects when any of
those reflection issues arise which is a nice and tidy solution.

Of course the above is just a suggestion; if you can see a better
approach please say so - or demonstrate the better solution in your
patch.

==

For tests, junit is what all Jakarta projects use; running "maven test"
in the main beanutils directory will run all the junit tests.

You probably just need to add one or two test methods to existing class
src/test/org/apache/commons/beanutils/ConvertUtilsTestCase.java


Good luck!

Simon



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]