feat(translation): introduce cache busting logic #4240

IgnacioBecerra · 2020-10-15T22:23:15Z

Related Ticket(s)

Description

Added a new timestamp property within the response json to compare with the current time. If the timestamp is older than two hours, a new response will be fetched with a new, current timestamp.

Also implemented tests for the new methods.

Changelog

New

timestamp property added to the response json for cache busting purposes

ibmdotcom-bot · 2020-10-15T22:40:47Z

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/4240/index.html

Built with commit: f8128e76a2a42533c1edfb2959e24b50056ba639

ibmdotcom-bot · 2020-10-15T22:46:39Z

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/4240/index.html

Built with commit: f8128e76a2a42533c1edfb2959e24b50056ba639

asudoh · 2020-10-16T01:14:51Z

Interesting change @IgnacioBecerra - Seems that the code adds timestamp for each cache data item so you wanted to manage cache TTL per-item? Note that clearCache() removes all cache data items.

IgnacioBecerra · 2020-10-16T15:10:35Z

That's the idea, add a timestamp to the cached item, and before using it again, check if it's been longer than two hours. If so, it expires and we retrieve the newest response + update the timestamp. I'll keep that clearcache() in mind, thanks!

asudoh

Great to see this is ready for review @IgnacioBecerra!

asudoh · 2020-10-19T23:29:26Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    );
+
+    // reinitializing import
+    const TranslationAPI = (await import('../Translation')).default;


I won't block proceeding this PR with my comment here, but I'd rather see resetting things done via mocks. The behavior of dynamic import tends to be dependent on build toolchain and we may have high chance to get an undefined behavior if it's used as a mean for "resetting".

Ah I see, I'll be applying the same logic for Locale API, so I'll use mocks there and also add it here. Thanks!

Any update here...? Thanks!

asudoh · 2020-10-19T23:30:39Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    const timeDiff = response.timestamp - previousSession.timestamp,
+      _twoHours = 60 * 60 * 2000;


Would you explain what makes the cache elapsed time here greater than two hours?

Sure! As an example, the cached time in previousSession is 1337 (a very old timestamp), while response would have the current time as 1603206344. By doing 1603206344 - 1337 we receive 160319297, which is greater than 7200000 (two hours). This is to ensure that the cache busting code runs for this test, as it is required for the time difference to be greater than two hours.

If the previous session timestamp was much closer in time to the current one, the difference would be less than two hours, and the response received would remain unchanged from the previous session.

Thank you for your answer @IgnacioBecerra! I was asking where in the code or in test that makes timeDiff greater than two hours, given the test says "timeDiff should be greater than two hours". What code sets response.timestamp and what code sets previousSession.timestamp? Did you make the timestamp deterministic by e.g. mocks (rather than relying on system time that is non-deterministic)?

Ah I see what you mean! I set previousSession in line 9 using import oldSession from './data/timestamp_response.json';.

timestamp_response.json uses the same data in ./data/response.json, with the only difference is that it includes timestamp: 1337. This would be the only deterministic timestamp. On the other hand, response always contains the current timestamp from the latest response received from TranslationAPI.

Cool thanks @IgnacioBecerra! Given your response, given we know the timestamps come from mock, why do we need to test the two timestamps between the two mocks? Or was it simply for ensuring we are picking up the right (fresh) data?

It's the latter, I wanted to make extra sure the logic was working. Since we tested the fresh data in sessionStorage was the same as in the response, I used the mock to compare times, and had to check if timeDiff was greater than two hours.

I see, thank you for clarifying @IgnacioBecerra! I suggest more direct way for testing if we pick up the right data. One approach can be object ref. Another approach may be unique key/ID.

ibmdotcom-bot · 2020-10-19T23:31:57Z

Deploy preview created for package "Services":
https://ibmdotcom-services.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/4240/index.html

Built with commit: f8128e76a2a42533c1edfb2959e24b50056ba639

asudoh

Some good improvements @IgnacioBecerra!

asudoh · 2020-10-22T09:57:30Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    const mockDate = new Date('2019');
+    const _Date = Date;
+    global.Date = jest.fn(() => mockDate);
+    global.Date.now = _Date.now;


Would you try: https://jestjs.io/docs/en/snapshot-testing#2-tests-should-be-deterministic

I tried using that and many other variations before using this approach, but unfortunately this one was the only one that actually had any effect on response. Not sure what is causing it though, the implementations don't differ much from another.

