search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

SearchableDropdownTrait::getSource doesn't respect lazy-loading #11272

Closed johannesx75 closed 1 week ago

johannesx75 commented 2 weeks ago

Module version(s) affected

5.2.10

Description

Prior to Silverstripe 5.2 auto scaffolding of has_one relations would use a NumericField when there were over 100 items. With the introduction of SearchableDropdownField the auto scaffolding now uses lazy-loading for large lists.

Unfortunately in the construction of the SearchableDropdownField getSource is called by getSourceEmpty. Even for lazy loaded SearchableDropdownFields.

SearchableDropdownTrait::getSource in turn calls SelectField::getListMap which tries to convert the large list into an array in memory.

As far as I can tell this array is never used, since the lazy-loading search will work on the Search Context to find elements. But trying to create the array on large lists leads to the form failing to render with a timeout or memory exhaustion.

How to reproduce

Reproduction Steps:

  1. Create simple model with has_one relationship (ClassA has_one ClassB, ClassB has_many ClassA)
  2. Add many thousands of Dataobjects of ClassB (250.000 in our case)
  3. Try opening ModelAdmin Detailform for ClassA

Possible Solution

Don't call getSource on lazyloaded SearchableDropdownFields.

Additional Context

Workaround: In ClassA::getCMSFields replace SearchableDropdownField with NumericField.

Validations

New issues created

PRs

AFTER merging PR

Reassign to Guy - I want to revert these changes and do the cleaner (API breaking) fix in CMS 6.

GuySartorelli commented 2 weeks ago

There are two different code paths that lead to calling getSource().

One is in the Field() method, which in DropdownField::Field() calls getSourceEmpty() which calls getSource(). We can take inspiration from silverstripe/tagfield here and just re-implement the Field() method to do the bare minimum that it needs to do.

The other is only when rendering the field in a react form (e.g. in elemental blocks). The call to getSchemaStateDefaults() ends up at SelectField::getSchemaStateDefaults() which calls getSource(). If I can find a clean solution for this one I'll raise a PR which solves both scenarios.

GuySartorelli commented 2 weeks ago

Ultimately the fact that SearchableDropdownTrait::getSource() returns an array should be considered a bug - it says it does so for compatibility with the parent class, but the parent class has a return type of array|ArrayAccess and DataList implements ArrayAccess, so it could have just returned that.

Regardless, the return type is strongly typed so we can't just change that in CMS 5. I'll submit a PR that fixes it for 5, and then submit another PR that fixes it better for 6.

emteknetnz commented 1 week ago

Linked PR has been merged, it will be automatically tagged shortly

GuySartorelli commented 1 week ago

Reopened and assigned to me as per instructions in issue description

GuySartorelli commented 1 week ago

getSourceEmpty() wants to concat something to the start of the source array (despite saying it can be ArrayAccess so there's a breach of types there) - I can't see a super easy way to resolve that right now so putting it in the too hard basket.