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: cache middleware - OKTA-280090 #218

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Okta Node SDK Changelog

## 4.4.0

- [218](https://github.com/okta/okta-sdk-nodejs/pull/218)
- Uses `req.url` as key to cache response
- Adds `json()` function to the default cache middleware response

## 4.3.1

### Bug Fixes
Expand Down
10 changes: 8 additions & 2 deletions src/default-cache-middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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.

.then(body => {
if (body) {
cacheHit = true;
ctx.res = {
status: 200,
text() {
return Promise.resolve(body);
},
json() {
try {
return Promise.resolve(JSON.parse(body));
} catch (err) {
return Promise.reject(err);
}
}
};
}
Expand Down
8 changes: 4 additions & 4 deletions src/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class Http {

let retriedOnAuthError = false;
const execute = () => {
const promise = this.prepareRequest(request)
const getRequestPromise = () => this.prepareRequest(request)
.then(() => this.requestExecutor.fetch(request))
.then(Http.errorFilter)
.catch(error => {
Expand All @@ -86,11 +86,11 @@ class Http {
});

if (!this.cacheMiddleware) {
return promise;
return getRequestPromise();
}

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

isCollection: context.isCollection,
resources: context.resources,
req: request,
Expand All @@ -101,7 +101,7 @@ class Http {
return;
}

return promise.then(res => ctx.res = res);
return getRequestPromise().then(res => ctx.res = res);
})
.then(() => ctx.res);
};
Expand Down
2 changes: 1 addition & 1 deletion test/jest/default-request-executor.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ describe('DefaultRequestExecutor', () => {
requestTimeout: 1
});
await expect(requestExecutor.fetch({url: 'https://www.okta.com'})).rejects
.toMatchObject({message:'network timeout at: https://www.okta.com'});
.toMatchObject({message:'network timeout at: https://www.okta.com/'});
});

it('sets the node-fetch timeout to requestTimeout for new requests', async () => {
Expand Down
3 changes: 1 addition & 2 deletions test/jest/http.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,7 @@ describe('Http class', () => {
describe('default cache middleware', () => {
testWithCacheMiddleware();

// TODO: fix cache middleware: https://oktainc.atlassian.net/browse/OKTA-280090
xit('Returns a cached result on subsequent call', () => {
it('Returns a cached result on subsequent call', () => {
const http = new Http({ requestExecutor });
expect(http.cacheMiddleware).toBeDefined();
return http.http('http://fakey.local')
Expand Down
17 changes: 8 additions & 9 deletions test/unit/default-cache-middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,19 @@ async function next(ctx, body = '{}') {
};
}

// TODO: use request.url per fetch spec: https://developer.mozilla.org/en-US/docs/Web/API/Request
// https://oktainc.atlassian.net/browse/OKTA-280090

describe('Default cache middleware', () => {
it('caches GET items with a \'self\' link', async () => {
const cacheStore = new MemoryStore();
const ctx = {
req: {
method: 'get',
uri: 'http://example.com/item'
url: 'http://example.com/item'
},
cacheStore
};
const body = JSON.stringify(_.set({}, '_links.self.href', 'http://example.com/item'));
await middleware(ctx, () => next(ctx, body));
expect(await cacheStore.get(ctx.req.uri)).to.equal(body);
expect(await cacheStore.get(ctx.req.url)).to.equal(body);
// make sure the middleware doesn't flush the stream
expect(await ctx.res.text()).to.equal(body);
});
Expand All @@ -49,14 +46,16 @@ describe('Default cache middleware', () => {
const ctx = {
req: {
method: 'get',
uri: 'http://example.com/item'
url: 'http://example.com/item'
},
cacheStore
};
const body = JSON.stringify(_.set({}, '_links.self.href', 'http://example.com/item'));
await cacheStore.set(ctx.req.uri, body);
const bodyObj = _.set({}, '_links.self.href', 'http://example.com/item');
const bodyStr = JSON.stringify(bodyObj);
await cacheStore.set(ctx.req.url, bodyStr);
await middleware(ctx, () => next(ctx));
expect(await ctx.res.text()).to.equal(body);
expect(await ctx.res.json()).to.eql(bodyObj);
Copy link
Contributor

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?

Copy link
Contributor Author

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

expect(await ctx.res.text()).to.equal(bodyStr);
});

it('doesn\'t cache GET items without a \'self\' link', async () => {
Expand Down