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 8 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
15 changes: 11 additions & 4 deletions src/controllers/locations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,15 @@ export default {
sortBy,
} = req.query;

const capDetailedLocations = (requestedLimit) => {
const maxDetailedLocations = 200;
return locationFieldsOnly
? requestedLimit
: Math.min(requestedLimit, maxDetailedLocations);
};

const pageNumber = _pageNumber ? parseInt(_pageNumber, 10) : undefined;
const pageSize = _pageNumber ? parseInt(_pageSize, 10) : undefined;
const pageSize = _pageNumber ? capDetailedLocations(parseInt(_pageSize, 10)) : undefined;
const age = _age ? parseInt(_age, 10) : undefined;
const ageMin = _ageMin ? parseInt(_ageMin, 10) : undefined;
const ageMax = _ageMax ? parseInt(_ageMax, 10) : undefined;
Expand Down Expand Up @@ -232,7 +239,7 @@ export default {
const taxonomyIds = taxonomyId.split(',');
filterParameters.taxonomyIds = await models.Taxonomy.getAllIdsWithinTaxonomies(taxonomyIds);
}
const limit = pageSize || maxResults;
const limit = pageSize || capDetailedLocations(maxResults);

const offset = pageNumber !== undefined && pageSize !== undefined ?
pageNumber * pageSize : undefined;
Expand All @@ -243,7 +250,7 @@ export default {
} = await models.Location.search({
position: (longitude && latitude) ? geometry.createPoint(longitude, latitude) : null,
radius,
minResults,
minResults: capDetailedLocations(minResults),
filterParameters,
locationFieldsOnly,
noServices: parseBoolean(noServices),
Expand All @@ -252,7 +259,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
104 changes: 53 additions & 51 deletions src/models/location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,13 @@ module.exports = (sequelize, DataTypes, Op) => {
return sequelize.and(requiredDocumentCondition, notRequiredDocumentCondition);
};

Location.findUniqueLocationIds = async (filterParameters,
Location.findUniqueLocationStubs = async (
filterParameters,
additionalConditions,
originalQueryProps = {},
selectedAttributeForOrderBy, noServices) => {
selectedAttributeForOrderBy,
noServices,
) => {
const queryProps = { order: originalQueryProps.order };
// eslint-disable-next-line prefer-destructuring
const limit = originalQueryProps.limit;
Expand Down Expand Up @@ -371,11 +374,9 @@ module.exports = (sequelize, DataTypes, Op) => {
return Location.findAll({
...queryProps,
where: sequelize.and(..._whereConditions, ...additionalConditions),
attributes: [
sequelize.fn('DISTINCT', sequelize.col('Location.id')),
// For SELECT DISTINCT, ORDER BY expressions must appear in select list.
...(selectedAttributeForOrderBy ? [selectedAttributeForOrderBy] : []),
],
attributes: {
include: selectedAttributeForOrderBy ? [selectedAttributeForOrderBy] : undefined,
},
raw: true,
// Not like associations and grouping work perfectly out of the box either though...
// https://github.com/sequelize/sequelize/issues/5481
Expand All @@ -390,14 +391,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 @@ -489,16 +491,17 @@ module.exports = (sequelize, DataTypes, Op) => {

].reduce((a, b) => a.concat(b));

// Remove duplicates
locations = Array.from(new Map(searchResults.map(item => [item.id, item])).values());
locations = searchResults;
} else {
locations = await findAll(whereConditions);
}

// apply limit and offset in memory here
locations = locations.slice(offset || 0, limit ? (offset || 0) + limit : undefined);
// Remove duplicates
locations = Array.from(new Map(locations.map(item => [item.id, item])).values())
// apply limit and offset in memory here
.slice(offset || 0, limit ? (offset || 0) + limit : undefined);

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

Location.search = async ({
Expand All @@ -512,7 +515,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 +549,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 +571,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