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

Groovy-backed cascade selects lose sorting/option order after 2.5 upgrade

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed (View Workflow)
    • Priority: Major
    • Resolution: Fixed
    • Component/s: active-choices-plugin
    • Labels:
      None
    • Environment:
      Jenkins 2.249.2 LTS using official Docker image; uno-choice 2.5
    • Similar Issues:

      Description

      We have quite a few jobs that use CascadeChoiceParameter PT_SINGLE_SELECT, getting their values from a GroovyScript secureScript running in the sandbox. The Groovy script returns a Map of strings.

      We've been using the same code for many years (since at least version 1.3 in July 2017; we didn't track our plugin versions before then), and relying on the fact that choices (options in the HTML) are displayed in the order that they're added to the Map. Essentially, we loop over some objects in sorted order and add them to the Map, where neither the map key nor the map value is the sort key.

      From uno-choice 1.3 until 2.4, options were displayed in the selects in the order they're added to the map. We upgraded to uno-choice 2.5 yesterday, and now the options seem to be displaying in what looks like random order. This has completely broken our use case, as we have select parameters with hundreds of items that are only useful to humans when the options are properly sorted.

      Example use case:
      We have an array of objects, where each object has properties "Name", "Id", and "Date". We loop over the objects sorted by Date, and add them to a map with Id as the key and Name as the value. Up to and including 2.4, this resulted in a select with options that displayed Name, had "Id" as their value, but were sorted by Date.

        Attachments

          Activity

          Hide
          kinow Bruno P. Kinoshita added a comment -

          Hi Jason Antman

          That sounds very strange. Our main change in 2.5 are security updates. I realized I forgot to add the 2.5 changelog, but I've just searched git log and pushed here: https://github.com/jenkinsci/active-choices-plugin/#version-25-20201013

          Thanks for the great description. From what you said, however, I can't really imagine where the issue could be coming from (assuming it's after 2.4). Here are some possibilities I guess:

          • Our security fixes may be iterating/changing order of the parameters
          • Another change in Jenkins is affecting the parameters
          • Another plug-in when used with Active Choices is causing the issue
          • There's something special about our use case that triggered it now (very unlikely IMO)
          • A new version of Groovy or the JVM, or maybe high memory pressure could be changing the order (we use a hashmap somewhere I think, I can't recall exactly its API contract, but I think the order is not always guaranteed?)

          These are just some random thoughts. It could well be something else

          What would help more, is if you could, perhaps:

          • Grab a copy of jenkins.war from jenkins.io
          • Install the plug-in in this new version, with the minimum of plug-ins possible
          • Create a small job to reproduce the issue
          • Share your Jenkinsfile and/or config.xml

          I think with that I should be able to reproduce the issue, and debug in my IDE.

          Bruno

          Show
          kinow Bruno P. Kinoshita added a comment - Hi Jason Antman That sounds very strange. Our main change in 2.5 are security updates. I realized I forgot to add the 2.5 changelog, but I've just searched git log and pushed here: https://github.com/jenkinsci/active-choices-plugin/#version-25-20201013 Thanks for the great description. From what you said, however, I can't really imagine where the issue could be coming from (assuming it's after 2.4). Here are some possibilities I guess: Our security fixes may be iterating/changing order of the parameters Another change in Jenkins is affecting the parameters Another plug-in when used with Active Choices is causing the issue There's something special about our use case that triggered it now (very unlikely IMO) A new version of Groovy or the JVM, or maybe high memory pressure could be changing the order (we use a hashmap somewhere I think, I can't recall exactly its API contract, but I think the order is not always guaranteed?) These are just some random thoughts. It could well be something else What would help more, is if you could, perhaps: Grab a copy of jenkins.war from jenkins.io Install the plug-in in this new version, with the minimum of plug-ins possible Create a small job to reproduce the issue Share your Jenkinsfile and/or config.xml I think with that I should be able to reproduce the issue, and debug in my IDE. Bruno
          Hide
          jantman Jason Antman added a comment -

          Bruno P. Kinoshita Thank you so much for the quick reply. I've reproduced this issue with a minimal job and minimal plugins using the WAR file.

          1. Run Jenkins 2.2.49.2 (latest LTS) from WAR. Install plugins as follows (uno-choice installed via HPI download):

          uno-choice: 2.4
          trilead-api: 1.0.11
          antisamy-markup-formatter: 2.1
          script-security: 1.75
          

          2. Create job from acTest_config.xml
          3. Build with Parameters for that job, and approve the required script signatures (method groovy.lang.Binding getVariables and method groovy.lang.Script println java.lang.Object)
          4. Refresh the page after approving signatures. You should see a select with options in the order they were added to the map, i.e. "snapshot e", d, c, b, a.

          5. Update uno-choice to 2.5 (all other plugins stay the same) with restart after update, log back in, refresh the Build with Parameters page for the job. You'll now see the parameters out of order... in this case d, c, a, b, e.

          I'm pretty sure that the issue is coming from the new output sanitization for maps, specifically https://github.com/jenkinsci/active-choices-plugin/commit/62fb22b2d21d3bd2dfeaa051f283686c65dd6f7c#diff-55aadde5a4307258d48d88b45f5bf4e3e15f247c0be909a90eb0be8808e49eebR227-R235

                      Map<Object, Object> map = (Map<Object, Object>) returnValue;
                      Map<Object, Object> returnMap = new HashMap<>(map.size());
                      map.forEach((key, value) -> {
                          String newKey = sanitizeString(key);
                          String newValue = sanitizeString(value);
                          returnMap.put(newKey, newValue);
                      });
                      return returnMap;
          

          I'm really not experienced with Java at all... but we're relying on the fact that Groovy's Map stores items in the order they're added, and I think something here in the type casting and subsequent forEach is changing that order.

          Show
          jantman Jason Antman added a comment - Bruno P. Kinoshita Thank you so much for the quick reply. I've reproduced this issue with a minimal job and minimal plugins using the WAR file. 1. Run Jenkins 2.2.49.2 (latest LTS) from WAR. Install plugins as follows (uno-choice installed via HPI download): uno-choice: 2.4 trilead-api: 1.0.11 antisamy-markup-formatter: 2.1 script-security: 1.75 2. Create job from acTest_config.xml 3. Build with Parameters for that job, and approve the required script signatures ( method groovy.lang.Binding getVariables and method groovy.lang.Script println java.lang.Object ) 4. Refresh the page after approving signatures. You should see a select with options in the order they were added to the map, i.e. "snapshot e", d, c, b, a. 5. Update uno-choice to 2.5 (all other plugins stay the same) with restart after update, log back in, refresh the Build with Parameters page for the job. You'll now see the parameters out of order... in this case d, c, a, b, e. I'm pretty sure that the issue is coming from the new output sanitization for maps, specifically https://github.com/jenkinsci/active-choices-plugin/commit/62fb22b2d21d3bd2dfeaa051f283686c65dd6f7c#diff-55aadde5a4307258d48d88b45f5bf4e3e15f247c0be909a90eb0be8808e49eebR227-R235 Map< Object , Object > map = (Map< Object , Object >) returnValue; Map< Object , Object > returnMap = new HashMap<>(map.size()); map.forEach((key, value) -> { String newKey = sanitizeString(key); String newValue = sanitizeString(value); returnMap.put(newKey, newValue); }); return returnMap; I'm really not experienced with Java at all... but we're relying on the fact that Groovy's Map stores items in the order they're added, and I think something here in the type casting and subsequent forEach is changing that order.
          Hide
          kinow Bruno P. Kinoshita added a comment -

          Thank *you* Jason Antman. I think your analysis is correct! Let me reproduce it locally, and if I manage to find a quick fix, we can release 2.5.1 this weekend or next week.

           

           

          Show
          kinow Bruno P. Kinoshita added a comment - Thank * you * Jason Antman . I think your analysis is correct! Let me reproduce it locally, and if I manage to find a quick fix, we can release 2.5.1 this weekend or next week.    
          Hide
          kinow Bruno P. Kinoshita added a comment - - edited

          Fixed over here Jason Antman: https://github.com/jenkinsci/active-choices-plugin/pull/39

          It was pretty easy to fix, mainly because of a) your good issue description and b) your excellent instructions to reproduce. And, of course, also your precise analysis of the issue.

          IOW, you made it really easy for me to fix it, which means we should be able to release it either this weekend or by Monday. Reason for not releasing right now is that I want to try to write a small test for that. If that method can be unit-tested, it will be pretty simple. If it gets too tricky to test that (due to mocks necessary, or whatnot), then I will need to use the Jenkins testing API, which takes a bit longer to get a test working OK.

          Thanks

          Bruno

          Show
          kinow Bruno P. Kinoshita added a comment - - edited Fixed over here Jason Antman : https://github.com/jenkinsci/active-choices-plugin/pull/39 It was pretty easy to fix, mainly because of a) your good issue description and b) your excellent instructions to reproduce. And, of course, also your precise analysis of the issue. IOW, you made it really easy for me to fix it, which means we should be able to release it either this weekend or by Monday. Reason for not releasing right now is that I want to try to write a small test for that. If that method can be unit-tested, it will be pretty simple. If it gets too tricky to test that (due to mocks necessary, or whatnot), then I will need to use the Jenkins testing API, which takes a bit longer to get a test working OK. Thanks Bruno
          Hide
          kinow Bruno P. Kinoshita added a comment -

          Wrote what looks like a functional test, using your example Jason Antman. Tested on `master`:

          java.lang.AssertionError: Wrong HTML options rendered (or out of order) expected:<[Select a backup file or snapshot to restore for foo enviro bar, snapshot e, snapshot d, snapshot c, snapshot b, snapshot a]> but was:<[Select a backup file or snapshot to restore for foo enviro bar, snapshot d, snapshot c, snapshot a, snapshot b, snapshot e]>

          And on the pull request code it passes with no issues. So hopefully we won't have this issue again (cannot promise other issues though )

          Thanks again for making it so easy to fix this issue. I'm cutting a release right now, it should be available in a couple hours or so. Even though it's a small release, if you have a testbed/backup server, that might be a good place to test it (being paranoid after some updates issues, but never hurts right).

           

          Cheers

          Bruno

          Show
          kinow Bruno P. Kinoshita added a comment - Wrote what looks like a functional test, using your example Jason Antman . Tested on `master`: java.lang.AssertionError: Wrong HTML options rendered (or out of order) expected:<[Select a backup file or snapshot to restore for foo enviro bar, snapshot e, snapshot d, snapshot c, snapshot b, snapshot a]> but was:<[Select a backup file or snapshot to restore for foo enviro bar, snapshot d, snapshot c, snapshot a, snapshot b, snapshot e]> And on the pull request code it passes with no issues. So hopefully we won't have this issue again (cannot promise other issues though ) Thanks again for making it so easy to fix this issue. I'm cutting a release right now, it should be available in a couple hours or so. Even though it's a small release, if you have a testbed/backup server, that might be a good place to test it (being paranoid after some updates issues, but never hurts right).   Cheers Bruno
          Hide
          jantman Jason Antman added a comment -

          Bruno,

           

          Thank you so, so, so much for the amazingly fast fix for this! I've verified the new releases fixes the issue on two of our Jenkins instances.

           

          Thanks!!!

          Jason

          Show
          jantman Jason Antman added a comment - Bruno,   Thank you so, so, so much for the amazingly fast fix for this! I've verified the new releases fixes the issue on two of our Jenkins instances.   Thanks!!! Jason
          Hide
          kinow Bruno P. Kinoshita added a comment -

          Thanks for confirming it fixed the issue Jason, and once again, thanks a lot for making it so easy to fix it

          Cheers

          Bruno

          Show
          kinow Bruno P. Kinoshita added a comment - Thanks for confirming it fixed the issue Jason, and once again, thanks a lot for making it so easy to fix it Cheers Bruno
          Hide
          kinow Bruno P. Kinoshita added a comment -

          In 2.5.1

          Show
          kinow Bruno P. Kinoshita added a comment - In 2.5.1

            People

            Assignee:
            kinow Bruno P. Kinoshita
            Reporter:
            jantman Jason Antman
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: