Skip to content

Commit

Permalink
fix(author): fail on create due to functions as enumerable properties…
Browse files Browse the repository at this point in the history
… overridden in Object.assign (#3600)

*alternate solution to #3578, which had to be reverted because [this.content in GdocPost](https://github.com/owid/owid-grapher/blob/3691a52abfa41b34d7853df84df3329d80a9a187/db/model/Gdoc/GdocPost.ts#L34-L36) would  be honored by _.defaults as a property not to override, but would result in the `content` property not respecting `OwidGdocContent` at run time.*

### What changed?

This PR promotes GdocBase instance-level functions (e.g. `_enrichSubclassContent` as class methods.

### Why make this change?

GdocAuthor (but also GdocPost, GdocHomepage and Gdoc DataInsight) are created from a GdocBase instance (only when inserting a new Gdoc) using `Object.assign(gdocAuthor, gdocBase)`. This call lists all enumerable properties on GdocBase, including instance-level functions such as `_enrichSubclassContent`, and overrides the subclass version.

This leads to the subclass enrichment not being performed, and in the case of author creation, an error.

## Follow-up

Promoting instance-level functions to class methods on `GdocBase` is enough to solve this issue, but we should consider whether to apply the same treatment to the GdocBase subclasses too.
  • Loading branch information
mlbrgl authored May 17, 2024
2 parents de17c42 + c34498e commit 6135c80
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 24 deletions.
27 changes: 27 additions & 0 deletions db/model/Gdoc/GdocAuthor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,33 @@ export class GdocAuthor extends GdocBase implements OwidGdocAuthorInterface {

static create(obj: OwidGdocBaseInterface): GdocAuthor {
const gdoc = new GdocAuthor()
// We need to prevent obj methods from overriding GdocAuthor methods.
// This happens when createGdocAndInsertIntoDb() passes a GdocBase
// instance to loadGdocFromGdocBase(), instead of a simple object. In
// this case, simply assigning obj to gdoc would override GdocAuthor
// methods with GdocBase methods, in particular the
// _enrichedSubclassContent. When creating a new author,
// _enrichedSubclassContent would run from the base GdocBase class
// instead of the GdocAuthor subclass, and the socials block would not
// be shaped as an ArchieML block, thus throwing an error.

// A first approach to avoid this would be to use Object.assign(), while
// omitting functions:
// Object.assign(gdoc, omitBy(obj, isFunction))

// A better approach is to avoid registering these functions as
// enumerable properties in the first place, so they are not listed by
// Object.assign(). The simplest way to do this is to define them as
// class methods, which are not enumerable, and instead attached to the
// class prototype. When we call Object.assign(gdoc, obj), these methods
// are ignored. Even after the assign, gdoc._enrichedSubclassContent()
// will then still accurately target the GdocAuthor method, and not the
// GdocBase method.

// see
// https://github.com/owid/owid-grapher/pull/3600#issuecomment-2116990248
// for a more detailed presentation on this topic

Object.assign(gdoc, obj)
return gdoc
}
Expand Down
39 changes: 26 additions & 13 deletions db/model/Gdoc/GdocBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
Span,
traverseEnrichedSpan,
uniq,
identity,
OwidGdocBaseInterface,
OwidGdocPublicationContext,
BreadcrumbItem,
Expand Down Expand Up @@ -74,26 +73,40 @@ export class GdocBase implements OwidGdocBaseInterface {
linkedIndicators: Record<number, LinkedIndicator> = {}
linkedDocuments: Record<string, OwidGdocMinimalPostInterface> = {}
latestDataInsights: MinimalDataInsightInterface[] = []

_getSubclassEnrichedBlocks: (gdoc: typeof this) => OwidEnrichedGdocBlock[] =
() => []
_enrichSubclassContent: (content: Record<string, any>) => void = identity
_validateSubclass: (
knex: db.KnexReadonlyTransaction,
gdoc: typeof this
) => Promise<OwidGdocErrorMessage[]> = async () => []
_omittableFields: string[] = []

_loadSubclassAttachments: (
knex: db.KnexReadonlyTransaction
) => Promise<void> = async () => undefined

constructor(id?: string) {
if (id) {
this.id = id
}
}

/******************************************************************
* !! Use methods instead of functions as enumerable properties *
* (see GdocAuthor.ts for rationale) *
******************************************************************/

_getSubclassEnrichedBlocks(_gdoc: typeof this): OwidEnrichedGdocBlock[] {
return []
}

_enrichSubclassContent(_content: Record<string, any>): void {
return
}

async _validateSubclass(
_knex: db.KnexReadonlyTransaction,
_gdoc: typeof this
): Promise<OwidGdocErrorMessage[]> {
return []
}

async _loadSubclassAttachments(
_knex: db.KnexReadonlyTransaction
): Promise<void> {
return
}

protected typeSpecificFilenames(): string[] {
return []
}
Expand Down
5 changes: 1 addition & 4 deletions db/model/Gdoc/GdocDataInsight.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,7 @@ export class GdocDataInsight
content!: OwidGdocDataInsightContent

constructor(id?: string) {
super()
if (id) {
this.id = id
}
super(id)
}

static create(obj: OwidGdocBaseInterface): GdocDataInsight {
Expand Down
10 changes: 7 additions & 3 deletions db/model/Gdoc/GdocFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ export async function createGdocAndInsertIntoDb(
// We have to fetch it here because we need to know the type of the Gdoc in load()
const base = new GdocBase(id)
await base.fetchAndEnrichGdoc()
await upsertGdoc(knex, base)

// Load its metadata and state so that subclass parsing & validation is also done.
// This involves a second call to the DB and Google, which makes me sad, but it'll do for now.
Expand All @@ -107,8 +106,13 @@ export async function createGdocAndInsertIntoDb(
GdocsContentSource.Gdocs
)

// 2024-03-12 Daniel: We used to save here before the knex refactor but I think that was redundant?
// await gdoc.save()
// Save the enriched Gdoc to the database (including subclass-specific
// enrichments, cf. _enrichSubclassContent()). Otherwise subclass
// enrichments are not present on the Gdoc subclass when loading from the DB
// (GdocsContentSource.Internal), since subclass enrichements are only done
// while fetching the live gdocs (GdocsContentSource.Gdocs) in
// loadGdocFromGdocBase().
await upsertGdoc(knex, gdoc)

return gdoc
}
Expand Down
5 changes: 1 addition & 4 deletions db/model/Gdoc/GdocHomepage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@ export class GdocHomepage
content!: OwidGdocHomepageContent

constructor(id?: string) {
super()
if (id) {
this.id = id
}
super(id)
}

static create(obj: OwidGdocBaseInterface): GdocHomepage {
Expand Down

0 comments on commit 6135c80

Please sign in to comment.