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

server: fix bulk add preview caching #1787

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

server: fix bulk add preview caching #1787

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 20 additions & 9 deletions server/infoextractor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
UnsupportedServiceException,
InvalidAddPreviewInputException,
FeatureDisabledException,
VideoNotFoundException,
} from "./exceptions";
import { getLogger } from "./logger";
import { redisClient } from "./redisclient";
Expand Down Expand Up @@ -264,25 +265,35 @@
.filter(p => !video[p]),
}))
.filter(request => request.missingInfo.length > 0);

if (requests.length === 0 && adapter.isCacheSafe) {
return cachedVideos;
}

const fetchedVideos = await adapter.fetchManyVideoInfo(requests);
const finalResults = cachedVideos.map(video => {
const fetchedVideo = fetchedVideos.find(v => v.id === video.id);
if (fetchedVideo) {
return mergeVideo(video, fetchedVideo);
} else {
return video;
}
});
const finalResults = cachedVideos
.map(video => {
const fetchedVideo = fetchedVideos.find(v => v.id === video.id);
const videoKeys = Object.keys(video);
if (fetchedVideo) {
return mergeVideo(video, fetchedVideo);
} else if (videoKeys.length > 2) {
return video;
Comment on lines +279 to +280
Copy link
Owner

Choose a reason for hiding this comment

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

What is this branch here for?

Copy link
Author

Choose a reason for hiding this comment

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

You're right it's still broken. I don't think this approach is correct, I'll revise it.

The branch is to only return the video if the video is found in the cache, since it should contain more than the VideoId fields then.

Copy link
Owner

Choose a reason for hiding this comment

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

This is incorrect behavior then. We need to be able to return videos that were not already in the cache.

} else {
return null;
}

Check warning on line 283 in server/infoextractor.ts

View check run for this annotation

Codecov / codecov/patch

server/infoextractor.ts#L280-L283 

Added lines #L280 - L283 were not covered by tests
})
.filter(v => v !== null) as Video[];

return finalResults;
})
);

const flattened = results.flat();

if (flattened.length === 0) {
throw new VideoNotFoundException();
}

Check warning on line 295 in server/infoextractor.ts

View check run for this annotation

Codecov / codecov/patch

server/infoextractor.ts#L294-L295 

Added lines #L294 - L295 were not covered by tests

// type cast should be safe here because find should always be able to find a video.
const result = videoIds
.map(video => flattened.find(v => v.id === video.id) as Video)
Expand Down
Loading