Fix raw GitHub URL being output to content (#2147)

* fix thumbnail quality

* Revert "fix(git-gateway): fix previews for GitHub images not in Large Media (#2125)"

This reverts commit d17f896f479292db06d3a4b39f2e51b6c41101bd.

* wip

* Stop using thunks to load media display URLs

* Revert changes to dev-test

* Revert changes to large media docs

* fix lint error

* Update docs to point to the upcoming version with non-broken media
This commit is contained in:
Shawn Erquhart
2019-03-07 21:28:14 -05:00
committed by Benaiah Mischenko
parent 40df666151
commit 37138834d6
12 changed files with 161 additions and 115 deletions

View File

@ -219,23 +219,24 @@ export default class Bitbucket {
}
getMedia() {
const sem = semaphore(MAX_CONCURRENT_DOWNLOADS);
return this.api
.listAllFiles(this.config.get('media_folder'))
.then(files =>
files.map(({ id, name, path }) => ({ id, name, path, displayURL: { id, path } })),
);
}
return this.api.listAllFiles(this.config.get('media_folder')).then(files =>
files.map(({ id, name, path }) => {
const getDisplayURL = () =>
new Promise((resolve, reject) =>
sem.take(() =>
this.api
.readFile(path, id, { parseText: false })
.then(blob => URL.createObjectURL(blob))
.then(resolve, reject)
.finally(() => sem.leave()),
),
);
return { id, name, getDisplayURL, path };
}),
getMediaDisplayURL(displayURL) {
this._mediaDisplayURLSem = this._mediaDisplayURLSem || semaphore(MAX_CONCURRENT_DOWNLOADS);
const { id, path } = displayURL;
return new Promise((resolve, reject) =>
this._mediaDisplayURLSem.take(() =>
this.api
.readFile(path, id, { parseText: false })
.then(blob => URL.createObjectURL(blob))
.then(resolve, reject)
.finally(() => this._mediaDisplayURLSem.leave()),
),
);
}

View File

@ -214,16 +214,19 @@ export default class GitGateway {
if (!largeMediaClient.enabled) {
return mediaFiles;
}
const largeMediaURLThunks = await this.getLargeMedia(mediaFiles);
return mediaFiles.map(({ id, url, urlIsPublicPath, getDisplayURL, ...rest }) => ({
...rest,
id,
url,
urlIsPublicPath: largeMediaURLThunks[id] ? false : urlIsPublicPath,
getDisplayURL: largeMediaURLThunks[id]
? largeMediaURLThunks[id]
: getDisplayURL || (url && (() => Promise.resolve(url))),
}));
const largeMediaDisplayURLs = await this.getLargeMediaDisplayURLs(mediaFiles);
return mediaFiles.map(({ id, displayURL, path, ...rest }) => {
return {
...rest,
id,
path,
displayURL: {
path,
original: displayURL,
largeMedia: largeMediaDisplayURLs[id],
},
};
});
},
);
}
@ -270,13 +273,13 @@ export default class GitGateway {
['backend', 'use_large_media_transforms_in_media_library'],
true,
)
? { nf_resize: 'fit', w: 280, h: 160 }
? { nf_resize: 'fit', w: 560, h: 320 }
: false,
});
},
);
}
getLargeMedia(mediaFiles) {
getLargeMediaDisplayURLs(mediaFiles) {
return this.getLargeMediaClient().then(client => {
const largeMediaItems = mediaFiles
.filter(({ path }) => client.matchPath(path))
@ -296,16 +299,35 @@ export default class GitGateway {
}),
)
.then(unzip)
.then(async ([idMaps, files]) => [
idMaps,
await client.getResourceDownloadURLThunks(files).then(fromPairs),
])
.then(([idMaps, files]) =>
Promise.all([idMaps, client.getResourceDownloadURLArgs(files).then(fromPairs)]),
)
.then(([idMaps, resourceMap]) =>
idMaps.map(({ pointerId, resourceId }) => [pointerId, resourceMap[resourceId]]),
)
.then(fromPairs);
});
}
getMediaDisplayURL(displayURL) {
const { path, original, largeMedia: largeMediaDisplayURL } = displayURL;
return this.getLargeMediaClient().then(client => {
if (client.enabled && client.matchPath(path)) {
return client.getDownloadURL(largeMediaDisplayURL);
}
if (this.backend.getMediaDisplayURL) {
return this.backend.getMediaDisplayURL(original);
}
const err = new Error(
`getMediaDisplayURL is not implemented by the ${
this.backendType
} backend, but the backend returned a displayURL which was not a string!`,
);
err.displayURL = displayURL;
return Promise.reject(err);
});
}
persistEntry(entry, mediaFiles, options) {
return this.backend.persistEntry(entry, mediaFiles, options);
}
@ -332,11 +354,7 @@ export default class GitGateway {
raw: pointerFileString,
value,
};
const persistedMediaFile = await this.backend.persistMedia(persistMediaArgument, options);
return {
...persistedMediaFile,
urlIsPublicPath: false,
};
return this.backend.persistMedia(persistMediaArgument, options);
});
});
}