asudoh · 2020-10-22T09:58:01Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+  it('timestamp should not change if within two hours', async () => {
+    // using recent cached session
+    sessionStorageMock.setItem(
+      'dds-translation-us-en',
+      JSON.stringify(responseSuccess)
+    );
+
+    const previousSession = JSON.parse(
+      sessionStorageMock.getItem('dds-translation-us-en')
+    );
+
+    // reinitializing import
+    const TranslationAPI = (await import('../Translation')).default;
+    const response = await TranslationAPI.getTranslation({
+      lc: 'en',
+      cc: 'us',
+    });
+
+    // timestamp should remain unchanged
+    expect(response.timestamp).toEqual(previousSession.timestamp);
+  });


Do you have some details on what this case tests?

We use responseSuccess that was just recently refreshed as the cached data. Since it's very recent (just ran), then running the API again shouldn't change the cached data again -- it simply retrieves it. Here we just check if the timestamp remains the same from the previous session.

I see, given we don't (and won't) change the timestamp of existing cache, probably we wanted to simply see if the unique ID of response equals to one of previousSession?

asudoh

Thank you for your response to my earlier question @IgnacioBecerra!

packages/services/src/services/Translation/Translation.js

packages/services/src/services/Translation/__tests__/Translation.test.js

asudoh · 2020-10-26T12:38:16Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+  it('timestamp should not change if within two hours', async () => {
+    // using recent cached session
+    sessionStorageMock.setItem(
+      'dds-translation-us-en',
+      JSON.stringify(responseSuccess)
+    );
+
+    const previousSession = JSON.parse(
+      sessionStorageMock.getItem('dds-translation-us-en')
+    );
+
+    // reinitializing import
+    const TranslationAPI = (await import('../Translation')).default;
+    const response = await TranslationAPI.getTranslation({
+      lc: 'en',
+      cc: 'us',
+    });
+
+    // timestamp should remain unchanged
+    expect(response.timestamp).toEqual(previousSession.timestamp);
+  });


I see, given we don't (and won't) change the timestamp of existing cache, probably we wanted to simply see if the unique ID of response equals to one of previousSession?

IgnacioBecerra · 2020-10-26T19:47:28Z

@asudoh The unique ID for responseSuccess is TRANSLATION_FRESH, given that it was just created. If it's been requested again within two hours, it changes to TRANSLATION_UNCHANGED (changed it from TRANSLATION_OLD).

Since it's the only thing that differs between these two responses, I checked the ID change, deleted the IDs from their jsons, and compared the rest in the latest commit. Also applied your suggestions, thanks!

jeffchew · 2020-10-26T20:54:41Z

@IgnacioBecerra did something happen to this branch? There's now 94 changed files.

IgnacioBecerra · 2020-10-26T21:02:57Z

@jeffchew I merged master into this branch to resolved the merge conflict, so the changed files should be from all the recent commits from master. I hadn't noticed the 94 files, but I think there's probably a better way to do this

jeffchew · 2020-10-26T21:06:25Z

@jeffchew I merged master into this branch to resolved the merge conflict, so the changed files should be from all the recent commits from master. I hadn't noticed the 94 files, but I think there's probably a better way to do this

It shouldn't be this many files, just the ones you changed so I think the merge might not have happened correctly.

IgnacioBecerra · 2020-10-26T21:26:18Z

@jeffchew Resolved the issue! Back to only three files changed.

asudoh · 2020-10-26T23:07:20Z

@asudoh The unique ID for responseSuccess is TRANSLATION_FRESH, given that it was just created. If it's been requested again within two hours, it changes to TRANSLATION_UNCHANGED (changed it from TRANSLATION_OLD).

Could you fix this? Given we set the unique ID solely for testing purpose and we use it for the cache record identify, the ID being changed won't make sense. Thanks!

asudoh

Thank you for the updates @IgnacioBecerra!

asudoh · 2020-10-26T23:41:40Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    );
+
+    // reinitializing import
+    const TranslationAPI = (await import('../Translation')).default;


Any update here...? Thanks!

asudoh · 2020-10-26T23:43:06Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    // timestamps should have at least a two hour difference
+    const timeDiff = response.timestamp - previousSession.timestamp,
+      _twoHours = 60 * 60 * 2000;
+    expect(timeDiff).toBeGreaterThan(_twoHours);


Per discussion, these lines are no longer needed:

Suggested change

// timestamps should have at least a two hour difference

const timeDiff = response.timestamp - previousSession.timestamp,

_twoHours = 60 * 60 * 2000;

