Spring Framework
  1. Spring Framework
  2. SPR-2539

Patch which adds support for using constructor parameter names with constructor injection

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Core
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      This patch adds support for using parameter names when using constructor injection. With this patch the following will be possible:

      <bean id="connectionPool" class="ConnectionPoolImpl">
      <constructor-arg name="host" value="gbg5"/>
      <constructor-arg name="port" value="3066"/>
      <constructor-arg name="username" value="bob"/>
      <constructor-arg name="password" value="Ns7Ysh"/>
      </bean>

      provided that ConnectionPoolImpl has the following constructor:

      ConnectionPoolImpl(String host, String port, String username, String password)

      The patch also work with static and instance factory methods.

      The patch uses ASM to read the local variable debugging information from the .class file of the class being created. This leads to some obvious limitations: the class has to be compiled with local variable debug info and the .class file must be readable from the class path. Even with these limitations I think it would be a nice addition to Spring.

      Have a look at my blog for a more detailed description of the technique and why I'd like Spring to support this: http://therning.org/niklas/node/164

      1. named-constructor-args.diff
        52 kB
        Niklas Therning
      2. named-constructor-args2.diff
        25 kB
        Niklas Therning

        Activity

        Hide
        Niklas Therning added a comment -

        The patch is against the current HEAD (2006-09-05).

        Show
        Niklas Therning added a comment - The patch is against the current HEAD (2006-09-05).
        Hide
        Chris Wood added a comment -

        This does seem slightly dangerous to me. Your app falls over as soon as you compile without debug information. I can easily see people spending hours trying to find the problem.

        I've got a good suggestion for resolving this. If upon startup the by-name setter discovers arguments specified out of order with respect to the order they are defined in the constructor (or with respect the the explicitly declared arg index), it fails with an error. That way the change would still work even without debug information present.

        This is even safer than the original, since the name acts as a sanity check on the argument specification.

        Show
        Chris Wood added a comment - This does seem slightly dangerous to me. Your app falls over as soon as you compile without debug information. I can easily see people spending hours trying to find the problem. I've got a good suggestion for resolving this. If upon startup the by-name setter discovers arguments specified out of order with respect to the order they are defined in the constructor (or with respect the the explicitly declared arg index), it fails with an error. That way the change would still work even without debug information present. This is even safer than the original, since the name acts as a sanity check on the argument specification.
        Hide
        Niklas Therning added a comment -

        I see your point. You will get an error message which says:

        "Could not find constructor parameter names for class java.lang.String. Constructor and method parameter names can only be determined for classes compiled with local variable debug information (javac -g or javac -g:vars)."

        So if people actually read the error message I don't think they will have to spend hours to track down the problem. Anyway, what you suggest seems like a good idea. I'll have to think about it, what would have to be changed in the patch. Thanks for your comment!

        Show
        Niklas Therning added a comment - I see your point. You will get an error message which says: "Could not find constructor parameter names for class java.lang.String. Constructor and method parameter names can only be determined for classes compiled with local variable debug information (javac -g or javac -g:vars)." So if people actually read the error message I don't think they will have to spend hours to track down the problem. Anyway, what you suggest seems like a good idea. I'll have to think about it, what would have to be changed in the patch. Thanks for your comment!
        Hide
        Colin Yates added a comment -

        I suppose the nirvana is that we can do something like:

        <bean id=".." class="...">
        <firstParam>sdfsdf</firstParam>
        <secondParam>sdfsdf</secondParam>
        </bean>

        which would determine the appropriate combination of constructors/setters to call. Of course the <property and <constructor-arg should still be able to be used if needed.

        Just my thoughts

        Show
        Colin Yates added a comment - I suppose the nirvana is that we can do something like: <bean id=".." class="..."> <firstParam>sdfsdf</firstParam> <secondParam>sdfsdf</secondParam> </bean> which would determine the appropriate combination of constructors/setters to call. Of course the <property and <constructor-arg should still be able to be used if needed. Just my thoughts
        Hide
        Eugene Kuleshov added a comment -

        BTW, there is LocalVariableTableParameterNameDiscover. Though it is not being used for contructor-args...

        Show
        Eugene Kuleshov added a comment - BTW, there is LocalVariableTableParameterNameDiscover. Though it is not being used for contructor-args...
        Hide
        Niklas Therning added a comment -

        Ok, thanks for letting me know. I didn't know that. I'll change my patch to use that instead. LocalVariableTableParameterNameDiscover doesn't seem to work with static methods (it seems to always skip the first parameter name which is 'this' for instance methods) so I guess I'll have to fix that too.

        Show
        Niklas Therning added a comment - Ok, thanks for letting me know. I didn't know that. I'll change my patch to use that instead. LocalVariableTableParameterNameDiscover doesn't seem to work with static methods (it seems to always skip the first parameter name which is 'this' for instance methods) so I guess I'll have to fix that too.
        Hide
        Niklas Therning added a comment -

        Uploading a new patch which uses LocalVariableTableParameterNameDiscover instead of my own ParameterNamesExtractor as used in the first patch. This new patch is against HEAD as of today. This new patch replaces the old one so please ignore the named-constructor-args.diff file.

        Please note that this patch won't work unless SPR-2556 is fixed first. The reason is that LocalVariableTableParameterNameDiscover cannot handle static methods and methods with long and doubles as args. SPR-2556 addresses these issues.

        Show
        Niklas Therning added a comment - Uploading a new patch which uses LocalVariableTableParameterNameDiscover instead of my own ParameterNamesExtractor as used in the first patch. This new patch is against HEAD as of today. This new patch replaces the old one so please ignore the named-constructor-args.diff file. Please note that this patch won't work unless SPR-2556 is fixed first. The reason is that LocalVariableTableParameterNameDiscover cannot handle static methods and methods with long and doubles as args. SPR-2556 addresses these issues.
        Hide
        Chris Beams added a comment -

        This has since been implemented. See the name attribute of the <constructor-arg> element.

        Show
        Chris Beams added a comment - This has since been implemented. See the name attribute of the <constructor-arg> element.

          People

          • Assignee:
            Chris Beams
            Reporter:
            Niklas Therning
            Last updater:
            Trevor Marshall
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              1 year, 44 weeks, 1 day ago