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 9 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
123 changes: 71 additions & 52 deletions src/models/location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ module.exports = (sequelize, DataTypes, Op) => {
});

const SERVICE_COUNT_COLUMN_ALIAS = 'service_count';

const SERVICE_COUNT_SUBQUERY = [
sequelize.literal(`(
SELECT cast(COUNT(*) as integer)
Expand All @@ -49,6 +48,16 @@ module.exports = (sequelize, DataTypes, Op) => {
SERVICE_COUNT_COLUMN_ALIAS,
];

const EVENTS_WITH_INFO_COLUMN_ALIAS = 'events_with_info';
const EVENTS_WITH_INFO_COLUMN_SUBQUERY = [
sequelize.literal(`(
SELECT JSON_AGG(DISTINCT event_related_info.event)
FROM event_related_info
WHERE event_related_info.location_id = "Location".id
)`),
EVENTS_WITH_INFO_COLUMN_ALIAS,
];

Location.associate = (models) => {
Location.belongsTo(models.Organization, { foreignKey: 'organization_id' });
Location.belongsToMany(models.Service, {
Expand Down Expand Up @@ -297,10 +306,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 +383,12 @@ 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: [
EVENTS_WITH_INFO_COLUMN_SUBQUERY,
...(selectedAttributeForOrderBy ? [selectedAttributeForOrderBy] : []),
],
Comment on lines +386 to +390

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore distinct ID projection in stub lookup query

Location.search still uses this helper for pagination/count paths ((await ...).length), but this change replaced the old DISTINCT Location.id projection with a full-row select (plus extra subqueries/joins) and then deduplicates in JavaScript. On broad searches this pulls and materializes many joined rows per location just to count IDs, which can significantly increase query latency and memory use and make /locations requests time out under production-sized datasets.

Useful? React with 👍 / 👎.

},
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 +403,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 +503,22 @@ 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);
const reconstructEvents = ({
[EVENTS_WITH_INFO_COLUMN_ALIAS]: events, ...rest
}) => ({ ...rest, EventRelatedInfos: (events || []).map(event => ({ event })) });

// 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)
.map(reconstructEvents);

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

Location.search = async ({
Expand All @@ -512,7 +532,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 +566,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 +588,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