-
Notifications
You must be signed in to change notification settings - Fork 60
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: cache middleware - OKTA-280090 #218
Conversation
CHANGELOG.md
Outdated
|
||
- [218](https://github.com/okta/okta-sdk-nodejs/pull/218) | ||
- Uses `req.url` as key to cache response | ||
- Addes `json()` function to the default cache middleware response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Adds not Addes
src/http.js
Outdated
@@ -71,7 +71,7 @@ class Http { | |||
|
|||
let retriedOnAuthError = false; | |||
const execute = () => { | |||
const promise = this.prepareRequest(request) | |||
const promise = () => this.prepareRequest(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the variable name? It's now a function that returns a promise, not a promise
await middleware(ctx, () => next(ctx)); | ||
expect(await ctx.res.text()).to.equal(body); | ||
expect(await ctx.res.json()).to.eql(bodyObj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eql()? I couldn't find this matcher in the jest docs, what is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests under test/unit/
are using chai, https://www.chaijs.com/api/bdd/#method_eql
} | ||
|
||
const ctx = { | ||
uri, // TODO: remove unused property. req.url should be the key. | ||
uri, // TODO: remove unused property. req.url should be the key. OKTA-351525 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I think it is a case of a duplicate property. ctx.uri
and ctx.req.url
now hold the same value. The code in default-cache-middleware
was looking for ctx.req.uri
which does not exist and was always undefined. (typescript could have caught this, having a strongly typed context type)
We can choose which value should be used as the cache key. For some reason, which I don't recall, it was chosen to be ctx.req.url
and now ctx.uri
is duplicate/unused. We can keep it around for compatibility, or remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kept for backward compatibility, as the users are able to provide other cache options that depend on the ctx.uri
key.
I created OKTA-351525 to remove it in the next major version update.
@@ -15,15 +15,21 @@ const _ = require('lodash'); | |||
module.exports = function defaultCacheMiddleware(ctx, next) { | |||
let cacheCheck, cacheHit = false; | |||
if (ctx.req.method.toLowerCase() === 'get' && !ctx.isCollection) { | |||
// TODO: use ctx.req.url as cache key and add json() method to cached result: https://oktainc.atlassian.net/browse/OKTA-280090 | |||
cacheCheck = ctx.cacheStore.get(ctx.req.uri) | |||
cacheCheck = ctx.cacheStore.get(ctx.req.url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: ctx.req.uri
was a typo and so the value was always undefined. ctx.req.url
holds a string value for the original uri of the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have better testing. The same typo in the tests (context.req.uri
) meant they passed. Would be great to have better testing for the cache feature. Maybe make two requests with the same URI and verify that only one mock XHR was called.
Looks like you have handled it in test before, I just re-enabled it in this PR, https://github.com/okta/okta-sdk-nodejs/pull/218/files#diff-abdf0a4c5bf82c7aa5f922cead41f12e9e607b9c48bc3692b8b538865323e81dR356 |
No description provided.