Open lindseydiloreto opened 3 years ago
I came across this while encountering an N+1 problem when listing elements along with their addresses and wondered if there was an eager-loading opportunity. I'd suggest approaching this with a behavior and a left join on the addresses table, similar to how I achieved it here: https://github.com/putyourlightson/craft-entry-count/blob/82f92377178de7b1c4cf7c6ef36997f5d10abb72/src/EntryCount.php#L97-L132
Ah, fantastic tip! Thanks @bencroker, I'll almost certainly go this route when I eventually implement it. 🍺
It may still be awhile, however, before I make any adjustments. Craft 4 is likely to bring some changes which may affect this architecture.
I just ran across the N+1 problem with the address field as well. I'm working on an API that returns a couple hundred entries with an address field. Every entry needs one additional query for the address, I'm trying to optimize that. I actually found a mostly workable solution that would require only a minor change without a backwards compatibility break.
First, here's the current code that's causing the N+1 problem (location
being the address field):
$coordinates = ['lat' => 50.9549, 'lng' => 7.0121, ];
$entries = Entry::find()
->location(['target' => $coordinates])
->orderBy(['distance' => SORT_ASC, 'title' => SORT_ASC])
->limit(100)
->all();
foreach ($entries as $entry) {
$location = $entry->location;
// do additional stuff
}
Now every call to $entry->location
causes one additional database query because the location record is loaded on demand in AddressField::normalizeValue
:
$record = AddressRecord::findOne([
'elementId' => $element->id,
'fieldId' => $this->id,
]);
To prevent this, we can just use the address active record class directly to query all addresses at once:
$locationsField = Craft::$app->getFields()->getFieldByHandle('location');
$locations = Address::find()->where([
'elementId' => array_column($entries, 'id'),
'fieldId' => $locationsField->id,
])->indexBy('elementId')->all();
foreach ($entries as $entry) {
$location = $locations[$entry->id];
// do additional stuff
}
The problem is that now, $location
is an doublesecretagency\googlemaps\records\Address
instead of an doublesecretagency\googlemaps\models\Address
. Now I can just copy over everything that AddressField::normalizeValue
does to create a model based on the record:
// Get the record attributes
$omitColumns = ['dateCreated','dateUpdated','uid'];
$attr = $record->getAttributes(null, $omitColumns);
// Convert coordinates to floats
$attr['lat'] = ($attr['lat'] ? (float) $attr['lat'] : null);
$attr['lng'] = ($attr['lng'] ? (float) $attr['lng'] : null);
// Check if JSON is valid
// Must use this function to validate (I know it's redundant)
$valid = json_decode($attr['raw']);
// Convert raw data to an array
$attr['raw'] = ($valid ? Json::decode($attr['raw']) : null);
// If part of a proximity search, get the distance
if ($value && is_numeric($value)) {
$attr['distance'] = (float) $value;
}
// Return an Address model
return new AddressModel($attr);
This works fine, but of course copying all this code over is not ideal. I see two possible solutions:
AddressField::normalizeValue
, check if the passed $value
is already an address record and then perform the same steps to transform it into an address model. Then we could just call AddressField::normalizeValue($addressRecord)
to get the address model.AddressField
or a helper class. Then I could call, for example, AddressField::addressRecordToModel($locationRecord)
.Maybe even both. Both of those would be backwards compatible. Maybe the process could even be simplified by providing a utility to perform the bulk loading of addresses.
@lindseydiloreto Would you be open to those suggestions? I'd be happy to write up a PR.
This one is a bit tricky. The native behavior for eager loading only takes into account full-fledged Elements. However, the Address is just a single field.
Assuming that it's even possible, we'll probably need to get a bit creative to make eager-loading work. We're not ready to take this on quite yet, but in theory we'll circle back on it eventually.
We'll keep it on our radar, but definitely can't provide an ETA. Feel free to subscribe to this thread for eventual updates.