[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: File testPage.yaml    
Issue Links:
dependency
depends upon PAGES-38 TemplateSelectorFieldFactory may retu... Closed
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:
It was first encountered in UI tests when adding a page at root level (maybe since MGNLSTK-1500, instance with STK + templating-samples). Basically, if the TemplateDefinition has no title set, then the TemplateSelectorFieldFactory (in pages app) may set the SelectFieldOptionDefinition's label to null.
Note that super-factory SelectFieldFactory uses the value when label would otherwise be null. We may create a separate ticket to address that.

Example 2, Jan says:

you can run into it on creation of template when someone else is opening the selector ... so there is a race condition and we need to fix it in comparator

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:

  • sgn(x.compareTo(y)) == -sgn(y.compareTo(x)) is not fulfilled if both labels are null
  • must throw NullPointerException if the specified object (in this case other) is null
  • strongly recommends that (x.compareTo(y)==0) == (x.equals(y))—which is hard to imagine in our case (label is not a unique representation of the object).

In fact, because of this, I don't think there's a way to fully respect the contract of #compareTo.

  1. I would advice to replace the "natural" comparison in SelectFieldFactory@138 with a default label-based Comparator. This gives us more freedom with implementation.
    • Deprecating SelectFieldOptionDefinition#compareTo along the way.
  2. It is our job to make sure label is never null, and in fact not blank.
    • We need a fix in pages' TemplateSelectorFieldFactory.
    • UI's SelectFieldFactory seems to be fine, see #getLabel() there.

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.
For the label, I think we should fallback to the TemplateDefinition's name (if not blank) or id (never blank).

Comment by Aleksandr Pchelintcev [ 22/Sep/15 ]
  • DefaultSelectFieldLabelComparator
    • I think the name is a bit unfortunate, since it compares not the SelectField labels, but select field options via their labels.
  • As for mgeljic's suggestion to prevent label from being null - maybe should just add a fallback to name should the labels be the same/both nulls?

OptionGroupFieldFactory - good refactoring w/ CompoProvider, but

  • CompoProvider is usually set from outside By FieldFactoryFactory,so in c-tor setComponentProvider(componentProvider) should probably be omitted (and tests adapted)
Comment by Federico Grilli [ 22/Sep/15 ]

DefaultSelectFieldLabelComparator
I think the name is a bit unfortunate, since it compares not the SelectField labels, but select field options via their labels.

What about DefaultSelectFieldOptionLabelComparator?

As for Mikaël Geljić's suggestion to prevent label from being null - maybe should just add a fallback to name should the labels be the same/both nulls?

If i got it right Mika suggested to fix that in TemplateSelectorFieldFactory which I did in PAGES-38

OptionGroupFieldFactory - good refactoring w/ CompoProvider, but
CompoProvider is usually set from outside By FieldFactoryFactory,so in c-tor setComponentProvider(componentProvider) should probably be omitted (and tests adapted)

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 ]
  • DefaultSelectFieldOptionLabelComparator sounds almost good to me, though I'd maybe lose the 'Default' word cause then it kinda suggest that there could be other SelectFieldOptionLabelComparator's (which itself is a concrete case already).
  • re:PAGES-38 - if you discussed that w/ mgeljic, guess then so be it, though I think it's a bit an overkill to make sure that several factories/definitions eagerly comply to a comparator requirements. As I said - comparator falling back to an option name (dunno if it can be null) sounds a bit more logical to me.
  • With compo provider - also okay, but in normal case it's still gonna be reset from outside, so that setCompoProvider call in c-tor might be a bit mis-leading
Comment by Mikaël Geljić [ 22/Sep/15 ]
  1. Naming: I'd drop the "Label" part if you ask me (at least either default or label, not both) => DefaultSelectFieldOptionComparator, or even DefaultOptionComparator (see point 2)
  2. Move it out of the SelectFieldOptionDefinition, I think it belongs to the SelectFieldFactory actually
  3. Redundancy: let's avoid having it both as the default comparatorClass and re-instantiated in SelectFieldFactory. I'd probably go with the latter (especially in conjunction with point 2)
  4. check on label-only is indeed what we want (It needs to be non-blank for the user, besides sorting anyway).
  5. componentProvider being set from the outside sounds a bit fishy to me as well (seemingly by FormBuilder though). If in doubt, rather not change this?
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.

Generated at Mon Feb 12 09:07:26 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.