expect(timeDiff).toBeGreaterThan(_twoHours);

asudoh

Thank you for the updates @IgnacioBecerra!

asudoh · 2020-10-27T03:13:08Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    },
+    setItem(key, value) {
+      value = JSON.parse(value);
+      value['id'] = 'TRANSLATION_FRESH';


The ID is better set from test cases:

Suggested change

value['id'] = 'TRANSLATION_FRESH';

asudoh · 2020-10-27T03:16:33Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    const previousSession = JSON.parse(
+      sessionStorageMock.getItem('dds-translation-us-en')
+    );
+
+    expect(previousSession.id).toEqual('TRANSLATION_FRESH');


Given we are not testing sessionStorageMock here, I don't think this is needed:

Suggested change

const previousSession = JSON.parse(

sessionStorageMock.getItem('dds-translation-us-en')

);

expect(previousSession.id).toEqual('TRANSLATION_FRESH');

asudoh · 2020-10-27T03:21:35Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    // using very old cached session
+    sessionStorageMock.setItem(
+      'dds-translation-us-en',
+      JSON.stringify(oldSession)


Suggested change

JSON.stringify(oldSession)

JSON.stringify(Object.assign(oldSession, { id: uniqueId }))

asudoh · 2020-10-27T03:23:21Z

packages/services/src/services/Translation/__tests__/Translation.test.js

    expect(response).toEqual(responseSuccess);
  });
