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

Fixed default Model cache key generation issue #1084

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

Jake13f
Copy link
Contributor

@Jake13f Jake13f commented Aug 13, 2024

Support GET & POST requests

The default generator was ignoring variances between GET & POST methods. This caused POST requests to get cached incorrectly. Now, both the request query and body are loaded and merged into a common object and stringified as part of the key generation omitting query params from the url string. This will make the url be the same between GET and POST and allow for the parameters to be common when submitted into the hasher function.

Larger Hash Keys

I also updated the hasher method to use the larger hash size to help prevent collisions since large datasets can be submitted in as part of the caching data. Users of the api can submit in large sets of OBJECTIDS and Geometry as part of query parameters. This should help mitigate any cache collisions.

New Unit Tests

I added some new unit tests to verify that common GET and POST requests use the same cache even though the request urls will be different. (ie. GET will be in the query string and POST will be in the body)

…methods ie. GET & POST

The default generator was ignoring variances between GET & POST methods. This caused POST requests
to get cached incorrectly.
Copy link

changeset-bot bot commented Aug 13, 2024

🦋 Changeset detected

Latest commit: bece09a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@koopjs/koop-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rgwozdz
Copy link
Member

rgwozdz commented Aug 13, 2024

Failures are an issue with CI configuration, not PR changes. Tested locally, passes.

@rgwozdz rgwozdz merged commit 793a97f into koopjs:master Aug 13, 2024
5 of 7 checks passed
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