Provide access to Guice's own internal TypeConverters

[GoogleCodeExporter]

Guice lets you provide your own type converters using:

  Binder.convertToTypes(matcher, typeConverter);

Original issue reported on code.google.com by mccu...@gmail.com on 10 Oct 2009 at 3:28

[GoogleCodeExporter]

  (whoops, accidentally hit return!)

OK, so Guice lets you provide your own type converters using:

  Binder.convertToTypes(matcher, typeConverter);

but there's no way to get access to Guice's own type converters for primitive 
types.
Having access to these primitive type converters would make it much easier to 
write
composite-style type converters (think of XStream).

Original comment by mccu...@gmail.com on 10 Oct 2009 at 3:35

• Changed title: Provide access to Guice's own internal TypeConverters

[GoogleCodeExporter]

If you'd like to submit a patch that adds these constants to the interface, 
that would 
be quite reasonable:
  public static final TypeConverter BOOLEAN_CONVERTER;
  public static final TypeConverter SHORT_CONVERTER;
  public static final TypeConverter INTEGER_CONVERTER;
  public static final TypeConverter LONG_CONVERTER;
  public static final TypeConverter FLOAT_CONVERTER;
  public static final TypeConverter DOUBLE_CONVERTER;
  public static final TypeConverter CHARACTER_CONVERTER;
  public static final TypeConverter STRING_CONVERTER;
  public static final TypeConverter ENUM_CONVERTER;
  public static final TypeConverter CLASS_CONVERTER;
You'll need to expose them from TypeConverterBindingProcessor, which should be 
just fine. Or move them.

Note that Guice's type conversion facility isn't really general-purpose; I 
suspect our 
performance will be quite horrible if you exceed say, a thousand convertible 
types 
(because we always ask all converters whether they can convert a value).

Original comment by limpbizkit on 10 Oct 2009 at 4:37

[GoogleCodeExporter]

