Skip to content

Limit IDs passed to location assocations query #189

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.

Sign up for GitHub

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

Open
Rovack wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Limit IDs passed to location assocations query #189

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
346a456
feature: limit IDs passed to location assocations query
Rovack Mar 13, 2026
bb91275
Actually return the location stubs fetched in the 1st query
Rovack Mar 13, 2026
2d37376
Fix pagination to take the updated limit into account
Rovack Mar 13, 2026
e39f2f1
Skip redundant plain-ification for already-raw locations
Rovack Mar 13, 2026
bd996a6
Actually return all fetched fields in stubs, not just IDs
Rovack Mar 13, 2026
d7aa7f1
Fix remaining pageSize vs. limit discrepancies
Rovack Mar 13, 2026
3b92c5e
Cap minResults too (though highly unlikely to exceed 200)
Rovack Mar 13, 2026
6ffc84e
Fix sorting by mostServices
Rovack Mar 13, 2026
bcb3f77
Restore event info to the locationFieldsOnly case
Rovack Mar 14, 2026
6bc827a
Stop passing the unused Services to isLocationClosed
Rovack Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/controllers/locations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,15 @@ export default {
const taxonomyIds = taxonomyId.split(',');
filterParameters.taxonomyIds = await models.Taxonomy.getAllIdsWithinTaxonomies(taxonomyIds);
}
const limit = pageSize || maxResults;

const offset = pageNumber !== undefined && pageSize !== undefined ?
pageNumber * pageSize : undefined;
const requestedLimit = pageSize || maxResults;
const maxDetailedResults = 200;
const limit = locationFieldsOnly
? requestedLimit
: Math.min(requestedLimit, maxDetailedResults);
Comment thread
Rovack marked this conversation as resolved.
Outdated
Comment thread
Rovack marked this conversation as resolved.
Outdated
Comment thread
Rovack marked this conversation as resolved.
Outdated

const offset = pageNumber !== undefined && limit !== undefined ?
pageNumber * limit : undefined;

const {
locations,
Expand All @@ -252,7 +257,7 @@ export default {
sortBy,
});
const plainLocations = await locations
.map(location => location.get({ plain: true }));
.map(location => (location.get ? location.get({ plain: true }) : location));
const paginationCount = Math.ceil(totalNumLocations / pageSize);