+
+  it('should return a json with a recent timestamp', async () => {
+    const mockDate = 1546300800000; // Epoch time of January 1, 2019 midnight UTC


Also please refer to how the ID is set via Object.assign():

Suggested change

const mockDate = 1546300800000; // Epoch time of January 1, 2019 midnight UTC

const uniqueId = `__translation_cache__${Math.random().toString(36).slice(2)}`;

const mockDate = 1546300800000; // Epoch time of January 1, 2019 midnight UTC

asudoh · 2020-10-27T03:27:08Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+      sessionStorageMock.getItem('dds-translation-us-en')
+    );
+
+    response['id'] = newSession.id;


It seems that we are testing if the data comes from the cache. If so, the cache should have its ID assigned already and thus we don't set one to response:

Suggested change

response['id'] = newSession.id;

asudoh · 2020-10-27T03:29:08Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+    // newest response and storage data should match
+    expect(response).toEqual(newSession);
+
+    // should contain timestamp
+    expect(response).toHaveProperty('timestamp');
+
+    // should equal mock timestamp
+    expect(response.timestamp).toEqual(mockDate);
+    expect(response.id).toEqual('TRANSLATION_FRESH');


It seems that we are testing if the data comes from the cache. If so, ID check is the only assertion we need:

Suggested change

// newest response and storage data should match

expect(response).toEqual(newSession);

// should contain timestamp

expect(response).toHaveProperty('timestamp');

// should equal mock timestamp

expect(response.timestamp).toEqual(mockDate);

expect(response.id).toEqual('TRANSLATION_FRESH');

expect(response.id).toEqual(uniqueId);

asudoh · 2020-10-27T03:30:39Z

packages/services/src/services/Translation/__tests__/Translation.test.js

+  it('timestamp should not change if within two hours', async () => {
+    // using recent cached session
+    sessionStorageMock.setItem(
+      'dds-translation-us-en',
+      JSON.stringify(responseSuccess)
+    );
+
+    const previousSession = JSON.parse(
+      sessionStorageMock.getItem('dds-translation-us-en')
+    );
+
+    const response = await TranslationAPI.getTranslation({
+      lc: 'en',
+      cc: 'us',
+    });
+
+    // body and timestamp should remain unchanged
+    expect(response).toEqual(previousSession);
+  });


Given we don't have any code to change the timestamp of cache, this test case seems not necessary:

Suggested change

it('timestamp should not change if within two hours', async () => {

// using recent cached session

sessionStorageMock.setItem(

'dds-translation-us-en',

JSON.stringify(responseSuccess)

);

const previousSession = JSON.parse(

sessionStorageMock.getItem('dds-translation-us-en')

);

const response = await TranslationAPI.getTranslation({

lc: 'en',

cc: 'us',

});

// body and timestamp should remain unchanged

expect(response).toEqual(previousSession);

});

asudoh

LGTM 👍 - Thanks @IgnacioBecerra for all the updates! 🎉

### Related Ticket(s) #3959 ### Description As done in [TranslationAPI](#4240), this PR adds a new timestamp property within the response json to compare with the current time. If the timestamp is older than two hours, a new response will be fetched with a new, current timestamp. Also implemented tests for the new methods. ### Changelog **New** - `timestamp` property added to the response json for cache busting purposes

feat(translation): introduce cache busting logic

adcbe6f

IgnacioBecerra added 3 commits October 16, 2020 16:55

feat(translation): updated cache busting logic

7ec46be

feat(translation): adding tests

20563b1

feat(translation): implemented tests for cache busting logic

4a69b12

IgnacioBecerra added hacktoberfest-accepted package: services Work necessary for the Carbon for IBM.com services package labels Oct 19, 2020

Merge branch 'master' into translation-cache-busting

8ec32fe

IgnacioBecerra marked this pull request as ready for review October 19, 2020 23:05

IgnacioBecerra requested review from jeffchew, asudoh, annawen1, ariellalgilmore, kennylam, raphaelamadeu-zz and andysherman2121 October 19, 2020 23:06

asudoh reviewed Oct 19, 2020

View reviewed changes

IgnacioBecerra added 2 commits October 21, 2020 13:59

feat(translation): date mock testing

7bfe950

feat(translation): embedded unique ID to response

fc025e3

asudoh reviewed Oct 22, 2020

View reviewed changes

asudoh reviewed Oct 26, 2020

View reviewed changes

feat(translation): resolved conflict and merge

ccb566c

IgnacioBecerra force-pushed the translation-cache-busting branch from f840e8d to ccb566c Compare October 26, 2020 21:23

IgnacioBecerra added 2 commits October 26, 2020 14:23

feat(translation): changed string

d18b6ee

Merge branch 'master' into translation-cache-busting

07c7970

feat(translation): removed changing id

3246da0

asudoh reviewed Oct 26, 2020

View reviewed changes

IgnacioBecerra added 2 commits October 26, 2020 19:26

feat(translation): removed dynamic imports

0484e64

feat(translation): removed unique id from production

84670da

asudoh reviewed Oct 27, 2020

View reviewed changes

feat(translation): added cache property

9188268

asudoh approved these changes Oct 27, 2020

View reviewed changes

Merge branch 'master' into translation-cache-busting

e3cb32d

jeffchew added the Ready to merge Label for the pull requests that are ready to merge label Oct 27, 2020

Merge branch 'master' into translation-cache-busting

f8128e7

kodiakhq bot merged commit 3aa6800 into carbon-design-system:master Oct 27, 2020

IgnacioBecerra mentioned this pull request Oct 28, 2020

feat(locale): introducing cache busting logic #4325

Merged

jeffchew mentioned this pull request Nov 10, 2020

November 2020 Release Plan (v1.13.0) #3598

Closed

		const timeDiff = response.timestamp - previousSession.timestamp,
		_twoHours = 60 * 60 * 2000;

	JSON.stringify(oldSession)
	JSON.stringify(Object.assign(oldSession, { id: uniqueId }))

	const mockDate = 1546300800000; // Epoch time of January 1, 2019 midnight UTC
	const uniqueId = `__translation_cache__${Math.random().toString(36).slice(2)}`;
	const mockDate = 1546300800000; // Epoch time of January 1, 2019 midnight UTC

feat(translation): introduce cache busting logic #4240

feat(translation): introduce cache busting logic #4240

Conversation

IgnacioBecerra commented Oct 15, 2020 • edited Loading

Related Ticket(s)

Description

Changelog

ibmdotcom-bot commented Oct 15, 2020 • edited Loading

ibmdotcom-bot commented Oct 15, 2020 • edited Loading

asudoh commented Oct 16, 2020

IgnacioBecerra commented Oct 16, 2020

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgnacioBecerra Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibmdotcom-bot commented Oct 19, 2020 • edited Loading

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgnacioBecerra commented Oct 26, 2020

jeffchew commented Oct 26, 2020

IgnacioBecerra commented Oct 26, 2020 • edited Loading

jeffchew commented Oct 26, 2020

IgnacioBecerra commented Oct 26, 2020

asudoh commented Oct 26, 2020

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

IgnacioBecerra commented Oct 15, 2020 •

edited

Loading

ibmdotcom-bot commented Oct 15, 2020 •

edited

Loading

ibmdotcom-bot commented Oct 15, 2020 •

edited

Loading

IgnacioBecerra Oct 20, 2020 •

edited

Loading

ibmdotcom-bot commented Oct 19, 2020 •

edited

Loading

IgnacioBecerra commented Oct 26, 2020 •

edited

Loading