Limit IDs passed to location assocations query - #189
Conversation
- When locationFieldsOnly is true, get all datafields in the 1st query, so the 2nd one doesn't need a giant IN with countless IDs. - When locationFieldsOnly is false, disallow fetching >200 locations.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces a blocking regression in the locationFieldsOnly code path and changes response-limiting behavior without safeguards/tests.
Blocking issues
- Broken API behavior:
Location.search()now returns rawlocationIdswhenlocationFieldsOnlyis truthy (src/models/location.js:590-592), but the controller still assumes Sequelize model instances and callslocation.get(...)on each result (src/controllers/locations.js:259-260). This will throw at runtime (get is not a function) forlocationFieldsOnlyrequests. - Potential regression with no validation/tests: the new hard cap (
Math.min(requestedLimit, 200)) for non-locationFieldsOnlyresponses (src/controllers/locations.js:236-240) silently changes API behavior for clients requesting >200 results, including paginated flows, and no accompanying tests/documentation are present to prove this is safe.
Rovack
changed the title
Rovack
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 346a456f41
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
The PR introduces a blocking API regression in the locationFieldsOnly path and is not safe to approve.
Blocking issues
- Broken API behavior:
Location.searchnow returns raw plain objects forlocationFieldsOnly(locationsWithAssociations = locationStubs), but the controller still callslocation.get({ plain: true })on every result. This will throwTypeError: location.get is not a functionat runtime forGET /locationswithlocationFieldsOnly=true(src/models/location.js, src/controllers/locations.js). - Behavioral regression: the
locationFieldsOnlyresponse is now built fromfindUniqueLocationStubs, whose query selects onlyDISTINCT Location.id(+ optional sort attribute) withraw: true. Even if the runtime error were fixed, this path no longer reliably returns expected location fields/association structure used to computeclosed, changing API response semantics (src/models/location.js).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb912756e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Not safe to approve. The refactor introduces API behavior regressions in locationFieldsOnly responses and pagination semantics, with no accompanying test coverage for the new risky paths.
Blocking issues
- Broken API behavior for
locationFieldsOnly:Location.searchnow returnslocationStubsdirectly (src/models/location.js:592-594), but those stubs are produced by araw: truequery selecting onlyDISTINCT Location.id(plus optional sort attr) (src/models/location.js:374-379). This means callers can receive incomplete/flattened records instead of full location fields, andclosedcomputation in the controller can run with missingEventRelatedInfos/Servicesdata (src/controllers/locations.js:263-271). - Pagination regression: offset is now computed from capped
limit(src/controllers/locations.js:242-243) instead of requestedpageSize. For non-locationFieldsOnlyrequests wherepageSize > 200, pages will overlap/skip (e.g., page 1 offset 0, page 2 offset 200 instead of 300), causing duplicated/missing results and inconsistent paging behavior. - Risky logic change lacks targeted tests: no integration tests cover
locationFieldsOnlyresponse shape/closedcorrectness or pagination behavior with capped detailed results, so these regressions would not be caught pre-merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e39f2f156d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Remove attribute filtering from the query for location stubs. - Dedupe in memory, as the query doesn't do DISTINCT anymore.
There was a problem hiding this comment.
I found blocking API-behavior regressions in pagination and locationFieldsOnly result shaping, so this PR is not safe to approve yet.
Blocking issues
src/controllers/locations.js+src/models/location.js: pagination semantics are now inconsistent whenlocationFieldsOnlyis false andpageSize > 200. The code capslimitto 200 but still computespaginationCountusing the originalpageSize, which can cause clients to stop paging early and miss results.src/models/location.js(findUniqueLocationStubs+searchlocationFieldsOnly path):locationFieldsOnlynow returns rows from araw: trueinclude query, then deduplicates byid. Raw include rows do not provide the nestedServices/EventRelatedInfosstructure expected bysrc/controllers/locations.js(isLocationClosed(...)), soclosedcomputation and response shape can be incorrect.src/models/location.js: deduplicatingrawjoined rows before returning stubs drops multi-row association data (keeps first row per location). In thelocationFieldsOnlyfast-path this can silently remove service/schedule context, creating incorrect API output.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd996a66b4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Revert to using pageSize as before, but capping both it _and_ maxResults (not limit directly) if asking for location details.
There was a problem hiding this comment.
Not safe to approve. The PR changes introduce likely API regressions in locationFieldsOnly responses and closed computation, with no accompanying tests proving compatibility.
Blocking issues
locationFieldsOnlynow returns raw stub rows (src/models/location.js:589) produced by araw: truemulti-join query (src/models/location.js:376-419) instead of hydrated location models. This changes response shape and can expose flattened join columns (for example association-prefixed fields) rather than the prior contract.- Controller logic still expects association arrays (
EventRelatedInfos,Services) to computeclosed(src/controllers/locations.js:266-273), but raw stubs do not provide that structure. This can silently produce incorrectclosedvalues and behavior regressions for occasion-based queries. - Risky behavioral change lacks targeted tests for
locationFieldsOnlyoutput schema andclosedcalculation after the query-path rewrite. Given the response-path change, missing regression tests is blocking.
- Re-add selectedAttributeForOrderBy to findAll, so when it's a computed column it's still selected and therefore exists for WHERE.
There was a problem hiding this comment.
I found blocking regressions in the new locationFieldsOnly flow and query shape changes that can alter API behavior.
Blocking issues
- Broken API behavior for
locationFieldsOnly=true:Location.searchnow returnslocationStubsdirectly whenlocationFieldsOnlyis set, but those stubs come fromfindUniqueLocationStubswithraw: trueand joined includes. Insrc/controllers/locations.js, the formatter expects nestedEventRelatedInfos/Servicesand strips them; with raw rows these are not present as expected, which can breakclosedcalculation and response shape. findUniqueLocationStubsremoved SQL-levelDISTINCTid selection and now fetches broader joined rows (including always-joinedEventRelatedInfoandService.HolidaySchedule) before deduping in memory. This changes result ordering/selection behavior under joins and creates regression risk for pagination/sort correctness inLocation.search(especially when limit/offset are applied after dedupe).- Risky logic changes are not accompanied by targeted tests for: (a)
locationFieldsOnlyresponse structure andclosedderivation, and (b) pagination/sort stability after theDISTINCTremoval and in-memory dedupe path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ffc84ee25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Even when using the `raw: true` query, without associations, use a literal to add a column with the event list from EventRelatedInfos. - Standardize the response structure by returning it as objects resembling the (relevant parts of) EventRelatedInfos.
There was a problem hiding this comment.
The refactor introduces a likely API regression in locationFieldsOnly mode and lacks coverage for the new query/response behavior.
Blocking issues
- Broken API behavior risk in
src/models/location.js+src/controllers/locations.js:Location.searchnow returnslocationStubs(raw rows) directly whenlocationFieldsOnly=true. Those stubs are produced withraw: trueand joined includes, then onlyEventRelatedInfosis reconstructed.Servicesis not reconstructed as an array/object, so downstream logic inlocationscontroller (isLocationClosed(occasion, EventRelatedInfos, Services)) can receive incorrect/empty service data and compute wrongclosedvalues. - Payload shape regression risk for
locationFieldsOnlyresponses: because stubs come fromraw: truejoined queries, response objects may contain flattened association columns instead of the prior model/plain shape. This is a behavior change to a public API path and is not guarded by tests in this PR. - Missing tests for risky logic changes: no tests were added for (a)
locationFieldsOnlyresult shape/closedcomputation after switching to stubs, and (b) the new detailed-results cap behavior (capDetailedLocations) affectinglimit/minResults. Given the scope of query and response-path changes, this is a blocking gap.
Rovack
changed the title
- Remove services from the signature and calls to isLocationClosed. - To still be able to omit it from the returned location objects, configure varsIgnorePattern for eslint. (Doesn't really has to be part of this PR, but both changes are good to have regardless, and should make Codex less confused.)
There was a problem hiding this comment.
Not safe to approve. The PR introduces a likely API behavior regression in the locationFieldsOnly path and lacks coverage for high-risk query/response-shape changes.
Blocking issues
locationFieldsOnlycan now return unintended fields/shape changes. Insrc/models/location.js,findUniqueLocationStubsstill joins broad associations (Organization,PhysicalAddress,Phone,Service, etc.), andLocation.searchnow returns those stubs directly whenlocationFieldsOnlyis true. Insrc/controllers/locations.js, onlyEventRelatedInfosandServicesare stripped, so other joined fields can leak into the response, changing API behavior.- Risky query-path refactor has no accompanying tests in this PR. The move from
findUniqueLocationIdsto rawfindUniqueLocationStubs+ reconstructedEventRelatedInfos+ capped detailed pagination changes response construction and pagination behavior; without tests forlocationFieldsOnlyresponse contract and pagination/order semantics, regression risk is too high.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc827a61e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| attributes: { | ||
| include: [ | ||
| EVENTS_WITH_INFO_COLUMN_SUBQUERY, | ||
| ...(selectedAttributeForOrderBy ? [selectedAttributeForOrderBy] : []), | ||
| ], |
There was a problem hiding this comment.
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 👍 / 👎.
1 participant
Uh oh!
There was an error while loading. Please reload this page.