const formattedLocations = plainLocations.map((location) => {
Expand Down
82 changes: 41 additions & 41 deletions src/models/location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ module.exports = (sequelize, DataTypes, Op) => {
return sequelize.and(requiredDocumentCondition, notRequiredDocumentCondition);
};

Location.findUniqueLocationIds = async (filterParameters,
Location.findUniqueLocationStubs = async (filterParameters,
additionalConditions,
originalQueryProps = {},
selectedAttributeForOrderBy, noServices) => {
Expand Down Expand Up @@ -390,14 +390,15 @@ module.exports = (sequelize, DataTypes, Op) => {
sequelize.models.Organization,
sequelize.models.PhysicalAddress,
sequelize.models.Phone,
sequelize.models.EventRelatedInfo,
{
model: sequelize.models.Service,
required: !noServices,
include: [
sequelize.models.Taxonomy,
sequelize.models.HolidaySchedule,
...(areRequiredDocsSpecified ? [sequelize.models.RequiredDocument] : []),
...((openAt && !occasion) ? [sequelize.models.RegularSchedule] : []),
...(occasion ? [sequelize.models.HolidaySchedule] : []),
...(servesZipcode ? [sequelize.models.ServiceArea] : []),
...(shouldJoinEligibilities ? [{
model: sequelize.models.Eligibility,
Expand Down Expand Up @@ -498,7 +499,7 @@ module.exports = (sequelize, DataTypes, Op) => {
// apply limit and offset in memory here
locations = locations.slice(offset || 0, limit ? (offset || 0) + limit : undefined);

return locations.map(location => location.id);
return locations;
};

Location.search = async ({
Expand All @@ -512,7 +513,7 @@ module.exports = (sequelize, DataTypes, Op) => {
sortBy,
noServices,
}) => {
let locationIds;
let locationStubs;
let distance;
let totalNumLocations;
// order is used to specify the attribute referenced in the ORDER BY
Expand Down Expand Up @@ -546,12 +547,12 @@ module.exports = (sequelize, DataTypes, Op) => {
if (radius && position) {
const distanceCondition = sequelize.where(distance, { [Op.lte]: radius });

totalNumLocations = (await Location.findUniqueLocationIds(
totalNumLocations = (await Location.findUniqueLocationStubs(
filterParameters,
[distanceCondition],
)).length;

locationIds = await Location.findUniqueLocationIds(
locationStubs = await Location.findUniqueLocationStubs(
filterParameters,
[distanceCondition].filter(Boolean), {
order,
Expand All @@ -568,54 +569,53 @@ module.exports = (sequelize, DataTypes, Op) => {
// However, filtering by window functions requires nested queries, which
// aren't natively supported by sequelize and would require a raw query.
// For now, the simplicity and security of sequelize seems worth the slight performance hit.
if (minResults && locationIds.length < minResults) {
totalNumLocations = (await Location.findUniqueLocationIds(filterParameters, [])).length;
locationIds = await Location.findUniqueLocationIds(filterParameters, [], {
if (minResults && locationStubs.length < minResults) {
totalNumLocations = (await Location.findUniqueLocationStubs(filterParameters, [])).length;
locationStubs = await Location.findUniqueLocationStubs(filterParameters, [], {
order,
limit: minResults,
offset,
}, selectedAttributeForOrderBy, noServices);
}
} else {
totalNumLocations = (await Location.findUniqueLocationIds(filterParameters, [])).length;
locationIds = await Location.findUniqueLocationIds(filterParameters, [], {
totalNumLocations = (await Location.findUniqueLocationStubs(filterParameters, [])).length;
locationStubs = await Location.findUniqueLocationStubs(filterParameters, [], {
limit,
offset,
order,
}, selectedAttributeForOrderBy);
}

const additionalLocationData = locationFieldsOnly ? [
sequelize.models.EventRelatedInfo,
{
model: sequelize.models.Service,
include: [
sequelize.models.HolidaySchedule,
],
},
] : [
sequelize.models.Organization,
sequelize.models.EventRelatedInfo,
{
model: sequelize.models.Service,
include: [
sequelize.models.Taxonomy,
sequelize.models.RequiredDocument,
sequelize.models.HolidaySchedule,
],
},
sequelize.models.Phone,
sequelize.models.PhysicalAddress,
];
const locationIds = locationStubs.map(location => location.id);

const locationsWithAssociations = await Location.findAll({
attributes: {
include: selectedAttributeForOrderBy ? [selectedAttributeForOrderBy] : undefined,
},
where: { id: { [Op.in]: locationIds } },
include: additionalLocationData,
order,
});
let locationsWithAssociations;
if (locationFieldsOnly) {
locationsWithAssociations = locationStubs;
Comment thread
Rovack marked this conversation as resolved.
Comment thread
Rovack marked this conversation as resolved.
} else {
Comment thread
Rovack marked this conversation as resolved.
const additionalLocationData = [
sequelize.models.Organization,
sequelize.models.EventRelatedInfo,
{
model: sequelize.models.Service,
include: [
sequelize.models.Taxonomy,
sequelize.models.RequiredDocument,
sequelize.models.HolidaySchedule,
],
},
sequelize.models.Phone,
sequelize.models.PhysicalAddress,
];

locationsWithAssociations = await Location.findAll({
attributes: {
include: selectedAttributeForOrderBy ? [selectedAttributeForOrderBy] : undefined,
},
where: { id: { [Op.in]: locationIds } },
include: additionalLocationData,
order,
});
}

function sortByLocationIds(a, b) {
return locationIds.indexOf(a.id) - locationIds.indexOf(b.id);
Expand Down
Loading