OK, will try out a few options and attach the best patch. The approach I'm 
using has
a handful of type converters and I combine them to cover a much wider range of 
types,
so performance should be OK (but I'll watch out for any bottlenecks).

Original comment by mccu...@gmail.com on 12 Oct 2009 at 4:08

[GoogleCodeExporter]

I took a slightly different approach, and added a "convertConstant" method to 
the
injector. This is simpler than exposing the individual primitive type 
converters.

I've attached a proposed patch of the various changes, but need to add unit 
tests.

Original comment by mccu...@gmail.com on 12 Oct 2009 at 10:34

Attachments:

• GUICE_ISSUE_436_20091012.txt

[GoogleCodeExporter]

I gotta reject this patch because it overreaches the Guice injector to become a 
general 
type conversion tool. We actually have a full type converter framework that's 
built 
upon Guice, but it's currently Google proprietary. Like a solid framework, that 
converter architecture is fully typesafe (it doesn't return Object instances), 
it supports 
lots of target types (protobuffers, possibly DOM elements), plus polymorphism 
and 
composition.

I'd like to avoid supporting weak converters when I know that it's insufficient 
for 
general purpose use. 

That said, there is a market for a general purpose type converter framework. 
I'm not 
sure whether ours is worthy of open sourcing, or if we have the manpower to do 
so.

Original comment by limpbizkit on 12 Oct 2009 at 11:06

• Changed state: Invalid

[GoogleCodeExporter]

> I gotta reject this patch because it overreaches the Guice injector to become
> a general type conversion tool.

That's disappointing, without this patch I'll have to duplicate functionality 
already
contained inside Guice because it's not available via an external API.
(I'm working on replacing Plexus type conversion with a Guice-driven solution.)

BTW, here's how I came up with this patch...

Your original suggestion was to simply expose the ~10 internal type converter
implementations as constant objects on the API. IMHO this would lead to tight
coupling and make it harder to change the set of converters provided by Guice.

I considered adding an API/SPI to list/query the installed set of type 
converters
including the internal set (like you can do with all bindings, incl. 
just-in-time)
but clients would still be using the raw TypeConverter API which is not 
type-safe.
They would also be duplicating the lookup+checking already done by the Injector.

Therefore I decided to provide a type-safe "convertConstant" method, which 
linked the
required type and return value. The injector already has code to find the right
converter and check the converted value is of the right type, so this is really 
just
a bit of refactoring and a new method to allow clients to request the same sort 
of
conversions as done by the injector itself.

> We actually have a full type converter framework that's built upon Guice, but
> it's currently Google proprietary. Like a solid framework, that converter
> architecture is fully typesafe (it doesn't return Object instances), it
> supports lots of target types (protobuffers, possibly DOM elements), plus
> polymorphism and composition.

Hehe, knowing there's a great proprietary system out there isn't much comfort ;)

> I'd like to avoid supporting weak converters when I know that it's 
insufficient
> for general purpose use.

Sure, but this is really just allowing clients to apply the same conversions 
that
Guice applies internally to satisfy constant bindings. We're not adding 
additional
logic to the Injector, merely exposing the constant conversion feature to 
clients.

> That said, there is a market for a general purpose type converter framework.
> I'm not sure whether ours is worthy of open sourcing, or if we have the 
manpower
> to do so.

Unfortunately I'm on a tight schedule. We were hoping to replace Plexus 
completely
with Guice, but it looks like we may have to keep the Plexus type conversion 
system
around, or swap in something like XStream. Then again I could do a simple 
copy-paste
of the bits I need (although I hate duplicating code) or maintain a patched 
build of
Guice for our own use (which I also hate doing).

Oh well, back to the drawing board...

Original comment by mccu...@gmail.com on 13 Oct 2009 at 5:04

[GoogleCodeExporter]

FYI, decided to use a locally patched build for now as this feature also lets 
us use
type converters contributed by users. Without this patch they would need to 
register
their converters first with Guice (for the injector) and second with our 
extension.

Original comment by mccu...@gmail.com on 15 Oct 2009 at 3:11

[GoogleCodeExporter]

Would it work if Injector had an SPI method to get the full set of registered 
type 
converters? I don't mind doing that:
  public Set<TypeConverterBinding> getTypeConverterBindings()

Original comment by limpbizkit on 15 Oct 2009 at 3:55

[GoogleCodeExporter]

Sure, that would be acceptable. I thought providing a "convertConstant" method 
would
be less disruptive as it's just exposing existing internal functionality. But 
an SPI
method is probably the more correct solution, as you can already query other 
bindings.

Original comment by mccu...@gmail.com on 15 Oct 2009 at 4:04

[GoogleCodeExporter]

I'll try to whip up a patch tonight to add getTypeConverterBindings()

Original comment by mccu...@gmail.com on 15 Oct 2009 at 5:05

• Changed state: Accepted

[GoogleCodeExporter]

Hmm... looks like adding a "getTypeConverterBindings()" method will be a much 
larger
patch because TypeConverterBinding has a package-private constructor. This 
means we
would either have to maintain two separate lists in State (one 
TypeConverterBindings
from external modules and one internal MatcherAndConverters) or find some way 
to get
the internal converters into TypeConverterBinding elements.

Any suggestions? My gut still suggests the "convertConstant" approach is 
safer...

Original comment by mccu...@gmail.com on 15 Oct 2009 at 6:04

[GoogleCodeExporter]

I'd like to expose the TypeConverterBinding, even if we need to make its 
constructor 
more public. I don't mind if that requires us to create TypeConverterBindings 
for the 
built-in converters.

Another benefit of this approach is that we could get ConvertedConstantBinding 
to 
expose the converter that they used.

Original comment by limpbizkit on 16 Oct 2009 at 1:48

[GoogleCodeExporter]

New patch that adds getTypeConverterBindings() to the injector and exposes the 
type
converter used in ConvertedConstantBinding. I also added javax.inject and the 
TCK to
the test launcher in build.xml, as this is needed to run tests on the command 
line.

Couple of points about the patch:

1) used List instead of Set to match other existing methods on the injector.

2) converted TypeConverterBinding to an interface, which is then implemented 
inside
the internal package. This should maintain compatibility with clients while 
limiting
access to the constructor.

3) made sure getScopeBindings() and getTypeConverterBindings() return 
unmodifiable
collections.

Original comment by mccu...@gmail.com on 16 Oct 2009 at 8:53

Attachments:

• GUICE_ISSUE_436_20091016.txt

[GoogleCodeExporter]

Passing issue to Jesse for review, latest patch works fine for my use-case.

Original comment by mccu...@gmail.com on 28 Oct 2009 at 10:35

• Added labels: Milestone-Release2.1

[GoogleCodeExporter]

Simplified patch that just makes the TypeConverterBinding constructor public 
(better
binary compatibility).

Original comment by mccu...@gmail.com on 10 Mar 2010 at 8:00

Attachments:

• GUICE_ISSUE_436_20100309.txt

[GoogleCodeExporter]

Description
===========

This patch (GUICE_ISSUE_436_20100504.patch) exposes a new method in the 
Injector:

   List<TypeConverterBinding> getTypeConverterBindings();

which returns the list of type converters registered with the injector, 
including
internal converters registered by Guice. It also adds a method to the converted
constant binding in the SPI:

   TypeConverterBinding getTypeConverterBinding();

which lets you find out which type converter was used to convert that constant.

Implementation
==============

This patch makes TypeConverterBinding public and uses this as a replacement for 
the
internal MatcherAndConverter (which was basically a clone of 
TypeConverterBinding).
It also contains a NPE fix to the Errors.appliesTo() method and makes sure that 
the
collection views returned from the injector are immutable.

Original comment by mccu...@gmail.com on 4 May 2010 at 8:23

