Fix the fetchFullItems race condition

[aasandei-vsp]

web-app/src/app/shared/services/data/data.service.ts

Lines 287 to 401 in 16e8da1

	public async fetchFullItems(items: Array<ItemVO>, withChildren?: boolean) {
	this.debug('fetchFullItems %d items requested', items.length);
	const itemResolves = [];
	const itemRejects = [];
	const records: Array<any | RecordVO> = [];
	const folders: Array<any | FolderVO> = [];
	items.forEach((item) => {
	item.isFetching = true;
	item.fetched = new Promise((resolve, reject) => {
	itemResolves.push(resolve);
	itemRejects.push(reject);
	});
	if (item.isRecord) {
	records.push(item);
	} else {
	folders.push(item);
	}
	});
	const promises: Promise<any>[] = [];
	promises.push(
	records.length ? this.api.record.get(records) : Promise.resolve(),
	);
	if (withChildren) {
	promises.push(
	folders.length
	? this.api.folder.getWithChildren(folders)
	: Promise.resolve(),
	);
	} else {
	promises.push(
	folders.length ? this.api.folder.get(folders) : Promise.resolve(),
	);
	}
	return await Promise.all(promises)
	.then(async (results) => {
	const recordResponse: RecordResponse = results[0];
	const folderResponse: FolderResponse = results[1];
	let fullRecords: Array<any | RecordVO>;
	let fullFolders: Array<any | FolderVO>;
	if (recordResponse) {
	fullRecords = recordResponse.getRecordVOs();
	}
	if (folderResponse) {
	fullFolders = folderResponse.getFolderVOs();
	}
	for (let i = 0; i < records.length; i += 1) {
	records[i].update(fullRecords[i]);
	records[i].dataStatus = DataStatus.Full;
	this.tags.checkTagsOnItem(records[i]);
	}
	for (let i = 0; i < folders.length; i += 1) {
	const folder = folders[i] as FolderVO;
	folder.update(
	fullFolders[i] as FolderVOData,
	folders[i] === this.currentFolder,
	);
	folder.dataStatus = DataStatus.Full;
	this.tags.checkTagsOnItem(folders[i]);
	}
	itemResolves.forEach((resolve, index) => {
	items[index].fetched = null;
	this.byArchiveNbr[items[index].archiveNbr] = items[index];
	resolve();
	});
	this.debug('fetchFullItems %d items fetched', items.length);
	return await Promise.resolve(true);
	})
	.catch(() => {
	itemRejects.forEach((reject, index) => {
	items[index].fetched = null;
	reject();
	});
	});
	}
	public async refreshCurrentFolder(sortOnly = false) {
	this.debug('refreshCurrentFolder (sortOnly = %o)', sortOnly);
	return await this.api.folder
	.navigate(this.currentFolder)
	.pipe(
	map((response: FolderResponse) => {
	this.debug('refreshCurrentFolder data fetched', sortOnly);
	if (!response.isSuccessful) {
	throw response;
	}
	return response.getFolderVO(true);
	}),
	)
	.toPromise()
	.then((updatedFolder: FolderVO) => {
	this.updateChildItems(this.currentFolder, updatedFolder, sortOnly);
	this.hideItemsInCurrentFolder();
	this.debug('refreshCurrentFolder done', sortOnly);
	this.folderUpdate.emit(this.currentFolder);
	this.currentHiddenItems = [];
	});
	}

The method fetchFullItems in the data service chains multiple promises, which normally wouldn't be a problem. But in this case, we do not get the modified records at the end of chaining the promises, we actually just modify the reference to the records list we sent as an argument and this implementation yields unpredictable results.

Ideally, we should refactor the method, so the return is what we rely on, not a reference we happen to pass as an argument.

The issue here is that refactoring this method is high risk, low gain. It is used in several places and it does seem peculiar that it did not yield more bugs, taking into account that the results are unpredictable. Probably because it is being used in async flows and in the view.