View File

@ -99,10 +99,7 @@ const resourceExists = async ({ rootURL, makeAuthorizedRequest }, { sha, size })
// to fit
};
const getDownloadURLThunkFromSha = (
{ rootURL, makeAuthorizedRequest, transformImages: t },
sha,
) => () =>
const getDownloadURL = ({ rootURL, transformImages: t, makeAuthorizedRequest }, { sha }) =>
makeAuthorizedRequest(
`${rootURL}/origin/${sha}${
t && Object.keys(t).length > 0 ? `?nf_resize=${t.nf_resize}&w=${t.w}&h=${t.h}` : ''
@ -113,17 +110,13 @@ const getDownloadURLThunkFromSha = (
.then(blob => URL.createObjectURL(blob))
.catch(err => console.error(err) || Promise.resolve(''));
// We allow users to get thunks which load the blobs instead of fully
// resolved blob URLs so that media clients can download the blobs
// lazily. This behaves more similarly to the behavior of string
// URLs, which only trigger an image download when the DOM element for
// that image is created.
const getResourceDownloadURLThunks = (clientConfig, objects) =>
Promise.resolve(objects.map(({ sha }) => [sha, getDownloadURLThunkFromSha(clientConfig, sha)]));
const getResourceDownloadURLArgs = (clientConfig, objects) => {
return Promise.resolve(objects.map(({ sha }) => [sha, { sha }]));
};
const getResourceDownloadURLs = (clientConfig, objects) =>
getResourceDownloadURLThunks(clientConfig, objects)
.then(map(([sha, thunk]) => Promise.all([sha, thunk()])))
getResourceDownloadURLArgs(clientConfig, objects)
.then(map(downloadURLArg => getDownloadURL(downloadURLArg)))
.then(Promise.all.bind(Promise));
const uploadOperation = objects => ({
@ -171,7 +164,8 @@ const clientFns = {
resourceExists,
getResourceUploadURLs,
getResourceDownloadURLs,
getResourceDownloadURLThunks,
getResourceDownloadURLArgs,
getDownloadURL,
uploadResource,
matchPath,
};

View File

@ -164,7 +164,7 @@ export default class GitHub {
if (url.pathname.match(/.svg$/)) {
url.search += (url.search.slice(1) === '' ? '?' : '&') + 'sanitize=true';
}
return { id: sha, name, size, url: url.href, urlIsPublicPath: true, path };
return { id: sha, name, size, displayURL: url.href, path };
}),
);
}
@ -178,8 +178,14 @@ export default class GitHub {
await this.api.persistFiles(null, [mediaFile], options);
const { sha, value, path, fileObj } = mediaFile;
const url = URL.createObjectURL(fileObj);
return { id: sha, name: value, size: fileObj.size, url, path: trimStart(path, '/') };
const displayURL = URL.createObjectURL(fileObj);
return {
id: sha,
name: value,
size: fileObj.size,
displayURL,
path: trimStart(path, '/'),
};
} catch (error) {
console.error(error);
throw error;

View File

@ -143,33 +143,34 @@ export default class GitLab {
}
getMedia() {
const sem = semaphore(MAX_CONCURRENT_DOWNLOADS);
return this.api.listAllFiles(this.config.get('media_folder')).then(files =>
files.map(({ id, name, path }) => {
const getDisplayURL = () =>
new Promise((resolve, reject) =>
sem.take(() =>
this.api
.readFile(path, id, { parseText: false })
.then(blob => {
// svgs are returned with mimetype "text/plain" by gitlab
if (blob.type === 'text/plain' && name.match(/\.svg$/i)) {
return new window.Blob([blob], { type: 'image/svg+xml' });
}
return blob;
})
.then(blob => URL.createObjectURL(blob))
.then(resolve, reject)
.finally(() => sem.leave()),
),
);
return { id, name, getDisplayURL, path };
return { id, name, path, displayURL: { id, name, path } };
}),
);
}
getMediaDisplayURL(displayURL) {
this._mediaDisplayURLSem = this._mediaDisplayURLSem || semaphore(MAX_CONCURRENT_DOWNLOADS);
const { id, name, path } = displayURL;
return new Promise((resolve, reject) =>
this._mediaDisplayURLSem.take(() =>
this.api
.readFile(path, id, { parseText: false })
.then(blob => {
// svgs are returned with mimetype "text/plain" by gitlab
if (blob.type === 'text/plain' && name.match(/\.svg$/i)) {
return new window.Blob([blob], { type: 'image/svg+xml' });
}
return blob;
})
.then(blob => URL.createObjectURL(blob))
.then(resolve, reject)
.finally(() => this._mediaDisplayURLSem.leave()),
),
);
}
async persistEntry(entry, mediaFiles, options = {}) {
return this.api.persistFiles([entry], options);
}

View File

@ -159,16 +159,14 @@ export function persistMedia(file, opts = {}) {
try {
const id = await getBlobSHA(file);
const getDisplayURL = () => URL.createObjectURL(file);
const displayURL = URL.createObjectURL(file);
const assetProxy = await createAssetProxy(fileName, file, false, privateUpload);
dispatch(addAsset(assetProxy));
if (!integration) {
const asset = await backend.persistMedia(state.config, assetProxy);
return dispatch(mediaPersisted({ id, getDisplayURL, ...asset }));
return dispatch(mediaPersisted({ id, displayURL, ...asset }));
}
return dispatch(
mediaPersisted({ id, getDisplayURL, ...assetProxy.asset }, { privateUpload }),
);
return dispatch(mediaPersisted({ id, displayURL, ...assetProxy.asset }, { privateUpload }));
} catch (error) {
console.error(error);
dispatch(
@ -231,28 +229,41 @@ export function deleteMedia(file, opts = {}) {
export function loadMediaDisplayURL(file) {
return async (dispatch, getState) => {
const { getDisplayURL, id, url, urlIsPublicPath } = file;
const { mediaLibrary: mediaLibraryState } = getState();
const displayURLPath = ['displayURLs', id];
const shouldLoadDisplayURL =
id &&
((url && urlIsPublicPath) ||
(getDisplayURL &&
!mediaLibraryState.getIn([...displayURLPath, 'url']) &&
!mediaLibraryState.getIn([...displayURLPath, 'isFetching']) &&
!mediaLibraryState.getIn([...displayURLPath, 'err'])));
if (shouldLoadDisplayURL) {
try {
dispatch(mediaDisplayURLRequest(id));
const newURL = (urlIsPublicPath && url) || (await getDisplayURL());
if (newURL) {
dispatch(mediaDisplayURLSuccess(id, newURL));
return newURL;
}
const { displayURL, id, url } = file;
const state = getState();
const displayURLState = state.mediaLibrary.getIn(['displayURLs', id], Map());
if (
!id ||
// displayURL is used by most backends; url (like urlIsPublicPath) is used exclusively by the
// assetStore integration. Only the assetStore uses URLs which can actually be inserted into
// an entry - other backends create a domain-relative URL using the public_folder from the
// config and the file's name.
(!displayURL && !url) ||
displayURLState.get('url') ||
displayURLState.get('isFetching') ||
displayURLState.get('err')
) {
return Promise.resolve();
}
if (typeof url === 'string') {
dispatch(mediaDisplayURLRequest(id));
return dispatch(mediaDisplayURLSuccess(id, displayURL));
}
if (typeof displayURL === 'string') {
dispatch(mediaDisplayURLRequest(id));
return dispatch(mediaDisplayURLSuccess(id, displayURL));
}
try {
const backend = currentBackend(state.config);
dispatch(mediaDisplayURLRequest(id));
const newURL = await backend.getMediaDisplayURL(displayURL);
if (newURL) {
return dispatch(mediaDisplayURLSuccess(id, newURL));
} else {
throw new Error('No display URL was returned!');
} catch (err) {
dispatch(mediaDisplayURLFailure(id, err));
}
} catch (err) {
return dispatch(mediaDisplayURLFailure(id, err));
}
};
}
@ -322,6 +333,7 @@ export function mediaDisplayURLSuccess(key, url) {
}
export function mediaDisplayURLFailure(key, err) {
console.error(err);
return {
type: MEDIA_DISPLAY_URL_FAILURE,
payload: { key, err },

View File

@ -486,6 +486,17 @@ class Backend {
return this.implementation.getMedia();
}
getMediaDisplayURL(displayURL) {
if (this.implementation.getMediaDisplayURL) {
return this.implementation.getMediaDisplayURL(displayURL);
}
const err = new Error(
'getMediaDisplayURL is not implemented by the current backend, but the backend returned a displayURL which was not a string!',
);
err.displayURL = displayURL;
return Promise.reject(err);
}
entryWithFormat(collectionOrEntity) {
return entry => {
const format = resolveFormat(collectionOrEntity, entry);

View File

@ -24,14 +24,14 @@ const IMAGE_EXTENSIONS_VIEWABLE = ['jpg', 'jpeg', 'webp', 'gif', 'png', 'bmp', '
const IMAGE_EXTENSIONS = [...IMAGE_EXTENSIONS_VIEWABLE];
const fileShape = {
key: PropTypes.string.isRequired,
displayURL: PropTypes.oneOfType([PropTypes.string, PropTypes.object]).isRequired,
id: PropTypes.string.isRequired,
key: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
size: PropTypes.number,
queryOrder: PropTypes.number,
size: PropTypes.number,
url: PropTypes.string,
urlIsPublicPath: PropTypes.bool,
getDisplayURL: PropTypes.func,
};
class MediaLibrary extends React.Component {
@ -119,7 +119,7 @@ class MediaLibrary extends React.Component {
toTableData = files => {
const tableData =
files &&
files.map(({ key, name, id, size, queryOrder, url, urlIsPublicPath, getDisplayURL }) => {
files.map(({ key, name, id, size, queryOrder, url, urlIsPublicPath, displayURL }) => {
const ext = fileExtension(name).toLowerCase();
return {
key,
@ -130,7 +130,7 @@ class MediaLibrary extends React.Component {
queryOrder,
url,
urlIsPublicPath,
getDisplayURL,
displayURL,
isImage: IMAGE_EXTENSIONS.includes(ext),
isViewableImage: IMAGE_EXTENSIONS_VIEWABLE.includes(ext),
};

View File

@ -62,9 +62,9 @@ class MediaLibraryCard extends React.Component {
</Card>
);
}
UNSAFE_componentWillMount() {
componentDidMount() {
const { displayURL, loadDisplayURL } = this.props;
if (!displayURL || (!displayURL.url && !displayURL.isFetching && !displayURL.err)) {
if (!displayURL.get('url')) {
loadDisplayURL();
}
}

View File

@ -48,7 +48,7 @@ const MediaLibraryCardGrid = ({
width={cardWidth}
margin={cardMargin}
isPrivate={isPrivate}
displayURL={displayURLs.get(file.id, Map())}
displayURL={displayURLs.get(file.id, file.url ? Map({ url: file.url }) : Map())}
loadDisplayURL={() => loadDisplayURL(file)}
type={file.type}
/>
@ -65,10 +65,13 @@ MediaLibraryCardGrid.propTypes = {
setScrollContainerRef: PropTypes.func.isRequired,
mediaItems: PropTypes.arrayOf(
PropTypes.shape({
displayURL: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
id: PropTypes.string.isRequired,
key: PropTypes.string.isRequired,
isViewableImage: PropTypes.bool,
name: PropTypes.string.isRequired,
type: PropTypes.string.isRequired,
url: PropTypes.string,
name: PropTypes.string,
urlIsPublicPath: PropTypes.bool,
}),
).isRequired,
isSelectedFile: PropTypes.func.isRequired,

View File

@ -180,14 +180,14 @@ const MediaLibraryModal = ({
};
const fileShape = {
key: PropTypes.string.isRequired,
displayURL: PropTypes.oneOfType([PropTypes.string, PropTypes.object]).isRequired,
id: PropTypes.string.isRequired,
key: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
size: PropTypes.number,
queryOrder: PropTypes.number,
size: PropTypes.number,
url: PropTypes.string,
urlIsPublicPath: PropTypes.bool,
getDisplayURL: PropTypes.func.isRequired,
};
MediaLibraryModal.propTypes = {