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.

Sign up for GitHub

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: Only request vendorlist once #209

Merged
merged 6 commits into from
Oct 2, 2018
Merged

fix: Only request vendorlist once #209

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3682721
fix: Only request vendorlist once
Fumler Sep 6, 2018
e77dab6
refactor: Remove console.log from test
Fumler Sep 6, 2018
9a0693c
refactor: Simplify checking for cached vendorlist
Fumler Sep 7, 2018
9fff479
remove comments and add semicolon
phogel Sep 10, 2018
24ed51d
refactor: Change tests to make sure we do not have cached version
Fumler Sep 27, 2018
4dc66cd
fix: Resolve promise instead of return
Fumler Oct 2, 2018
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
8 changes: 7 additions & 1 deletion src/scripts/core/core_vendor_information.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,25 @@ export const DEFAULT_VENDOR_LIST = {
};

export let cachedVendorList;
export let pendingVendorlistPromise = null;

export function loadVendorList() {
return new Promise(function (resolve) {
if (cachedVendorList) {
resolve(cachedVendorList);
} else if (pendingVendorlistPromise) {
return pendingVendorlistPromise;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to check if this works correctly. I think this doesn't return function loadVendorList() but the inner anonymous function (resolve) {} unresolved instead. I would expect unexpected behavior where loadVendorList() is used. I think you want to return an unnested promise instead that behaves exactly like all the others.

Copy link
Contributor

@marcelb marcelb Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably refactor the whole function that the whole scope isn't inside that huge Promise return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. The idea is:

  • On the first call to loadVendorList we return a promise, that when finished, will resolve to the vendorlist data.
  • On the second call to loadVendorList we also return a promise, that when finished, will resolve to the vendorlist data, as long as there is no cached version, and we have already started a promise
  • On the third call to loadVendorList we also return a promise, that when finished, will resolve to the vendorlist data, as long as there is no cached version, and we have already started a promise
  • .. etc

The second and third cases will only return the started promise if it exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"- On the second call to loadVendorList we also return a promise, that when finished, will resolve to the vendorlist data, as long as there is no cached version, and we have already started a promise"

Thats the one im not sure that it works as intended. There is a return statement returning the promise inside another promise (line 16). Not resolving, returning. (line 19)

Copy link
Contributor

@marcelb marcelb Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But anyway, the tests are looking fine and should fail, if its actually broken. Thank you!
The change makes a lot of sense, thank you for the contribution and sorry for the long delay!

Im not entirely sure yet though and can't create a new version today anymore and will be on a 1 day vacation tomorrow. @ltparis2018 @tbtz Can you have a look as well and merge it tomorrow? If not I'll be back on monday and finish this up. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im on it again :) I think we will be merging it soon, but we might do a small refactoring on it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fumler I did some tests on the code and I believe its still broken.

The test can't catch the errors though, because its also flawed. The tests done() function is executed before the Promises can resolve, so all the expects within this test wont run at all.
A quick fix is to move the done() in the last async method, which results in a failed test (because the last loadVendorList() never resolves).

There are two bugs within the function. One being returning the pendingVendorlistPromise instead of resolving it and the second one is the call to fetchJsonData which doesn't set the pendingVendorlistPromise variable probably. It resolves the right side first, resulting in a wrong value in the variable.

The test also assumes implicit timings of the code to work, which seems to be ok though, not raising any race conditions.

Please fix those remaining issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should rather read
resolve(pendingVendorlistPromise);

This of course results in nested Promises, which works.

} else {
let iabVendorListUrl = getIabVendorListUrl();
fetchJsonData(iabVendorListUrl)
pendingVendorlistPromise = fetchJsonData(iabVendorListUrl)
.then(response => {
cachedVendorList = response;
pendingVendorlistPromise = null;
sortVendors(cachedVendorList);
resolve(cachedVendorList);
})
.catch(error => {
pendingVendorlistPromise = null;
logError(`OIL getVendorList failed and returned error: ${error}. Falling back to default vendor list!`);
resolve(getVendorList());
});
Expand Down Expand Up @@ -67,6 +72,7 @@ export function getVendorList() {

export function clearVendorListCache() {
cachedVendorList = undefined;
pendingVendorlistPromise = null;
}

export function getVendorsToDisplay() {
Expand Down
35 changes: 32 additions & 3 deletions test/specs/core/core_vendor_information.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import {
getVendors,
getLimitedVendors,
getVendorsToDisplay,
loadVendorList
loadVendorList,
cachedVendorList,
pendingVendorlistPromise
} from '../../../src/scripts/core/core_vendor_information';
import VENDOR_LIST from '../../fixtures/vendorlist/simple_vendor_list.json';
import { resetOil } from '../../test-utils/utils_reset';
Expand Down Expand Up @@ -52,6 +54,33 @@ describe('core_vendor_information', () => {
});
});

it('should wait for cached vendor list if request is already started', (done) => {
let fetchSpy = spyOn(CoreUtils, 'fetchJsonData').and.returnValue(new Promise((resolve) => resolve(VENDOR_LIST)));
spyOn(CoreConfig, 'getIabVendorListUrl').and.returnValue("https://iab.vendor.list.url");

expect(pendingVendorlistPromise).toBeNull();
expect(cachedVendorList).toBeUndefined();
loadVendorList().then((retrievedVendorList) => {
expect(retrievedVendorList.vendorListVersion).toEqual(VENDOR_LIST.vendorListVersion);
expect(retrievedVendorList).toEqual(VENDOR_LIST);
expect(cachedVendorList).toBeDefined();
});
expect(cachedVendorList).toBeUndefined();
expect(pendingVendorlistPromise).toBeDefined();
loadVendorList().then((retrievedVendorList) => {
expect(retrievedVendorList.vendorListVersion).toEqual(VENDOR_LIST.vendorListVersion);
expect(retrievedVendorList).toEqual(VENDOR_LIST);
});
expect(cachedVendorList).toBeUndefined();
loadVendorList().then((retrievedVendorList) => {
expect(retrievedVendorList.vendorListVersion).toEqual(VENDOR_LIST.vendorListVersion);
expect(retrievedVendorList).toEqual(VENDOR_LIST);
});
expect(fetchSpy.calls.count()).toBe(1);

done();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This done() should be on line 77 instead to make the test work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

});

it('should use default vendor list if vendor list fetching fails', (done) => {
spyOn(CoreUtils, 'fetchJsonData').and.returnValue(new Promise((resolve, reject) => reject(new Error("something went wrong"))));
spyOn(CoreConfig, 'getIabVendorListUrl').and.returnValue("https://iab.vendor.list.url");
Expand Down Expand Up @@ -273,7 +302,7 @@ describe('core_vendor_information', () => {
});

describe('getLimitedVendors', function() {

it('returns regular vendors when no whitelist or blacklist exists', function() {
spyOn(CoreConfig, 'getShowLimitedVendors').and.returnValue(true);
expect(getLimitedVendors().length).toEqual(DEFAULT_VENDOR_LIST.maxVendorId);
Expand All @@ -294,7 +323,7 @@ describe('core_vendor_information', () => {
});

describe('getVendorsToDisplay', function() {

it('should return full vendor list when configuration parameter show_limited_vendors_only is false', function() {
spyOn(CoreConfig, 'getShowLimitedVendors').and.returnValue(false);
let result = getVendorsToDisplay();
Expand Down