Attachments:

• GUICE_ISSUE_436_20100504.patch

[GoogleCodeExporter]

Original comment by mccu...@gmail.com on 4 May 2010 at 11:27

• Added labels: Type-Patch
• Removed labels: Type-Enhancement

[GoogleCodeExporter]

Any chance this feature could go into 3.0? We've found it useful at Sonatype.

Original comment by mccu...@gmail.com on 1 Jul 2010 at 3:42

• Added labels: 3.0
• Removed labels: Milestone-Release2.1

[GoogleCodeExporter]

Any update on if/when this patch will make it into Guice 3?

Original comment by mccu...@gmail.com on 17 Nov 2010 at 11:31

[GoogleCodeExporter]

Updated patch that can be cleanly applied against latest trunk

Original comment by mccu...@gmail.com on 17 Nov 2010 at 11:40

Attachments:

• GUICE_ISSUE_436_20101117.patch

[GoogleCodeExporter]

I'm personally inclined to leave this particular one out of guice3, primarily 
because it exposes more of the internal API.

Original comment by sberlin on 18 Nov 2010 at 4:02

[GoogleCodeExporter]

Where does it expose the internal API? The intent of this patch is to extend 
the SPI to allow clients to introspect and discover what converters are 
registered with the injector and find out which converter converted a 
particular value (both valuable features if you want to know what's going on 
wrt. value conversion).

Original comment by mccu...@gmail.com on 18 Nov 2010 at 11:09

[GoogleCodeExporter]

Mainly in that it exposes a TypeConverterBinding instance, but looking up in 
the comments it appears Jesse's OK with that.  I'll try to grab his ear to talk 
to him.

Also, a question: The patch changes Errors.Converter.appliesTo to add a null 
check for 'o', which seems to be used in the Errors.convert method.  When would 
someone want to convert 'null'?  Why would we want to ignore it (as a potential 
source of mistake/failure)?  I didn't see any obvious places looking in the 
caller where null was acceptable.

Original comment by sberlin on 18 Nov 2010 at 1:03

[GoogleCodeExporter]

You'll see the NPE issue whenever you report a custom error message that 
includes a null parameter.

For example:

   String value = null;
   //...
   binder.addError("Some error, value=%s", value);

Won't actually add the error message to the final output, but instead you'll 
see an NPE stack trace below it:

Caused by: java.lang.NullPointerException
    at com.google.inject.internal.Errors$Converter.appliesTo(Errors.java:612)
    at com.google.inject.internal.Errors.convert(Errors.java:646)
    at com.google.inject.internal.Errors.format(Errors.java:503)
    at com.google.inject.spi.Elements$RecordingBinder.addError(Elements.java:241)
    at com.google.inject.AbstractModule.addError(AbstractModule.java:125)

which then hides the original message.

It should be possible to use null values in error messages hence the null check.

[ this is also related to Issue 432: 
http://code.google.com/p/google-guice/issues/detail?id=432 ]

Original comment by mccu...@gmail.com on 18 Nov 2010 at 2:30

[GoogleCodeExporter]

I'm okay with the proposed change but like Sam I'm slightly anxious about the 
change in behaviour around errors.

I also think Set is the right type rather than List, because order is not 
significant and because each type converter can exist only once.

Original comment by limpbizkit on 18 Nov 2010 at 4:28

[GoogleCodeExporter]

Wrt. the Errors change - what if I (as a user) want to add a binder error 
message that contains a potentially null message parameter? ie. like 
binder.addError("Some error, value=%s", value); where value might possibly be 
null.

This is not an unusual situation, but at the moment this will cause an NPE 
inside the format conversion code in Guice, wiping out the original error 
message and replacing it with a misleading stack trace that suggests the 
problem lies inside Guice rather than the true cause.

This is also inconsistent with the javadoc for Binder.addError() which states 
that String.format() will be used to format the message, and String.format() 
can handle null message parameters whereas currently Errors cannot.

Original comment by mccu...@gmail.com on 18 Nov 2010 at 4:37

[GoogleCodeExporter]

Wrt. List vs Set - I'm fine with it using Set

Original comment by mccu...@gmail.com on 18 Nov 2010 at 4:41

[GoogleCodeExporter]

@mcculls - null in that scenario makes sense to me. Sounds like we're good!

Original comment by limpbizkit on 19 Nov 2010 at 2:14

[GoogleCodeExporter]

SGTM, will patch this in...

Original comment by sberlin on 19 Nov 2010 at 2:20

[GoogleCodeExporter]

committed in r1376 & minor followup in r1377.  thanks very much, Stuart!

Original comment by sberlin on 19 Nov 2010 at 2:40

• Changed state: Fixed

[GoogleCodeExporter]

Excellent, thanks!

Original comment by mccu...@gmail.com on 19 Nov 2010 at 5:37