Updated uniqueness validation for the Recommendations URL (#18253)
closes https://github.com/TryGhost/Product/issues/3818 - instead of fetching all recommendations and matching URLs on the frontend, we now query the database directly to find an existing Recommendation by URL. When comparing URLs, we don't take into account the protocol, www, query parameters nor hash fragments
This commit is contained in:
Sag
GitHub
parent
715d658e11
commit
8515bdf587
@ -1,4 +1,4 @@
|
||||
import {Meta, createMutation, createPaginatedQuery} from '../utils/apiRequests';
|
||||
import {Meta, apiUrl, createMutation, createPaginatedQuery, useFetchApi} from '../utils/apiRequests';
|
||||
|
||||
export type Recommendation = {
|
||||
id: string
|
||||
@ -69,3 +69,26 @@ export const useAddRecommendation = createMutation<RecommendationResponseType, P
|
||||
dataType
|
||||
}
|
||||
});
|
||||
|
||||
export const useGetRecommendationByUrl = () => {
|
||||
const fetchApi = useFetchApi();
|
||||
const path = '/recommendations/';
|
||||
|
||||
return {
|
||||
async query(url: URL): Promise<RecommendationResponseType | null> {
|
||||
const urlFilter = `url:~'${url.host.replace('www.', '')}${url.pathname.replace(/\/$/, '')}'`;
|
||||
const endpoint = apiUrl(path, {filter: urlFilter, limit: '1'});
|
||||
try {
|
||||
const result = await fetchApi(endpoint, {
|
||||
method: 'GET',
|
||||
timeout: 5000
|
||||
});
|
||||
return result as RecommendationResponseType;
|
||||
} catch (e) {
|
||||
// eslint-disable-next-line no-console
|
||||
console.error(e);
|
||||
return null;
|
||||
}
|
||||
}
|
||||
};
|
||||
};
|
||||
|
||||
@ -6,11 +6,12 @@ import React from 'react';
|
||||
import URLTextField from '../../../../admin-x-ds/global/form/URLTextField';
|
||||
import useForm from '../../../../hooks/useForm';
|
||||
import useRouting from '../../../../hooks/useRouting';
|
||||
import {EditOrAddRecommendation, useBrowseRecommendations} from '../../../../api/recommendations';
|
||||
import {AlreadyExistsError} from '../../../../utils/errors';
|
||||
import {EditOrAddRecommendation, RecommendationResponseType, useGetRecommendationByUrl} from '../../../../api/recommendations';
|
||||
import {RoutingModalProps} from '../../../providers/RoutingProvider';
|
||||
import {arePathsEqual, trimSearchAndHash} from '../../../../utils/url';
|
||||
import {showToast} from '../../../../admin-x-ds/global/Toast';
|
||||
import {toast} from 'react-hot-toast';
|
||||
import {trimSearchAndHash} from '../../../../utils/url';
|
||||
import {useExternalGhostSite} from '../../../../api/external-ghost-site';
|
||||
import {useGetOembed} from '../../../../api/oembed';
|
||||
|
||||
@ -24,7 +25,7 @@ const AddRecommendationModal: React.FC<RoutingModalProps & AddRecommendationModa
|
||||
const {updateRoute} = useRouting();
|
||||
const {query: queryOembed} = useGetOembed();
|
||||
const {query: queryExternalGhostSite} = useExternalGhostSite();
|
||||
const {data: {recommendations} = {}} = useBrowseRecommendations();
|
||||
const {query: getRecommendationByUrl} = useGetRecommendationByUrl();
|
||||
|
||||
const {formState, updateForm, handleSave, errors, validate, saveState, clearError} = useForm({
|
||||
initialState: recommendation ?? {
|
||||
@ -41,7 +42,13 @@ const AddRecommendationModal: React.FC<RoutingModalProps & AddRecommendationModa
|
||||
validatedUrl = new URL(formState.url);
|
||||
validatedUrl = trimSearchAndHash(validatedUrl);
|
||||
|
||||
// First check if it s a Ghost site or not
|
||||
// Check if the recommendation already exists
|
||||
const {recommendations = []} = await getRecommendationByUrl(validatedUrl) as RecommendationResponseType;
|
||||
if (recommendations && recommendations.length > 0) {
|
||||
throw new AlreadyExistsError('A recommendation with this URL already exists.');
|
||||
}
|
||||
|
||||
// Check if it s a Ghost site or not
|
||||
let externalGhostSite = validatedUrl.protocol === 'https:' ? (await queryExternalGhostSite('https://' + validatedUrl.host)) : null;
|
||||
|
||||
// Use the hostname as fallback title
|
||||
@ -90,11 +97,6 @@ const AddRecommendationModal: React.FC<RoutingModalProps & AddRecommendationModa
|
||||
if (!u.hostname.includes('.')) {
|
||||
newErrors.url = 'Please enter a valid URL.';
|
||||
}
|
||||
|
||||
// Check that it doesn't exist already
|
||||
if (recommendations?.find(r => arePathsEqual(r.url, u.toString()))) {
|
||||
newErrors.url = 'A recommendation with this URL already exists.';
|
||||
}
|
||||
} catch (e) {
|
||||
newErrors.url = 'Please enter a valid URL.';
|
||||
}
|
||||
@ -133,9 +135,10 @@ const AddRecommendationModal: React.FC<RoutingModalProps & AddRecommendationModa
|
||||
try {
|
||||
await handleSave({force: true});
|
||||
} catch (e) {
|
||||
const message = e instanceof AlreadyExistsError ? e.message : 'Something went wrong while checking this URL, please try again.';
|
||||
showToast({
|
||||
type: 'pageError',
|
||||
message: 'Something went wrong while checking this URL, please try again.'
|
||||
message
|
||||
});
|
||||
}
|
||||
}}
|
||||
|
||||
@ -37,3 +37,9 @@ export class ValidationError extends JSONError {
|
||||
super(response, data, data.errors[0].message);
|
||||
}
|
||||
}
|
||||
|
||||
export class AlreadyExistsError extends Error {
|
||||
constructor(message?: string) {
|
||||
super(message);
|
||||
}
|
||||
}
|
||||
|
||||
@ -13,26 +13,3 @@ export function trimSearchAndHash(url: URL) {
|
||||
url.hash = '';
|
||||
return url;
|
||||
}
|
||||
|
||||
/* Compare two URLs based on their hostname and pathname.
|
||||
* Query params, hash fragements, protocol and www are ignored.
|
||||
*
|
||||
* Example:
|
||||
* - https://a.com, http://a.com, https://www.a.com, https://a.com?param1=value, https://a.com/#segment-1 are all considered equal
|
||||
* - but, https://a.com/path-1 and https://a.com/path-2 are not
|
||||
*/
|
||||
export function arePathsEqual(urlStr1: string, urlStr2: string) {
|
||||
let url1, url2;
|
||||
|
||||
try {
|
||||
url1 = new URL(urlStr1);
|
||||
url2 = new URL(urlStr2);
|
||||
} catch (e) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return (
|
||||
url1.hostname.replace('www.', '') === url2.hostname.replace('www.', '') &&
|
||||
url1.pathname === url2.pathname
|
||||
);
|
||||
}
|
||||
|
||||
@ -1,5 +1,5 @@
|
||||
import * as assert from 'assert/strict';
|
||||
import {arePathsEqual, trimHash, trimSearch, trimSearchAndHash} from '../../../src/utils/url';
|
||||
import {trimHash, trimSearch, trimSearchAndHash} from '../../../src/utils/url';
|
||||
|
||||
describe('trimSearch', function () {
|
||||
it('removes the query parameters from a URL', function () {
|
||||
@ -27,68 +27,3 @@ describe('trimSearchAndHash', function () {
|
||||
assert.equal(trimSearchAndHash(parsedUrl).toString(), 'https://example.com/path');
|
||||
});
|
||||
});
|
||||
|
||||
describe('arePathsEqual', function () {
|
||||
it('returns false if one of the param is not a URL', function () {
|
||||
const url1 = 'foo';
|
||||
const url2 = 'https://example.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), false);
|
||||
});
|
||||
|
||||
it('returns false if hostnames are different', function () {
|
||||
const url1 = 'https://a.com';
|
||||
const url2 = 'https://b.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), false);
|
||||
});
|
||||
|
||||
it('returns false if top level domains are different', function () {
|
||||
const url1 = 'https://a.io';
|
||||
const url2 = 'https://a.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), false);
|
||||
});
|
||||
|
||||
it('returns false if sub domains are different', function () {
|
||||
const url1 = 'https://sub.a.com';
|
||||
const url2 = 'https://subdiff.a.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), false);
|
||||
});
|
||||
|
||||
it('returns false if paths are different', function () {
|
||||
const url1 = 'https://a.com/path-1';
|
||||
const url2 = 'https://a.com/path-2';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), false);
|
||||
});
|
||||
|
||||
it('returns true even if protocols are different', function () {
|
||||
const url1 = 'http://a.com';
|
||||
const url2 = 'https://a.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), true);
|
||||
});
|
||||
|
||||
it('returns true even if www is used in one of the urls', function () {
|
||||
const url1 = 'https://www.a.com';
|
||||
const url2 = 'https://a.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), true);
|
||||
});
|
||||
|
||||
it('returns true even if query parameters are different', function () {
|
||||
const url1 = 'http://a.com?foo=bar';
|
||||
const url2 = 'http://a.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), true);
|
||||
});
|
||||
|
||||
it('returns true even if hash segments are different', function () {
|
||||
const url1 = 'http://a.com#segment-1';
|
||||
const url2 = 'http://a.com';
|
||||
|
||||
assert.equal(arePathsEqual(url1, url2), true);
|
||||
});
|
||||
});
|
||||
|
||||
@ -1213,6 +1213,76 @@ Object {
|
||||
}
|
||||
`;
|
||||
|
||||
exports[`Recommendations Admin API add Can add a recommendation with the same hostname but different paths 1: [body] 1`] = `
|
||||
Object {
|
||||
"recommendations": Array [
|
||||
Object {
|
||||
"created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/,
|
||||
"excerpt": null,
|
||||
"favicon": null,
|
||||
"featured_image": null,
|
||||
"id": StringMatching /\\[a-f0-9\\]\\{24\\}/,
|
||||
"one_click_subscribe": false,
|
||||
"reason": null,
|
||||
"title": "Recommendation 3 with a different path",
|
||||
"updated_at": null,
|
||||
"url": "https://recommendation3.com/path-1",
|
||||
},
|
||||
],
|
||||
}
|
||||
`;
|
||||
|
||||
exports[`Recommendations Admin API add Can add a recommendation with the same hostname but different paths 2: [headers] 1`] = `
|
||||
Object {
|
||||
"access-control-allow-origin": "http://127.0.0.1:2369",
|
||||
"cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
|
||||
"content-length": "299",
|
||||
"content-type": "application/json; charset=utf-8",
|
||||
"content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/,
|
||||
"etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
|
||||
"location": StringMatching /https\\?:\\\\/\\\\/\\.\\*\\?\\\\/recommendations\\\\/\\[a-f0-9\\]\\{24\\}\\\\//,
|
||||
"vary": "Accept-Version, Origin, Accept-Encoding",
|
||||
"x-cache-invalidate": "/*",
|
||||
"x-powered-by": "Express",
|
||||
}
|
||||
`;
|
||||
|
||||
exports[`Recommendations Admin API add Cannot add the same recommendation URL twice (exact URL match) 1: [body] 1`] = `
|
||||
Object {
|
||||
"errors": Array [
|
||||
Object {
|
||||
"code": null,
|
||||
"context": "A recommendation with this URL already exists.",
|
||||
"details": null,
|
||||
"ghostErrorCode": null,
|
||||
"help": null,
|
||||
"id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
|
||||
"message": "Validation error, cannot save recommendation.",
|
||||
"property": null,
|
||||
"type": "ValidationError",
|
||||
},
|
||||
],
|
||||
}
|
||||
`;
|
||||
|
||||
exports[`Recommendations Admin API add Cannot add the same recommendation twice (partial URL match) 1: [body] 1`] = `
|
||||
Object {
|
||||
"errors": Array [
|
||||
Object {
|
||||
"code": null,
|
||||
"context": "A recommendation with this URL already exists.",
|
||||
"details": null,
|
||||
"ghostErrorCode": null,
|
||||
"help": null,
|
||||
"id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/,
|
||||
"message": "Validation error, cannot save recommendation.",
|
||||
"property": null,
|
||||
"type": "ValidationError",
|
||||
},
|
||||
],
|
||||
}
|
||||
`;
|
||||
|
||||
exports[`Recommendations Admin API add Cannot add the same recommendation twice 1: [body] 1`] = `
|
||||
Object {
|
||||
"recommendations": Array [
|
||||
|
||||
@ -544,14 +544,23 @@ describe('Recommendations Admin API', function () {
|
||||
assert.equal(body.recommendations[0].one_click_subscribe, true);
|
||||
});
|
||||
|
||||
it('Cannot add the same recommendation twice', async function () {
|
||||
it('Can add a recommendation with the same hostname but different paths', async function () {
|
||||
// Add a recommendation with URL https://recommendation3.com
|
||||
await addDummyRecommendation(3);
|
||||
|
||||
await agent.post('recommendations/')
|
||||
.body({
|
||||
recommendations: [{
|
||||
title: 'Dog Pictures',
|
||||
url: 'https://dogpictures.com'
|
||||
title: 'Recommendation 3 with a different path',
|
||||
url: 'https://recommendation3.com/path-1'
|
||||
}]
|
||||
})
|
||||
.expectStatus(201)
|
||||
.matchHeaderSnapshot({
|
||||
'content-version': anyContentVersion,
|
||||
etag: anyEtag,
|
||||
location: anyLocationFor('recommendations')
|
||||
})
|
||||
.matchBodySnapshot({
|
||||
recommendations: [
|
||||
{
|
||||
@ -560,12 +569,38 @@ describe('Recommendations Admin API', function () {
|
||||
}
|
||||
]
|
||||
});
|
||||
});
|
||||
|
||||
it('Cannot add the same recommendation URL twice (exact URL match)', async function () {
|
||||
// Add a recommendation with URL https://recommendation3.com
|
||||
await addDummyRecommendation(3);
|
||||
|
||||
await agent.post('recommendations/')
|
||||
.body({
|
||||
recommendations: [{
|
||||
title: 'Dog Pictures 2',
|
||||
url: 'https://dogpictures.com'
|
||||
title: 'Recommendation 3 with the exact same URL',
|
||||
url: 'https://recommendation3.com'
|
||||
}]
|
||||
})
|
||||
.expectStatus(422)
|
||||
.matchBodySnapshot({
|
||||
errors: [
|
||||
{
|
||||
id: anyErrorId
|
||||
}
|
||||
]
|
||||
});
|
||||
});
|
||||
|
||||
it('Cannot add the same recommendation twice (partial URL match)', async function () {
|
||||
// Add a recommendation with URL https://recommendation3.com
|
||||
await addDummyRecommendation(3);
|
||||
|
||||
await agent.post('recommendations/')
|
||||
.body({
|
||||
recommendations: [{
|
||||
title: 'Recommendation 3 with the same hostname and pathname, but with different protocol, www, query params and hash fragement',
|
||||
url: 'http://www.recommendation3.com/?query=1#hash'
|
||||
}]
|
||||
})
|
||||
.expectStatus(422)
|
||||
|
||||
@ -75,8 +75,8 @@ export class BookshelfRecommendationRepository extends BookshelfRepository<strin
|
||||
} as Record<keyof Recommendation, string>;
|
||||
}
|
||||
|
||||
async getByUrl(url: URL): Promise<Recommendation | null> {
|
||||
const model = await (this.Model as RecommendationModelClass<string>).findOne({url: url.toString()}, {require: false});
|
||||
return model ? this.modelToEntity(model) : null;
|
||||
async getByUrl(url: URL): Promise<Recommendation[]> {
|
||||
const urlFilter = `url:~'${url.host.replace('www.', '')}${url.pathname.replace(/\/$/, '')}'`;
|
||||
return this.getPage({filter: urlFilter, page: 1, limit: 1});
|
||||
}
|
||||
}
|
||||
|
||||
@ -7,9 +7,8 @@ export class InMemoryRecommendationRepository extends InMemoryRepository<string,
|
||||
return entity;
|
||||
}
|
||||
|
||||
getByUrl(url: URL): Promise<Recommendation | null> {
|
||||
return this.getAll().then((recommendations) => {
|
||||
return recommendations.find(recommendation => recommendation.url.toString() === url.toString()) || null;
|
||||
});
|
||||
getByUrl(url: URL): Promise<Recommendation[]> {
|
||||
const urlFilter = `url:~'${url.host.replace('www.', '')}${url.pathname.replace(/\/$/, '')}'`;
|
||||
return this.getPage({filter: urlFilter, page: 1, limit: 1});
|
||||
}
|
||||
}
|
||||
|
||||
@ -4,7 +4,7 @@ import {Recommendation} from './Recommendation';
|
||||
export interface RecommendationRepository {
|
||||
save(entity: Recommendation): Promise<void>;
|
||||
getById(id: string): Promise<Recommendation | null>;
|
||||
getByUrl(url: URL): Promise<Recommendation | null>;
|
||||
getByUrl(url: URL): Promise<Recommendation[]>;
|
||||
getAll({filter, order}?: {filter?: string, order?: OrderOption<Recommendation>}): Promise<Recommendation[]>;
|
||||
getPage({filter, order, page, limit}: {
|
||||
filter?: string;
|
||||
|
||||
@ -135,7 +135,7 @@ export class RecommendationService {
|
||||
|
||||
// If a recommendation with this URL already exists, throw an error
|
||||
const existing = await this.repository.getByUrl(recommendation.url);
|
||||
if (existing) {
|
||||
if (existing && existing.length > 0) {
|
||||
throw new errors.ValidationError({
|
||||
message: 'A recommendation with this URL already exists.'
|
||||
});
|
||||
|
||||
Loading…
Reference in New Issue
Block a user