Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eager-loading for Address data #12

Open
lindseydiloreto opened this issue Feb 28, 2021 · 3 comments
Open

Eager-loading for Address data #12

lindseydiloreto opened this issue Feb 28, 2021 · 3 comments
Labels
Feature Request New feature or request

Comments

@lindseydiloreto
Copy link
Collaborator

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.


Migrated from Smart Map: doublesecretagency/craft-smartmap#15

@bencroker
Copy link
Contributor

bencroker commented Oct 25, 2021

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

@lindseydiloreto
Copy link
Collaborator Author

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.

@MoritzLost
Copy link

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:

  • In 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.
  • Move the logic to map an address record to an address model into a public static method on the 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants