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

fix: obs search reqs w/ spatial params should not return cached data #457

Merged
merged 3 commits into from
Sep 15, 2024

Conversation

kueda
Copy link
Member

@kueda kueda commented Sep 13, 2024

The symptom was that iNat Next was showing global results in Explore when it should have been showing Nearby... sometimes and not others.

I'm not sure if this is actually what we want (no caching for these params), but this will fix the bug. If we do want caching with those params, that wasn't working either, but it's a simple fix.

@kueda kueda changed the title fix: add more spatial params to obs search cache key fix: obs search reqs w/ spatial params should not return cached data Sep 14, 2024
@kueda kueda requested a review from pleary September 14, 2024 00:51
@@ -491,16 +491,12 @@ const util = class util {
if ( queryDup.order_by === "" ) {
delete queryDup.order_by;
}
if ( _.isEmpty( queryDup.lat ) && _.isEmpty( queryDup.lat ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe the root cause of the new symptom is that for v2, parameters are coerced into their defined data type and not all strings coming from the request parameter string as is the case with v1. The lodash _.isEmpty method considers numbers to be empty, resulting in v2 requests with lat/lng having their lat/lng ignored when generating a cache key. All else looks good, so I'll merge this but might restore this bit adding in an additional check of && !_.isNumber

@pleary pleary merged commit ed2e7e4 into main Sep 15, 2024
7 checks passed
@pleary pleary deleted the obs-search-cache-key-fixes branch September 15, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants