Uploaded image for project: 'Wicket'
  1. Wicket
  2. WICKET-6132

AbstractChoice#getChoices() should be final

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 7.2.0
    • Fix Version/s: 8.0.0-M1
    • Component/s: None
    • Labels:
      None

      Description

      Not a bug, but a welcome improvement. The AbstractChoice#getChoices() method is non-final (the setter is though), which can cause serious application faults migrating from Wicket 6.

      I attached a Wicket 6 quickstart as well as a Wicket 7 version to reproduce the issue using two DropDownChoices.

      In Wicket 6, you could do the following with no problem. First you construct a DropDownChoice, supplying your collection of choices. Apparently, in Wicket 6, this initial collection is not directly used when converting user input. You could override the getChoices function and supply a totally different collection without any problem. You could exploit this to create a dynamic DropDownChoice that has different select options, depending on the outcome of getChoices().

      In Wicket 7 however, while converting user input, the list of choices you supplied initially is directly accessed, circumventing the getChoices method. The dynamic DropDownChoice which was working fine in Wicket 6 will most probably still work, but with surprising results...

      To reproduce, fire up supplied Wicket 6 quickstart. You'll get two DropDownChoices, make and model of cars. Check that the model drop down is showing models for both Ferrari and Chevrolet. Now, choose Ferrari as the make and for example F355 as the model. A label has been updated to show what has been selected. Now, switch the make to Chevrolet and pick the El Camino. The label will have updated accordingly and will say "Chevrolet El Camino".

      Now, try the same reproduction path in the Wicket 7 quickstart. There is no error, but the label will say "Chevrolet Testarossa". As much as I would love to see how such a mash-up of iconic cars will work out, this is obviously not what the user expected.

      Migrating from Wicket 6 to 7 can get you unexpected behavior which could've been prevented if AbstractChoice#getChoices() had been made final. The resulting compile error is much easier to fix than having to fix potentially broken data a few weeks in production...

      Note: I know that this is a totally wrong approach to creating dynamic form input components, you should always update components through their models. It's just that you've always got that piece of legacy code, written by that someone that didn't quite grasp Wicket's way. It might even have been myself as a junior developer

      1. cars-wicket6.zip
        9 kB
        Rens Verhage
      2. cars-wicket7.zip
        10 kB
        Rens Verhage

        Issue Links

          Activity

          Hide
          mgrigorov Martin Grigorov added a comment -

          I don't see a way how to improve the state in Wicket 7.x.
          Initially I thought about marking #getChoices() as deprecated, and to recommend #getChoicesModel(). But there are several usages of #getChoices() in Wicket classes and if the application overrides #getChoicedModel() then Wicket won't use it.
          If we change those usages in Wicket then we will most probably break applications which override #getChoices() today.
          I'll just leave it as is in Wicket 7.x and an an entry to the migration guide for 8.x.

          Show
          mgrigorov Martin Grigorov added a comment - I don't see a way how to improve the state in Wicket 7.x. Initially I thought about marking #getChoices() as deprecated, and to recommend #getChoicesModel(). But there are several usages of #getChoices() in Wicket classes and if the application overrides #getChoicedModel() then Wicket won't use it. If we change those usages in Wicket then we will most probably break applications which override #getChoices() today. I'll just leave it as is in Wicket 7.x and an an entry to the migration guide for 8.x.
          Hide
          verhage Rens Verhage added a comment - - edited

          Yes, I agree This is indeed an issue in the quickstart I provided. My bad!

          Show
          verhage Rens Verhage added a comment - - edited Yes, I agree This is indeed an issue in the quickstart I provided. My bad!
          Hide
          mgrigorov Martin Grigorov added a comment -

          I've made #getChoices() final in 8.x. I agree this is a good thing to do to fix the problem with the desynchronization with #getChoicesModel().

          But as you can see (by updating your quickstart to 8.0.0-SNAPSHOT) this doesn't solve the problem in your quickstart. And I think the problem is in the quickstart itself. It needs to reset the dropdown's model when new choices are provided.
          I think all versions of Wicket allowed something like: new DropDownChoice(id, Model.of("NotAChoice"), Arrays.asList("One", "Two", "Three")).
          In this case the model object is "NotAChoice" so any Label that refers to this model will render this value. But the dropdown itself will render only the provided valid choices, and none of them will be 'selected'.

          Do you agree ?

          Show
          mgrigorov Martin Grigorov added a comment - I've made #getChoices() final in 8.x. I agree this is a good thing to do to fix the problem with the desynchronization with #getChoicesModel(). But as you can see (by updating your quickstart to 8.0.0-SNAPSHOT) this doesn't solve the problem in your quickstart. And I think the problem is in the quickstart itself. It needs to reset the dropdown's model when new choices are provided. I think all versions of Wicket allowed something like: new DropDownChoice(id, Model.of("NotAChoice"), Arrays.asList("One", "Two", "Three")). In this case the model object is "NotAChoice" so any Label that refers to this model will render this value. But the dropdown itself will render only the provided valid choices, and none of them will be 'selected'. Do you agree ?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7b10fa39a6496c1d0eb22adf794c8bb6ea1c785f in wicket's branch refs/heads/master from Martin Grigorov
          [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=7b10fa3 ]

          WICKET-6132 AbstractChoice#getChoices() should be final

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7b10fa39a6496c1d0eb22adf794c8bb6ea1c785f in wicket's branch refs/heads/master from Martin Grigorov [ https://git-wip-us.apache.org/repos/asf?p=wicket.git;h=7b10fa3 ] WICKET-6132 AbstractChoice#getChoices() should be final
          Hide
          mgrigorov Martin Grigorov added a comment -

          The problem is that in 7.x we have AbstractChoice#getChoicesModel() and AbstractChoice#getChoices().
          In 6.x we have only the latter.

          Making #getChoices() final won't help because AbstractSingleSelectChoice#convertChoiceIdToChoice() uses #getChoicesModel().

          So the real problem is that in 7.0+ the application must override both methods to behave properly.
          Let's see how this could be improved!

          Show
          mgrigorov Martin Grigorov added a comment - The problem is that in 7.x we have AbstractChoice#getChoicesModel() and AbstractChoice#getChoices(). In 6.x we have only the latter. Making #getChoices() final won't help because AbstractSingleSelectChoice#convertChoiceIdToChoice() uses #getChoicesModel(). So the real problem is that in 7.0+ the application must override both methods to behave properly. Let's see how this could be improved!
          Hide
          mgrigorov Martin Grigorov added a comment -

          Here Wicket 6.x quickstart always produces bad results, like "Ferrari Camaro".
          Wicket 7.x quickstart sometimes produces good result, i.e. updates the model and respectively the label, and sometimes behaves like the 6.x quickstart.

          I am a bit confused because you say that 6.x works good for you.

          Show
          mgrigorov Martin Grigorov added a comment - Here Wicket 6.x quickstart always produces bad results, like "Ferrari Camaro". Wicket 7.x quickstart sometimes produces good result, i.e. updates the model and respectively the label, and sometimes behaves like the 6.x quickstart. I am a bit confused because you say that 6.x works good for you.
          Hide
          verhage Rens Verhage added a comment -

          That sounds like a good solution for Wicket 7.x. I think it would be best to make the method final in Wicket 8 as it is a 'bug' that is easily re-introduced.

          Show
          verhage Rens Verhage added a comment - That sounds like a good solution for Wicket 7.x. I think it would be best to make the method final in Wicket 8 as it is a 'bug' that is easily re-introduced.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Thanks for the report!

          We cannot make the method 'final' because this may break many applications out there.
          I think it would be better if we always use #getChoices() instead of directly referencing the field.

          Show
          mgrigorov Martin Grigorov added a comment - Thanks for the report! We cannot make the method 'final' because this may break many applications out there. I think it would be better if we always use #getChoices() instead of directly referencing the field.
          Hide
          verhage Rens Verhage added a comment -

          Added both quickstart projects

          Show
          verhage Rens Verhage added a comment - Added both quickstart projects

            People

            • Assignee:
              mgrigorov Martin Grigorov
              Reporter:
              verhage Rens Verhage
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development