Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-25498

Inappropriate use of GString

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      Tried to file this under an icon-shim-plugin component but could not find one in JIRA.

      At any rate, IconSet has overloads taking Groovy GString. This kind of thing should not appear in an API. Take only a String, or if you must, accept an Object and call toString (if not null).

        Attachments

          Issue Links

            Activity

            Hide
            tfennelly Tom FENNELLY added a comment -

            Yeah, this is horrible and would be more than happy to get rid of it if possible. Please advise.

            KK was asking me about this some time ago too. Here's the conversation...

            I'll check is it still needed. At the time I added it though it was definitely needed - I didn't add it for fun.

            From memory, I think it was down to JEXL (I may be wrong - it was a while ago). I think it was needed in situations where the 'abc' variable you talk about in your example was the output of an earlier JEXL expression that created a GString object instance in the jelly context used to evaluate the expressions. Then later, when ${icons.getIconByUrl(abc)} is being evaluated, JEXL was trying to find an Icons.getIconByUrl(GString) method (because abc is of type GString) but was failing.

            As I say... this is from memory so I may be slightly wrong on the exact details, but I don't think I'm too far off and I certainly did not add it randomly. That said, it may be that this is no longer needed because I do remember having some trouble implementing the tag library in that plugin, which resulted in me implementing it a few different ways before I was able to get it to work (http://goo.gl/RSbacS). So maybe the GStrings are legacy from one of those attempts.

            On 19/10/2014 06:01, Kohsuke Kawaguchi wrote:
            > I remember what I wanted to ask you.
            >
            > In the following code, you have every method overloaded between String and GString.
            >
            > https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java
            >
            > What is that for? GString is mostly internal implementation details in Groovy, and it'll perform the appropriate type coercison to generate a plain String. So this seems redundant.
            >
            > And worse yet, JEXL (used by Jelly) has a trouble resolving overload when the null is an argument. So for example, invocations like
            >
            > ${icons.getIconByUrl(abc)}
            >
            > results in a problem if 'abc' is null.
            >
            > –
            > Kohsuke Kawaguchi

            Show
            tfennelly Tom FENNELLY added a comment - Yeah, this is horrible and would be more than happy to get rid of it if possible. Please advise. KK was asking me about this some time ago too. Here's the conversation... I'll check is it still needed. At the time I added it though it was definitely needed - I didn't add it for fun. From memory, I think it was down to JEXL (I may be wrong - it was a while ago). I think it was needed in situations where the 'abc' variable you talk about in your example was the output of an earlier JEXL expression that created a GString object instance in the jelly context used to evaluate the expressions. Then later, when ${icons.getIconByUrl(abc)} is being evaluated, JEXL was trying to find an Icons.getIconByUrl(GString) method (because abc is of type GString) but was failing. As I say... this is from memory so I may be slightly wrong on the exact details, but I don't think I'm too far off and I certainly did not add it randomly. That said, it may be that this is no longer needed because I do remember having some trouble implementing the tag library in that plugin, which resulted in me implementing it a few different ways before I was able to get it to work ( http://goo.gl/RSbacS ). So maybe the GStrings are legacy from one of those attempts. On 19/10/2014 06:01, Kohsuke Kawaguchi wrote: > I remember what I wanted to ask you. > > In the following code, you have every method overloaded between String and GString. > > https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java > > What is that for? GString is mostly internal implementation details in Groovy, and it'll perform the appropriate type coercison to generate a plain String. So this seems redundant. > > And worse yet, JEXL (used by Jelly) has a trouble resolving overload when the null is an argument. So for example, invocations like > > ${icons.getIconByUrl(abc)} > > results in a problem if 'abc' is null. > > – > Kohsuke Kawaguchi
            Hide
            tfennelly Tom FENNELLY added a comment -

            True, accepting an Object would be a bit better than GString if it worked.

            Show
            tfennelly Tom FENNELLY added a comment - True, accepting an Object would be a bit better than GString if it worked.
            Hide
            jglick Jesse Glick added a comment -

            Yes please dig up the original justification for this because it seems wrong. It also sounds like KK’s warning anticipated JENKINS-25499 exactly.

            Show
            jglick Jesse Glick added a comment - Yes please dig up the original justification for this because it seems wrong. It also sounds like KK’s warning anticipated JENKINS-25499 exactly.
            Hide
            danielbeck Daniel Beck added a comment - - edited

            Tom: I have a chatlog around somewhere where I asked you about that, maybe that has more detail.

            Update: Unfortunately not.

            [2014-08-30T01:19:33+0200] <+danielbeck> https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java#L88 GString?
            [2014-08-30T01:20:25+0200] <+danielbeck> What do you need Groovy strings for?
            [2014-08-30T01:20:40+0200] <tfennelly> danielbeck: trying to remember
            [2014-08-30T01:21:01+0200] <tfennelly> danielbeck: oh I think it was wrt jexl
            [2014-08-30T01:21:38+0200] <tfennelly> danielbeck: forget exactly but it's needed sometimes... otherwise not resolved

            Show
            danielbeck Daniel Beck added a comment - - edited Tom: I have a chatlog around somewhere where I asked you about that, maybe that has more detail. Update: Unfortunately not. [2014-08-30T01:19:33+0200] <+danielbeck> https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java#L88 GString? [2014-08-30T01:20:25+0200] <+danielbeck> What do you need Groovy strings for? [2014-08-30T01:20:40+0200] <tfennelly> danielbeck: trying to remember [2014-08-30T01:21:01+0200] <tfennelly> danielbeck: oh I think it was wrt jexl [2014-08-30T01:21:38+0200] <tfennelly> danielbeck: forget exactly but it's needed sometimes... otherwise not resolved
            Hide
            tfennelly Tom FENNELLY added a comment -

            Fixed this issue (and JENKINS-25499) in the icon-shim plugin. Updated dependency to new version of same.

            Show
            tfennelly Tom FENNELLY added a comment - Fixed this issue (and JENKINS-25499 ) in the icon-shim plugin. Updated dependency to new version of same.

              People

              Assignee:
              tfennelly Tom FENNELLY
              Reporter:
              jglick Jesse Glick
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: