[next] Story Improvements (#2054)

* feat: improved logs

* feat: improved scraper

- added scraper debug graph call

* feat: improved story closing

* fix: fixed tests
This commit is contained in:
Wyatt Johnson
2018-11-01 21:34:48 +01:00
committed by Kiwi
parent 6597658734
commit b3cfe32631
19 changed files with 377 additions and 92 deletions
@@ -15,7 +15,9 @@ it("renders correctly", () => {
createdAt: "2018-07-06T18:24:00.000Z",
replyCount: 4,
story: {
title: "Story Title",
metadata: {
title: "Story Title",
},
},
conversationURL: "http://localhost/conversation",
onGotoConversation: noop,
@@ -18,7 +18,9 @@ export interface HistoryCommentProps {
createdAt: string;
replyCount: number | null;
story: {
title: string | null;
metadata: {
title: string | null;
} | null;
};
conversationURL: string;
onGotoConversation: (e: React.MouseEvent) => void;
@@ -27,7 +29,10 @@ export interface HistoryCommentProps {
const HistoryComment: StatelessComponent<HistoryCommentProps> = props => {
return (
<HorizontalGutter>
<Localized id="profile-historyComment-story" $title={props.story.title}>
<Localized
id="profile-historyComment-story"
$title={props.story.metadata ? props.story.metadata.title : "N/A"} // FIXME: (wyattjoh) When a title for a Story isn't available, we need a fallback.
>
<Typography variant="heading4">{"Story: {$title}"}</Typography>
</Localized>
<Timestamp>{props.createdAt}</Timestamp>
@@ -55,8 +55,10 @@ const enhanced = withSetCommentIDMutation(
replyCount
story {
id
title
url
metadata {
title
}
}
}
`,
+3
View File
@@ -215,6 +215,9 @@ export const commentWithDeepestReplies = denormalizeComment({
});
export const baseStory = {
metadata: {
title: "title",
},
isClosed: false,
comments: {
edges: [],
+14 -3
View File
@@ -1,12 +1,13 @@
import builder from "content-security-policy-builder";
import {
doesRequireSchemePrefixing,
extractParentsOrigin,
extractParentsURL,
getOrigin,
isURLSecure,
prefixSchemeIfRequired,
} from "talk-server/app/url";
import { Tenant } from "talk-server/models/tenant";
import { isURLPermitted } from "talk-server/services/stories";
import { Request, RequestHandler } from "talk-server/types/express";
/**
@@ -65,8 +66,13 @@ export function generateFrameOptions(
return `allow-from ${getOrigin(tenant.domains[0])}`;
}
// Grab the parent's hostname.
const parentsOrigin = extractParentsOrigin(req);
const parentsURL = extractParentsURL(req);
if (!parentsURL) {
return "deny";
}
// Grab the parent's origin.
const parentsOrigin = getOrigin(parentsURL);
if (!parentsOrigin) {
return "deny";
}
@@ -88,6 +94,11 @@ export function generateFrameOptions(
)}`;
}
// Determine if this origin is allowed.
if (!isURLPermitted(tenant, parentsURL)) {
return "deny";
}
// As we can only return a single domain in the `allow-from` directive as per
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options
// We need to find the domain that is asking so we can respond with the right
+18 -9
View File
@@ -70,12 +70,7 @@ export function prefixSchemeIfRequired(secure: boolean, url: string) {
return url;
}
/**
* extractParentsOrigin will pull the parent's origin out.
*
* @param req the request where we want to extract the parent's hostname from.
*/
export function extractParentsOrigin(req: Request) {
export function extractParentsURL(req: Request) {
// The only two places this could be is in the referer header or the parentUrl
// query parameter (injected by pym.js). If both of these are empty, then we
// can't find anything.
@@ -87,16 +82,30 @@ export function extractParentsOrigin(req: Request) {
if (req.headers.referer && req.headers.referer.length > 0) {
// If the header contains multiple values, return the first one.
if (Array.isArray(req.headers.referer)) {
return getOrigin(req.headers.referer[0]);
return req.headers.referer[0];
}
return getOrigin(req.headers.referer);
return req.headers.referer;
}
// If the parentUrl query parameter is provided, then try to parse it.
if (req.query.parentUrl) {
return getOrigin(req.query.parentUrl);
return req.query.parentUrl;
}
return null;
}
/**
* extractParentsOrigin will pull the parent's origin out.
*
* @param req the request where we want to extract the parent's hostname from.
*/
export function extractParentsOrigin(req: Request) {
const url = extractParentsURL(req);
if (!url) {
return null;
}
return getOrigin(url);
}
@@ -1,11 +1,13 @@
import DataLoader from "dataloader";
import TenantContext from "talk-server/graph/tenant/context";
import { GQLStoryMetadata } from "talk-server/graph/tenant/schema/__generated__/types";
import {
FindOrCreateStoryInput,
retrieveManyStories,
Story,
} from "talk-server/models/story";
import { scraper } from "talk-server/services/queue/tasks/scraper";
import { findOrCreate } from "talk-server/services/stories";
export default (ctx: TenantContext) => ({
@@ -14,4 +16,7 @@ export default (ctx: TenantContext) => ({
story: new DataLoader<string, Story | null>(ids =>
retrieveManyStories(ctx.mongo, ctx.tenant.id, ids)
),
debugScrapeMetadata: new DataLoader<string, GQLStoryMetadata | null>(urls =>
Promise.all(urls.map(url => scraper.scrape(url)))
),
});
@@ -8,6 +8,8 @@ const Query: GQLQueryTypeResolver<void> = {
me: (source, args, ctx) => ctx.user,
discoverOIDCConfiguration: (source, { issuer }, ctx) =>
ctx.loaders.Auth.discoverOIDCConfiguration.load(issuer),
debugScrapeStoryMetadata: (source, { url }, ctx) =>
ctx.loaders.Stories.debugScrapeMetadata.load(url),
};
export default Query;
@@ -1,3 +1,5 @@
import { DateTime } from "luxon";
import { GQLStoryTypeResolver } from "talk-server/graph/tenant/schema/__generated__/types";
import { decodeActionCounts } from "talk-server/models/action";
import { Story } from "talk-server/models/story";
@@ -5,8 +7,20 @@ import { Story } from "talk-server/models/story";
const Story: GQLStoryTypeResolver<Story> = {
comments: (story, input, ctx) =>
ctx.loaders.Comments.forStory(story.id, input),
// TODO: implement this.
isClosed: () => false,
closedAt: (story, input, ctx) => {
if (story.closedAt) {
return story.closedAt;
}
if (ctx.tenant.autoCloseStream && ctx.tenant.closedTimeout) {
return DateTime.fromJSDate(story.created_at)
.plus(ctx.tenant.closedTimeout)
.toJSDate();
}
return null;
},
actionCounts: story => decodeActionCounts(story.action_counts),
commentCounts: story => story.comment_counts,
};
@@ -1211,6 +1211,49 @@ enum COMMENT_SORT {
RESPECT_DESC
}
"""
StoryMetadata stores all the metadata that is scraped using the scraping tools
inside Talk. Talk utilizes [metascraper](https://metascraper.js.org/) which uses
a variety of filters derived from [The Open Graph Protocol](https://ogp.me/) to
scan for metadata on the page.
"""
type StoryMetadata {
"""
title stores the scraped title from the Story page.
"""
title: String
"""
author stores the scraped author from the Story page.
"""
author: String
"""
description stores the scraped description from the Story page.
"""
description: String
"""
image stores the scraped image from the Story page.
"""
image: String
"""
publishedAt stores the scraped publication date from the Story page.
"""
publishedAt: Time
"""
modifiedAt stores the scraped modified date from the Story page.
"""
modifiedAt: Time
"""
section stores the scraped section from the Story page.
"""
section: String
}
"""
Story is an Article or Page where Comments are written on by Users.
"""
@@ -1226,9 +1269,14 @@ type Story {
url: String!
"""
title is the title of the scraped Story.
metadata stores the scraped metadata from the Story page.
"""
title: String
metadata: StoryMetadata
"""
scrapedAt is the Time that the Story had it's metadata scraped at.
"""
scrapedAt: Time
"""
comments are the comments on the Story.
@@ -1245,11 +1293,6 @@ type Story {
"""
actionCounts: ActionCounts! @auth(roles: [ADMIN, MODERATOR])
"""
author is the authors listed in the meta tags for the Story.
"""
author: String
"""
closedAt is the Time that the Story is closed for commenting.
"""
@@ -1338,6 +1381,13 @@ type Query {
"""
discoverOIDCConfiguration(issuer: String!): DiscoveredOIDCConfiguration
@auth(roles: [ADMIN])
"""
debugScrapeStoryMetadata will return the information that Talk was able to
scrape from the given URL. No data will be saved related to the Story based
on this scrape.
"""
debugScrapeStoryMetadata(url: String!): StoryMetadata @auth(roles: [ADMIN])
}
################################################################################
@@ -1679,7 +1729,7 @@ SettingsInput is the partial type of the Settings type for performing mutations.
"""
input SettingsInput {
"""
domains will return a given list of whitelisted domains.
domains will return a given list of domains that hosts Stories.
"""
domains: [String!]
+3
View File
@@ -18,6 +18,9 @@ function getStreams() {
const logger = bunyan.createLogger({
name: "talk",
// Include file references in log entries.
src: true,
serializers,
streams: getStreams(),
level: config.get("logging_level") as LogLevelString,
+34 -28
View File
@@ -3,7 +3,10 @@ import uuid from "uuid";
import { Omit } from "talk-common/types";
import { dotize } from "talk-common/utils/dotize";
import { GQLCOMMENT_STATUS } from "talk-server/graph/tenant/schema/__generated__/types";
import {
GQLCOMMENT_STATUS,
GQLStoryMetadata,
} from "talk-server/graph/tenant/schema/__generated__/types";
import { EncodedActionCounts } from "talk-server/models/action";
import { ModerationSettings } from "talk-server/models/settings";
import { TenantResource } from "talk-server/models/tenant";
@@ -23,20 +26,21 @@ export interface CommentStatusCounts {
export interface Story extends TenantResource {
readonly id: string;
url: string;
scraped?: Date;
closedAt?: Date;
closedMessage?: string;
created_at: Date;
modified_date?: Date;
title?: string;
description?: string;
image?: string;
section?: string;
subsection?: string;
author?: string;
publication_date?: Date;
/**
* url is the URL to the Story page.
*/
url: string;
/**
* metadata stores the scraped metadata from the Story page.
*/
metadata?: GQLStoryMetadata;
/**
* scrapedAt is the Time that the Story had it's metadata scraped at.
*/
scrapedAt?: Date;
/**
* action_counts stores all the action counts for all Comment's on this Story.
@@ -54,6 +58,17 @@ export interface Story extends TenantResource {
* specific Story.
*/
settings?: Partial<ModerationSettings>;
/**
* closedAt is the date that the Story was forced closed at, or false to
* indicate that the story was re-opened.
*/
closedAt?: Date | false;
/**
* created_at is the date that the Story was added to the Talk database.
*/
created_at: Date;
}
export interface UpsertStoryInput {
@@ -68,8 +83,6 @@ export async function upsertStory(
) {
const now = new Date();
// TODO: verify that the url for the given Story is whitelisted by the tenant.
// Create the story, optionally sourcing the id from the input, additionally
// porting in the tenant_id.
const update: { $setOnInsert: Story } = {
@@ -85,7 +98,7 @@ export async function upsertStory(
// Perform the find and update operation to try and find and or create the
// story.
const { value: story } = await collection(db).findOneAndUpdate(
const result = await collection(db).findOneAndUpdate(
{
url,
tenant_id: tenantID,
@@ -100,15 +113,8 @@ export async function upsertStory(
returnOriginal: false,
}
);
if (!story) {
return null;
}
if (!story.scraped) {
// TODO: create scrape job to collect story metadata
}
return story;
return result.value || null;
}
/**
@@ -127,7 +133,7 @@ export async function updateCommentStatusCount(
id: string,
commentStatusCounts: Partial<CommentStatusCounts>
) {
const { value: story } = await collection(mongo).findOneAndUpdate(
const result = await collection(mongo).findOneAndUpdate(
{
id,
tenant_id: tenantID,
@@ -140,7 +146,7 @@ export async function updateCommentStatusCount(
{ returnOriginal: false }
);
return story;
return result.value || null;
}
function createEmptyCommentCounts(): CommentStatusCounts {
@@ -283,5 +289,5 @@ export async function updateStoryActionCounts(
{ returnOriginal: false }
);
return result.value;
return result.value || null;
}
@@ -1,3 +1,5 @@
import { DateTime } from "luxon";
import { Comment } from "talk-server/models/comment";
import { Story } from "talk-server/models/story";
import { Tenant } from "talk-server/models/tenant";
@@ -6,14 +8,28 @@ import { storyClosed } from "talk-server/services/comments/moderation/phases/sto
describe("storyClosed", () => {
it("throws an error when the story is closed", () => {
const story = { closedAt: new Date() };
expect(() =>
storyClosed({
story: { closedAt: new Date() } as Story,
tenant: {} as Tenant,
comment: {} as Comment,
author: {} as User,
})
).toThrow();
storyClosed({
story: {} as Story,
tenant: { autoCloseStream: true } as Tenant,
comment: {} as Comment,
author: {} as User,
});
expect(() =>
storyClosed({
story: story as Story,
tenant: (null as any) as Tenant,
comment: (null as any) as Comment,
author: (null as any) as User,
story: { created_at: new Date() } as Story,
tenant: { autoCloseStream: true, closedTimeout: -6000 } as Tenant,
comment: {} as Comment,
author: {} as User,
})
).toThrow();
});
@@ -23,19 +39,23 @@ describe("storyClosed", () => {
expect(
storyClosed({
story: { closedAt: new Date(now.getTime() + 60000) } as Story,
tenant: (null as any) as Tenant,
comment: (null as any) as Comment,
author: (null as any) as User,
story: {
closedAt: DateTime.fromJSDate(now)
.plus(60000)
.toJSDate(),
} as Story,
tenant: {} as Tenant,
comment: {} as Comment,
author: {} as User,
})
).toBeUndefined();
expect(
storyClosed({
story: {} as Story,
tenant: (null as any) as Tenant,
comment: (null as any) as Comment,
author: (null as any) as User,
tenant: {} as Tenant,
comment: {} as Comment,
author: {} as User,
})
).toBeUndefined();
});
@@ -6,10 +6,20 @@ import {
// This phase checks to see if the story being processed is closed or not.
export const storyClosed: IntermediateModerationPhase = ({
story,
tenant,
}): IntermediatePhaseResult | void => {
// Check to see if the story has closed commenting...
if (story.closedAt && story.closedAt.valueOf() <= Date.now()) {
// TODO: (wyattjoh) return better error.
throw new Error("story is currently closed for commenting");
}
if (
story.closedAt !== false &&
tenant.autoCloseStream &&
tenant.closedTimeout &&
story.created_at.valueOf() + tenant.closedTimeout <= Date.now()
) {
// TODO: (wyattjoh) return better error.
throw new Error("story is currently closed for commenting");
}
};
@@ -1,4 +1,5 @@
import Queue, { Job } from "bull";
import Logger from "bunyan";
import cheerio from "cheerio";
import authorScraper from "metascraper-author";
import dateScraper from "metascraper-date";
@@ -7,6 +8,7 @@ import imageScraper from "metascraper-image";
import titleScraper from "metascraper-title";
import { Db } from "mongodb";
import { GQLStoryMetadata } from "talk-server/graph/tenant/schema/__generated__/types";
import logger from "talk-server/logger";
import { updateStory } from "talk-server/models/story";
import Task from "talk-server/services/queue/Task";
@@ -25,10 +27,9 @@ export interface ScraperData {
tenantID: string;
}
const createJobProcessor = (
options: ScrapeProcessorOptions,
scraper: Scraper
) => async (job: Job<ScraperData>) => {
const createJobProcessor = (options: ScrapeProcessorOptions) => async (
job: Job<ScraperData>
) => {
// Pull out the job data.
const { storyID: id, storyURL: url, tenantID } = job.data;
@@ -44,8 +45,8 @@ const createJobProcessor = (
);
// Get the metadata from the scraped html.
const meta = await scraper.scrape(url);
if (!meta) {
const metadata = await scraper.scrape(url);
if (!metadata) {
logger.error(
{
job_id: job.id,
@@ -61,14 +62,8 @@ const createJobProcessor = (
// Update the Story with the scraped details.
const story = await updateStory(options.mongo, tenantID, id, {
title: meta.title || undefined,
description: meta.description || undefined,
image: meta.image ? meta.image : undefined,
author: meta.author || undefined,
publication_date: meta.date ? new Date(meta.date) : undefined,
modified_date: meta.modified ? new Date(meta.modified) : undefined,
section: meta.section || undefined,
scraped: new Date(),
metadata,
scrapedAt: new Date(),
});
if (!story) {
logger.error(
@@ -105,25 +100,40 @@ export type Rule = Record<
class Scraper {
private rules: Rule[];
private log: Logger;
constructor(rules: Rule[]) {
this.rules = rules;
this.log = logger.child({ taskName: "scraper" });
}
public async scrape(url: string) {
public async scrape(url: string): Promise<GQLStoryMetadata | null> {
// Grab the page HTML.
// TODO: investigate adding scraping proxy support.
const log = this.log.child({ storyURL: url });
const start = Date.now();
log.debug("starting scrape of Story");
// TODO: investigate adding scraping proxy support based on the Tenant.
const res = await fetch(url, {});
if (res.status !== 200) {
return;
log.warn(
{ statusCode: res.status },
"scrape failed with non-200 status code"
);
return null;
}
const html = await res.text();
log.debug({ timeElapsed: Date.now() - start }, "scrape complete");
// Load the DOM.
const htmlDom = cheerio.load(html);
log.debug("parsed html");
// Gather the results by evaluating each of the rules.
const metadata: Record<string, string | undefined> = {};
@@ -146,16 +156,26 @@ class Scraper {
}
}
return metadata;
log.debug("extracted metadata");
return {
title: metadata.title || undefined,
description: metadata.description || undefined,
image: metadata.image ? metadata.image : undefined,
author: metadata.author || undefined,
publishedAt: metadata.date ? new Date(metadata.date) : undefined,
modifiedAt: metadata.modified ? new Date(metadata.modified) : undefined,
section: metadata.section || undefined,
};
}
}
export function createScraperTask(
queue: Queue.QueueOptions,
options: ScrapeProcessorOptions
) {
// Create the scraper object.
const scraper = new Scraper([
/**
* createScraper will create a scraper that will utilize the rules defined to
* scrape metadata from the target page.
*/
function createScraper() {
return new Scraper([
authorScraper(),
dateScraper(),
descriptionScraper(),
@@ -164,10 +184,17 @@ export function createScraperTask(
modifiedScraper(),
sectionScraper(),
]);
}
export const scraper = createScraper();
export function createScraperTask(
queue: Queue.QueueOptions,
options: ScrapeProcessorOptions
) {
return new Task({
jobName: JOB_NAME,
jobProcessor: createJobProcessor(options, scraper),
jobProcessor: createJobProcessor(options),
queue,
});
}
@@ -2,6 +2,7 @@ import { Rules } from "metascraper";
export const modifiedScraper = (): Rules => ({
modified: [
// From: http://ogp.me/#type_article
({ htmlDom: $ }) => $('meta[property="article:modified"]').attr("content"),
],
});
@@ -2,6 +2,7 @@ import { Rules } from "metascraper";
export const sectionScraper = (): Rules => ({
section: [
// From: http://ogp.me/#type_article
({ htmlDom: $ }) => $('meta[property="article:section"]').attr("content"),
],
});
@@ -0,0 +1,61 @@
import { isURLPermitted } from "talk-server/services/stories";
it("denies when the tenant has no specified domains", () => {
const tenant = { domains: [] };
expect(isURLPermitted(tenant, "")).toEqual(false);
});
it("denies when tenant has a domain but not a valid url", () => {
const tenant = { domains: ["https://coralproject.net"] };
expect(isURLPermitted(tenant, "")).toEqual(false);
});
it("denies when there are multiple tenants domains and not a valid url", () => {
const tenant = {
domains: ["https://coralproject.net", "https://news.coralproject.net"],
};
expect(isURLPermitted(tenant, "")).toEqual(false);
});
it("denies when there are multiple tenants domains and a invalid url", () => {
const tenant = {
domains: ["https://coralproject.net", "https://news.coralproject.net"],
};
expect(
isURLPermitted(tenant, "https://blog.coralproject.net/a/page")
).toEqual(false);
});
it("allows when there are multiple tenants domains and a valid url", () => {
const tenant = {
domains: ["https://coralproject.net", "https://news.coralproject.net"],
};
expect(
isURLPermitted(tenant, "https://news.coralproject.net/a/page")
).toEqual(true);
});
it("allows when there are multiple prefix domains and a valid url", () => {
const tenant = {
domains: ["coralproject.net", "news.coralproject.net"],
};
expect(isURLPermitted(tenant, "http://news.coralproject.net/a/page")).toEqual(
true
);
});
it("allows when there are some prefix domains and a valid url", () => {
const tenant = {
domains: ["http://coralproject.net", "news.coralproject.net"],
};
expect(
isURLPermitted(tenant, "https://news.coralproject.net/a/page")
).toEqual(true);
});
+54 -1
View File
@@ -1,5 +1,12 @@
import { Db } from "mongodb";
import {
doesRequireSchemePrefixing,
getOrigin,
isURLSecure,
prefixSchemeIfRequired,
} from "talk-server/app/url";
import logger from "talk-server/logger";
import {
findOrCreateStory,
FindOrCreateStoryInput,
@@ -16,6 +23,16 @@ export async function findOrCreate(
input: FindOrCreateStory,
scraper: Task<ScraperData>
) {
// If the URL is provided, and the url is not on a allowed domain, then refuse
// to create the Asset.
if (input.url && !isURLPermitted(tenant, input.url)) {
logger.warn(
{ storyURL: input.url, tenantDomains: tenant.domains },
"provided story url was not in the list of permitted tenant domains"
);
return null;
}
// TODO: check to see if the tenant has enabled lazy story creation.
const story = await findOrCreateStory(db, tenant.id, input);
@@ -23,7 +40,8 @@ export async function findOrCreate(
return null;
}
if (!story.scraped) {
if (!story.scrapedAt) {
// If the scraper has not scraped this story, we need to scrape it now!
await scraper.add({
storyID: story.id,
storyURL: story.url,
@@ -33,3 +51,38 @@ export async function findOrCreate(
return story;
}
/**
* isURLInsideAllowedDomains will validate if the given origin is allowed given
* the Tenant's domain configuration.
*/
export function isURLPermitted(
tenant: Pick<Tenant, "domains">,
targetURL: string
) {
// If there aren't any domains, then we reject it, because no url we have can
// satisfy those requirements.
if (tenant.domains.length === 0) {
return false;
}
// If the scheme can not be inferred, then we can't determine the
// admissability of the url.
if (doesRequireSchemePrefixing(targetURL)) {
return false;
}
// Determine the scheme of the targetOrigin. We know that the targetURL does
// not need prefixing, so it can only be true/false here.
const originSecure = isURLSecure(targetURL) as boolean;
// Extract the origin from the URL.
const targetOrigin = getOrigin(targetURL);
// Loop over all the Tenant domains provided. Prefix the domain of each if it
// is required with the target url scheme. Return if at least one match is
// found within the Tenant domains.
return tenant.domains
.map(domain => getOrigin(prefixSchemeIfRequired(originSecure, domain)))
.some(origin => origin === targetOrigin);
}