-
Notifications
You must be signed in to change notification settings - Fork 156
feat(parameters): AppConfigProvider to return the last valid value when the API returns empty value on subsequent calls #1365
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
Changes from 9 commits
0b90940
2d80baa
3fea0d7
838c341
f16d3a5
3aeacb8
d246306
3bde508
6e15a09
ba6e8db
61f6476
3c46d0c
cead3f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -73,10 +73,10 @@ export const handler = async (_event: unknown, _context: Context): Promise<void> | |||||||||||||||
// Test 3 - get a free-form base64-encoded plain text and apply binary transformation (should return a decoded string) | ||||||||||||||||
await _call_get(freeFormBase64encodedPlainText, 'get-freeform-base64-plaintext-binary', { transform: 'binary' }); | ||||||||||||||||
|
||||||||||||||||
// Test 5 - get a feature flag and apply json transformation (should return an object) | ||||||||||||||||
// Test 4 - get a feature flag and apply json transformation (should return an object) | ||||||||||||||||
await _call_get(featureFlagName, 'get-feature-flag-binary', { transform: 'json' }); | ||||||||||||||||
|
||||||||||||||||
// Test 6 | ||||||||||||||||
// Test 5 | ||||||||||||||||
// get parameter twice with middleware, which counts the number of requests, we check later if we only called AppConfig API once | ||||||||||||||||
try { | ||||||||||||||||
providerWithMiddleware.clearCache(); | ||||||||||||||||
|
@@ -94,7 +94,7 @@ export const handler = async (_event: unknown, _context: Context): Promise<void> | |||||||||||||||
}); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// Test 7 | ||||||||||||||||
// Test 6 | ||||||||||||||||
// get parameter twice, but force fetch 2nd time, we count number of SDK requests and check that we made two API calls | ||||||||||||||||
try { | ||||||||||||||||
providerWithMiddleware.clearCache(); | ||||||||||||||||
|
@@ -111,4 +111,24 @@ export const handler = async (_event: unknown, _context: Context): Promise<void> | |||||||||||||||
error: err.message | ||||||||||||||||
}); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// Test 7 | ||||||||||||||||
// get parameter twice, but wait long enough that cache expires count SDK calls and return values | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to reword this to reflect that we are no longer using the delay but instead avoiding cache with |
||||||||||||||||
try { | ||||||||||||||||
providerWithMiddleware.clearCache(); | ||||||||||||||||
middleware.counter = 0; | ||||||||||||||||
const expiredResult1 = await providerWithMiddleware.get(freeFormBase64encodedPlainText, { maxAge: 0, transform: 'base64' }); | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: I've just put the line on multiple lines so that it's not too long |
||||||||||||||||
const expiredResult2 = await providerWithMiddleware.get(freeFormBase64encodedPlainText, { maxAge: 0, transform: 'base64' }); | ||||||||||||||||
logger.log({ | ||||||||||||||||
test: 'get-expired', | ||||||||||||||||
value: middleware.counter, // should be 2 | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any chance that we can keep the original logger.log({
test: 'get-expired',
value: {
counter: middleware.counter,
results: [
expiredResult1,
expiredResult2,
]
},
}); or similar |
||||||||||||||||
result1: expiredResult1, | ||||||||||||||||
result2: expiredResult2 | ||||||||||||||||
}); | ||||||||||||||||
} catch (err) { | ||||||||||||||||
logger.log({ | ||||||||||||||||
test: 'get-expired', | ||||||||||||||||
error: err.message | ||||||||||||||||
}); | ||||||||||||||||
} | ||||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
* @group unit/parameters/AppConfigProvider/class | ||
*/ | ||
import { AppConfigProvider } from '../../src/appconfig/index'; | ||
import { ExpirableValue } from '../../src/ExpirableValue'; | ||
import { AppConfigProviderOptions } from '../../src/types/AppConfigProvider'; | ||
import { | ||
AppConfigDataClient, | ||
|
@@ -225,6 +226,51 @@ describe('Class: AppConfigProvider', () => { | |
// Act & Assess | ||
await expect(provider.get(name)).rejects.toThrow(); | ||
}); | ||
|
||
test('when session returns an empty configuration on the second call, it returns the last value', async () => { | ||
|
||
client.reset(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only test using this reset. We should possibly add a Maybe we can do this for this file, and then possibly in a future PR we can also apply the change to all other tests under What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, that works for me. I added it here to be surgical - the |
||
|
||
// Prepare | ||
const options: AppConfigProviderOptions = { | ||
application: 'MyApp', | ||
environment: 'MyAppProdEnv', | ||
}; | ||
const provider = new AppConfigProvider(options); | ||
const name = 'MyAppFeatureFlag'; | ||
|
||
const fakeInitialToken = 'aW5pdGlhbFRva2Vu'; | ||
const fakeNextToken1 = 'bmV4dFRva2Vu'; | ||
const fakeNextToken2 = 'bmV4dFRva2Vq'; | ||
const mockData = encoder.encode('myAppConfiguration'); | ||
|
||
client | ||
.on(StartConfigurationSessionCommand) | ||
.resolves({ | ||
InitialConfigurationToken: fakeInitialToken, | ||
}) | ||
.on(GetLatestConfigurationCommand) | ||
.resolvesOnce({ | ||
Configuration: mockData, | ||
NextPollConfigurationToken: fakeNextToken1, | ||
}) | ||
.resolvesOnce({ | ||
Configuration: undefined, | ||
NextPollConfigurationToken: fakeNextToken2, | ||
}); | ||
|
||
// Act | ||
|
||
// Load local cache | ||
const result1 = await provider.get(name, { forceFetch: true }); | ||
|
||
// Read from local cache, given empty response from service | ||
const result2 = await provider.get(name, { forceFetch: true }); | ||
|
||
// Assess | ||
expect(result1).toBe(mockData); | ||
expect(result2).toBe(mockData); | ||
}); | ||
}); | ||
|
||
describe('Method: _getMultiple', () => { | ||
|
@@ -243,3 +289,48 @@ describe('Class: AppConfigProvider', () => { | |
}); | ||
}); | ||
}); | ||
|
||
describe('Class: ExpirableValue', () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
describe('Method: constructor', () => { | ||
test('when created, it has ttl set to at least maxAge seconds from test start', () => { | ||
// Prepare | ||
const seconds = 10; | ||
const nowTimestamp = Date.now(); | ||
const futureTimestampSeconds = nowTimestamp/1000+(seconds); | ||
|
||
// Act | ||
const expirableValue = new ExpirableValue('foo', seconds); | ||
|
||
// Assess | ||
expect(expirableValue.ttl).toBeGreaterThan(futureTimestampSeconds); | ||
}); | ||
}); | ||
|
||
describe('Method: isExpired', () => { | ||
test('when called, it returns true when maxAge is in the future', () => { | ||
// Prepare | ||
const seconds = 60; | ||
|
||
// Act | ||
const expirableValue = new ExpirableValue('foo', seconds); | ||
|
||
// Assess | ||
expect(expirableValue.isExpired()).toBeFalsy(); | ||
}); | ||
|
||
test('when called, it returns false when maxAge is in the past', () => { | ||
// Prepare | ||
const seconds = -60; | ||
|
||
// Act | ||
const expirableValue = new ExpirableValue('foo', seconds); | ||
|
||
// Assess | ||
expect(expirableValue.isExpired()).toBeTruthy(); | ||
}); | ||
}); | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.