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

Dubious use of String.toUpperCase()

    Details

    • Type: Bug
    • Status: Reopened
    • Priority: Major
    • Resolution: Unresolved
    • Component/s: core
    • Labels:
      None
    • Similar Issues:

      Description

      StringParameterValue.buildEnvVars says

      env.put(name.toUpperCase(),value);

      Say the user specified 'includedTests' in the job configuration and used an Ant script:

      <property env="env."/>
      <junit ... includes="$

      {env.includedTests}

      "/>

      First of all, this will not work because only $

      {env.INCLUDEDTESTS}

      will be defined. It is nice that name.html says this will happen, but you would have to click the help button to see this unexpected behavior.

      Second, if you happen to run the Hudson server in Turkey, even that won't work because the environment variable will be $

      {env.─░NCLUDEDTESTS}

      due to the locale-sensitive upcasing!

      At a minimum, all usages of String.toUpper/LowerCase() in Hudson should be reviewed to see if passing Locale.ENGLISH would be more appropriate.

      In the case of *ParameterValue.buildEnvVars, it should be reconsidered whether automatic upcasing is even desirable; surely it is clearer to use an uppercase variable name in the job config if that is what you want to see during the build. (For compatibility, you could set both the upcased string and the original string, and remove the note about upcasing from the help text.)

        Attachments

          Issue Links

            Activity

            Hide
            jglick Jesse Glick added a comment -

            Hence my suggestion that "for compatibility, you could set both the upcased string and the original string".

            Given the new usage of EnvVars (and that its expand method should be case-insensitive already), probably this would be accomplished where ProcessBuilder.environment() is populated from EnvVars - in Launcher and Proc and perhaps elsewhere.

            Show
            jglick Jesse Glick added a comment - Hence my suggestion that "for compatibility, you could set both the upcased string and the original string". Given the new usage of EnvVars (and that its expand method should be case-insensitive already), probably this would be accomplished where ProcessBuilder.environment() is populated from EnvVars - in Launcher and Proc and perhaps elsewhere.
            Hide
            mcrooney mcrooney added a comment -

            Still seems to be broken on 1.345, I have a failing parameterized build and doing an 'env' in a build step shows only the lowercase versions defined.

            Show
            mcrooney mcrooney added a comment - Still seems to be broken on 1.345, I have a failing parameterized build and doing an 'env' in a build step shows only the lowercase versions defined.
            Hide
            vkodocha vkodocha added a comment -

            I've created the issue 5843 which is basically a duplicate of this here.

            I've seen that the backwards compatibility should be solved since version 1.345. And I also looked at the code of the current version. The code looks fine but it does not work for me.

            I have tested this with a clean install of Hudson 1.351, created a new job with one parameter with a lowercase name, and one build step to show all environment vars. I tested this on Mac OS X 10.6 and on a Vista machine with the same result for both platforms. Only the lowercase variable is set, the uppercase variant is just missing.

            On windows it seams not to be working as the environments vars seams to be ignoring case. Meaning a set hello=5 and then a set HELLO=10 will result in a variable named hello=10 to be set. But on mac this is not the case so I don't know why it is not working.
            (I don't have a problem with it not woking under Windows as I use Mac but I just wanted to verify that it's not a mac specific problem).

            Show
            vkodocha vkodocha added a comment - I've created the issue 5843 which is basically a duplicate of this here. I've seen that the backwards compatibility should be solved since version 1.345. And I also looked at the code of the current version. The code looks fine but it does not work for me. I have tested this with a clean install of Hudson 1.351, created a new job with one parameter with a lowercase name, and one build step to show all environment vars. I tested this on Mac OS X 10.6 and on a Vista machine with the same result for both platforms. Only the lowercase variable is set, the uppercase variant is just missing. On windows it seams not to be working as the environments vars seams to be ignoring case. Meaning a set hello=5 and then a set HELLO=10 will result in a variable named hello=10 to be set. But on mac this is not the case so I don't know why it is not working. (I don't have a problem with it not woking under Windows as I use Mac but I just wanted to verify that it's not a mac specific problem).
            Hide
            slide_o_mix Alex Earl added a comment -

            Is this still an issue?

            Show
            slide_o_mix Alex Earl added a comment - Is this still an issue?
            Hide
            danielbeck Daniel Beck added a comment -

            In a different way, yes:
            https://github.com/jenkinsci/jenkins/blob/d8af38b7e3367f3988ceb1d9e15b2fed4efd7971/core/src/main/java/hudson/model/StringParameterValue.java#L55...L57

            It simply defines both.

            Which is probably hilarious on the case of parameter names such as "User" or "Home".

            Related insanity is JENKINS-16255.

            Show
            danielbeck Daniel Beck added a comment - In a different way, yes: https://github.com/jenkinsci/jenkins/blob/d8af38b7e3367f3988ceb1d9e15b2fed4efd7971/core/src/main/java/hudson/model/StringParameterValue.java#L55...L57 It simply defines both. Which is probably hilarious on the case of parameter names such as "User" or "Home". Related insanity is JENKINS-16255 .

              People

              • Assignee:
                Unassigned
                Reporter:
                jglick Jesse Glick
              • Votes:
                2 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated: