[MGNLUI-3521] SelectFieldOptionDefinition comparison may throw NPE Created: 12/Aug/15 Updated: 15/Apr/16 Resolved: 22/Sep/15 |
|
| Status: | Closed |
| Project: | Magnolia UI |
| Component/s: | forms |
| Affects Version/s: | 5.4 |
| Fix Version/s: | 5.3.12, 5.4.3 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Mikaël Geljić | Assignee: | Federico Grilli |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | fields, select | ||
| Remaining Estimate: | 2d | ||
| Time Spent: | 1d | ||
| Original Estimate: | 3d | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| Acceptance criteria: |
Empty
|
||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||
| Date of First Response: | |||||||||
| Sprint: | Basel 11 | ||||||||
| Story Points: | 8 | ||||||||
| Description |
|
SelectFieldOptionDefinition's implementation of compareTo is fishy, and assumes the label field is never null, when in fact it might be. Example 1: Example 2, Jan says:
Results in the following NPE: [INFO] [talledLocalContainer] Caused by: java.lang.NullPointerException [INFO] [talledLocalContainer] at info.magnolia.ui.form.field.definition.SelectFieldOptionDefinition.compareTo(SelectFieldOptionDefinition.java:100) [INFO] [talledLocalContainer] at info.magnolia.ui.form.field.definition.SelectFieldOptionDefinition.compareTo(SelectFieldOptionDefinition.java:43) [INFO] [talledLocalContainer] at java.util.ComparableTimSort.binarySort(ComparableTimSort.java:232) [INFO] [talledLocalContainer] at java.util.ComparableTimSort.sort(ComparableTimSort.java:176) [INFO] [talledLocalContainer] at java.util.ComparableTimSort.sort(ComparableTimSort.java:146) [INFO] [talledLocalContainer] at java.util.Arrays.sort(Arrays.java:472) [INFO] [talledLocalContainer] at java.util.Collections.sort(Collections.java:155) [INFO] [talledLocalContainer] at info.magnolia.ui.form.field.factory.SelectFieldFactory.buildOptions(SelectFieldFactory.java:138) [INFO] [talledLocalContainer] at info.magnolia.ui.form.field.factory.SelectFieldFactory.createFieldComponent(SelectFieldFactory.java:98) |
| Comments |
| Comment by Mikaël Geljić [ 14/Sep/15 ] |
|
On a detail level, javadoc of Comparable#compareTo specifies a few requirements:
In fact, because of this, I don't think there's a way to fully respect the contract of #compareTo.
I'll just attach a sample yaml definition, that I used to produce the NPE—with current fix it produces a blank line in the combobox. |
| Comment by Aleksandr Pchelintcev [ 22/Sep/15 ] |
OptionGroupFieldFactory - good refactoring w/ CompoProvider, but
|
| Comment by Federico Grilli [ 22/Sep/15 ] |
What about DefaultSelectFieldOptionLabelComparator?
If i got it right Mika suggested to fix that in TemplateSelectorFieldFactory which I did in
OK, but then having c-tor with such argument is confusing if the practice is to set it from outside. Should we remove it and deprecate the c-tor? (IIRC there's also another couple of Factories getting a ComponentProvider via their c-tor) |
| Comment by Aleksandr Pchelintcev [ 22/Sep/15 ] |
|
| Comment by Mikaël Geljić [ 22/Sep/15 ] |
|
| Comment by Aleksandr Pchelintcev [ 22/Sep/15 ] |
|
fgrilli/mgeljic I also noticed the location of a default comparator, but then forgot to list it in the comment - what Mika said about moving makes sense and then a simple name like DefaultOptionComparator indeed makes sense. Redundancy is also smth we could approach, so I'd say go for Mika's variant. |