From 9d15681d20dfc53f56e12e271f37bb0675fa4cc7 Mon Sep 17 00:00:00 2001 From: Stephen Haberman Date: Thu, 9 May 2024 08:19:39 -0500 Subject: [PATCH] fix: Add tests for previous reactive hint fix, with more fixes. This adds a test to reproduce the prior "walk through a different subtype that does not have the relation" error, as well as applies the fix to m2o and m2m handling as well. There is also a related fix that runtime filtering of the type filter, i.e. `publisher@Publisher` should pass for subtypes of `Publisher` like either a `LargePublisher` or `SmallPublisher`, and previously we were only exact-matching on the type name. --- packages/orm/src/EntityMetadata.ts | 5 + packages/orm/src/reactiveHints.ts | 78 ++++++--- .../src/EntityManager.reactiveRules.test.tsx | 9 ++ .../tests/integration/src/entities/Critic.ts | 4 +- .../integration/src/reactiveHints.test.ts | 153 ++++++++++++------ 5 files changed, 176 insertions(+), 73 deletions(-) diff --git a/packages/orm/src/EntityMetadata.ts b/packages/orm/src/EntityMetadata.ts index 9b628ebc4..9b738561d 100644 --- a/packages/orm/src/EntityMetadata.ts +++ b/packages/orm/src/EntityMetadata.ts @@ -215,6 +215,11 @@ export function getBaseSelfAndSubMetas(meta: EntityMetadata): EntityMetadata[] { return [...meta.baseTypes, meta, ...meta.subTypes]; } +export function getSubMetas(meta: EntityMetadata): EntityMetadata[] { + // We should do recursion at some point + return meta.subTypes; +} + export function getBaseMeta(meta: EntityMetadata): EntityMetadata { if (!meta.baseType) { return meta; diff --git a/packages/orm/src/reactiveHints.ts b/packages/orm/src/reactiveHints.ts index 6aa1b57b6..649491e6f 100644 --- a/packages/orm/src/reactiveHints.ts +++ b/packages/orm/src/reactiveHints.ts @@ -1,6 +1,16 @@ import { Entity } from "./Entity"; import { FieldsOf, MaybeAbstractEntityConstructor, RelationsOf, getEmInternalApi } from "./EntityManager"; -import { EntityMetadata, getBaseAndSelfMetas, getMetadata } from "./EntityMetadata"; +import { + EntityMetadata, + ManyToManyField, + ManyToOneField, + OneToManyField, + OneToOneField, + PolymorphicFieldComponent, + getBaseAndSelfMetas, + getMetadata, + getSubMetas, +} from "./EntityMetadata"; import { Changes, FieldStatus, ManyToOneFieldStatus } from "./changes"; import { isChangeableField } from "./fields"; import { getProperties } from "./getProperties"; @@ -146,9 +156,10 @@ export function reverseReactiveHint( if (!isReadOnly) { fields.push(field.fieldName); } + const otherFieldName = maybeAddTypeFilterSuffix(meta, field); return reverseReactiveHint(rootType, field.otherMetadata().cstr, subHint, undefined, false).map( ({ entity, fields, path }) => { - return { entity, fields, path: [...path, field.otherFieldName] }; + return { entity, fields, path: [...path, otherFieldName] }; }, ); } @@ -161,21 +172,18 @@ export function reverseReactiveHint( return field.components.flatMap((comp) => { return reverseReactiveHint(rootType, comp.otherMetadata().cstr, subHint, undefined, false).map( ({ entity, fields, path }) => { - return { entity, fields, path: [...path, comp.otherFieldName] }; + const otherFieldName = maybeAddTypeFilterSuffix(meta, comp); + return { entity, fields, path: [...path, otherFieldName] }; }, ); }); } case "m2m": { - const otherField = - field.otherMetadata().allFields[field.otherFieldName] ?? - fail(`No field ${field.otherMetadata().type}.${field.otherFieldName}`); - const otherFieldName = - otherField.kind === "poly" ? `${field.otherFieldName}@${meta.type}` : field.otherFieldName; // While o2m and o2o can watch for just FK changes by passing `reactForOtherSide` (the FK lives in the other // table), for m2m reactivity we push the collection name into the reactive hint, because it's effectively // "the other/reverse side", and JoinRows will trigger it explicitly instead of `setField` for non-m2m keys. fields.push(field.fieldName); + const otherFieldName = maybeAddTypeFilterSuffix(meta, field); return reverseReactiveHint( rootType, field.otherMetadata().cstr, @@ -191,10 +199,7 @@ export function reverseReactiveHint( case "o2m": case "o2o": { const isOtherReadOnly = field.otherMetadata().allFields[field.otherFieldName].immutable; - const otherFieldName = - field.otherMetadata().allFields[field.otherFieldName].kind === "poly" || meta.baseType - ? `${field.otherFieldName}@${meta.type}` - : field.otherFieldName; + const otherFieldName = maybeAddTypeFilterSuffix(meta, field); // This is not a field, but we want our reverse side to be reactive, so pass reactForOtherSide return reverseReactiveHint( rootType, @@ -252,6 +257,21 @@ export function reverseReactiveHint( ]; } +function maybeAddTypeFilterSuffix( + meta: EntityMetadata, + field: ManyToOneField | OneToManyField | ManyToManyField | OneToOneField | PolymorphicFieldComponent, +): string { + // If we're Foo, and the other field (which we'll traverse back from at runtime) is actually + // a poly FK pointing back to multiple `owner=Foo | Bar | Zaz`, add a suffix of `@Foo` so + // that any runtime traversal that hit `owner` and see a `Bar | Zaz` will stop. + const nextFieldIsPoly = field.otherMetadata().allFields[field.otherFieldName].kind === "poly"; + // If we're a SubType, and the other field points to our base type, assume there might be + // SubType-specific fields in the path so far, so tell the other side to only walk back through + // us if its value is actually a SubType. + const nextFieldPointsToBaseType = !!meta.baseType && field.otherMetadata().cstr !== meta.cstr; + return nextFieldIsPoly || nextFieldPointsToBaseType ? `${field.otherFieldName}@${meta.type}` : field.otherFieldName; +} + /** * Walks `reverseHint` for every entity in `entities`. * @@ -264,11 +284,11 @@ export async function followReverseHint(entities: Entity[], reverseHint: string[ // And "walk backwards" through the reverse hint while (paths.length) { const path = paths.shift()!; - const [fieldName, viaPolyType] = path.split("@"); + const [fieldName, viaType] = path.split("@"); // The path might touch either a reference or a collection const entitiesOrLists = await Promise.all( current.flatMap((c: any) => { - const currentValuePromise = maybeLoadedPoly(c[fieldName].load(), viaPolyType); + const currentValuePromise = maybeApplyTypeFilter(c[fieldName].load(), viaType); // If we're going from Book.author back to Author to re-validate the Author.books collection, // see if Book.author has changed, so we can re-validate both the old author's books and the // new author's books. @@ -279,7 +299,7 @@ export async function followReverseHint(entities: Entity[], reverseHint: string[ if (isReference && changed && changed.hasUpdated && changed.originalValue) { return [ currentValuePromise, - maybeLoadedPoly((changed as ManyToOneFieldStatus).originalEntity, viaPolyType), + maybeApplyTypeFilter((changed as ManyToOneFieldStatus).originalEntity, viaType), ]; } if (isManyToMany) { @@ -374,14 +394,34 @@ export interface ReactiveTarget { path: string[]; } -async function maybeLoadedPoly(loadPromise: Promise, viaPolyType: string | undefined) { - if (viaPolyType) { - const loaded: Entity = await loadPromise; - return loaded && getMetadata(loaded).type === viaPolyType ? loaded : undefined; +function maybeApplyTypeFilter(loadPromise: Promise, viaType: string | undefined) { + if (viaType) { + return loadPromise.then((loaded) => { + if (Array.isArray(loaded)) { + return loaded.filter((e) => isTypeOrSubType(e, viaType)); + } else if (loaded && isTypeOrSubType(loaded, viaType)) { + return loaded; + } else { + return undefined; + } + }); } return loadPromise; } +/** Handle `viaType` filtering with subtype awareness. */ +function isTypeOrSubType(entity: Entity, typeName: string): boolean { + const meta = getMetadata(entity); + // Easy check for the name is the same + if (meta.type === typeName) return true; + // Otherwise see if the entity is a subtype of the typeName, i.e. if our poly/type + // filter is `@Publisher`, and we're a `SmallPublisher`, that's valid to traverse. + for (const other of getSubMetas(meta)) { + if (other.type === typeName) return true; + } + return false; +} + export function isPolyHint(key: string): boolean { return key.includes("@"); } diff --git a/packages/tests/integration/src/EntityManager.reactiveRules.test.tsx b/packages/tests/integration/src/EntityManager.reactiveRules.test.tsx index 7183a8a74..5ffcd016b 100644 --- a/packages/tests/integration/src/EntityManager.reactiveRules.test.tsx +++ b/packages/tests/integration/src/EntityManager.reactiveRules.test.tsx @@ -8,6 +8,7 @@ import { newBook, newBookReview, newPublisher, + newSmallPublisher, newTag, SmallPublisher, Tag, @@ -209,6 +210,14 @@ describe("EntityManager.reactiveRules", () => { expect(b.reviewsRuleInvoked).toBe(3); }); + it.withCtx("skips traversing through subtype-only relations", async ({ em }) => { + // Given we trigger an Image -> publishers -> critics (only exists on LargePublishers) + newSmallPublisher(em, { images: [{}] }); + // Then it does not blow up (and we can't assert against the Critic rule having executed + // b/c the scenario is that SmallPublishers don't have the `critics` relation) + await em.flush(); + }); + it.withCtx("creates the right reactive rules", async () => { const fn = expect.any(Function); expect(getReactiveRules(Author)).toMatchObject([ diff --git a/packages/tests/integration/src/entities/Critic.ts b/packages/tests/integration/src/entities/Critic.ts index baeb1422e..f49e02cee 100644 --- a/packages/tests/integration/src/entities/Critic.ts +++ b/packages/tests/integration/src/entities/Critic.ts @@ -2,5 +2,5 @@ import { CriticCodegen, criticConfig as config } from "./entities"; export class Critic extends CriticCodegen {} -// remove once you have actual rules/hooks -config.placeholder(); +/** For testing walking through subtype relations. */ +config.addRule({ favoriteLargePublisher: "images" }, () => {}); diff --git a/packages/tests/integration/src/reactiveHints.test.ts b/packages/tests/integration/src/reactiveHints.test.ts index 56778c7c6..8f867106a 100644 --- a/packages/tests/integration/src/reactiveHints.test.ts +++ b/packages/tests/integration/src/reactiveHints.test.ts @@ -1,116 +1,151 @@ -import { Author, Book, BookReview, Comment, Image, Tag } from "@src/entities"; -import { LoadHint, Loaded, Reacted, ReactiveHint, getMetadata, reverseReactiveHint } from "joist-orm"; -import { convertToLoadHint } from "joist-orm/build/reactiveHints"; +import { Author, Book, BookReview, Comment, Critic } from "@src/entities"; +import { + Entity, + LoadHint, + Loaded, + MaybeAbstractEntityConstructor, + Reacted, + ReactiveHint, + getMetadata, + reverseReactiveHint, +} from "joist-orm"; +import { ReactiveTarget, convertToLoadHint } from "joist-orm/build/reactiveHints"; const am = getMetadata(Author); describe("reactiveHints", () => { it("can do immediate primitive field names", () => { - expect(reverseReactiveHint(Author, Author, "firstName")).toEqual([ - { entity: Author, fields: ["firstName"], path: [] }, - ]); + expect(reverse(Author, Author, "firstName")).toEqual([{ entity: "Author", fields: ["firstName"], path: [] }]); }); it("can do parent primitive field names", () => { - expect(reverseReactiveHint(Book, Book, { author: ["firstName", "lastName"] })).toEqual([ - { entity: Book, fields: ["author"], path: [] }, - { entity: Author, fields: ["firstName", "lastName"], path: ["books"] }, + expect(reverse(Book, Book, { author: ["firstName", "lastName"] })).toEqual([ + { entity: "Book", fields: ["author"], path: [] }, + { entity: "Author", fields: ["firstName", "lastName"], path: ["books"] }, ]); }); it("can do grand-parent primitive field names", () => { - expect(reverseReactiveHint(BookReview, BookReview, { book: { author: ["firstName", "lastName"] } })).toEqual([ - { entity: BookReview, fields: [], path: [] }, - { entity: Book, fields: ["author"], path: ["reviews"] }, - { entity: Author, fields: ["firstName", "lastName"], path: ["books", "reviews"] }, + expect(reverse(BookReview, BookReview, { book: { author: ["firstName", "lastName"] } })).toEqual([ + { entity: "BookReview", fields: [], path: [] }, + { entity: "Book", fields: ["author"], path: ["reviews"] }, + { entity: "Author", fields: ["firstName", "lastName"], path: ["books", "reviews"] }, ]); }); it("can do parent and grand-parent primitive field names", () => { - expect( - reverseReactiveHint(BookReview, BookReview, { book: { title: {}, author: ["firstName", "lastName"] } }), - ).toEqual([ - { entity: BookReview, fields: [], path: [] }, - { entity: Book, fields: ["title", "author"], path: ["reviews"] }, - { entity: Author, fields: ["firstName", "lastName"], path: ["books", "reviews"] }, + expect(reverse(BookReview, BookReview, { book: { title: {}, author: ["firstName", "lastName"] } })).toEqual([ + { entity: "BookReview", fields: [], path: [] }, + { entity: "Book", fields: ["title", "author"], path: ["reviews"] }, + { entity: "Author", fields: ["firstName", "lastName"], path: ["books", "reviews"] }, ]); }); it("can do child o2m with primitive field names", () => { - expect(reverseReactiveHint(Author, Author, { books: "title" })).toEqual([ + expect(reverse(Author, Author, { books: "title" })).toEqual([ // Include the Author so that if no books are added, the rule still rules on create - { entity: Author, fields: [], path: [] }, - { entity: Book, fields: ["author", "title"], path: ["author"] }, + { entity: "Author", fields: [], path: [] }, + { entity: "Book", fields: ["author", "title"], path: ["author"] }, ]); }); it("can do child o2m with w/o any fields", () => { - expect(reverseReactiveHint(Author, Author, "books")).toEqual([ - { entity: Author, fields: [], path: [] }, - { entity: Book, fields: ["author"], path: ["author"] }, + expect(reverse(Author, Author, "books")).toEqual([ + { entity: "Author", fields: [], path: [] }, + { entity: "Book", fields: ["author"], path: ["author"] }, ]); }); it("can do child o2o with primitive field names", () => { - expect(reverseReactiveHint(Author, Author, { image: "fileName" })).toEqual([ - { entity: Author, fields: [], path: [] }, - { entity: Image, fields: ["author", "fileName"], path: ["author"] }, + expect(reverse(Author, Author, { image: "fileName" })).toEqual([ + { entity: "Author", fields: [], path: [] }, + { entity: "Image", fields: ["author", "fileName"], path: ["author"] }, ]); }); it("can do child m2m with primitive field names", () => { - expect(reverseReactiveHint(Book, Book, { tags: "name" })).toEqual([ - { entity: Book, fields: ["tags"], path: [] }, - { entity: Tag, fields: ["name"], path: ["books"] }, + expect(reverse(Book, Book, { tags: "name" })).toEqual([ + { entity: "Book", fields: ["tags"], path: [] }, + { entity: "Tag", fields: ["name"], path: ["books"] }, ]); }); it("can do nested child m2m with primitive field names", () => { - expect(reverseReactiveHint(Author, Author, { books: { tags: "name" } })).toEqual([ - { entity: Author, fields: [], path: [] }, - { entity: Book, fields: ["author", "tags"], path: ["author"] }, - { entity: Tag, fields: ["name"], path: ["books", "author"] }, + expect(reverse(Author, Author, { books: { tags: "name" } })).toEqual([ + { entity: "Author", fields: [], path: [] }, + { entity: "Book", fields: ["author", "tags"], path: ["author"] }, + { entity: "Tag", fields: ["name"], path: ["books", "author"] }, ]); }); it("can do via polymorphic reference", () => { - expect(reverseReactiveHint(Author, Author, { books: { comments: "text" } })).toEqual([ - { entity: Author, fields: [], path: [] }, - { entity: Book, fields: ["author"], path: ["author"] }, - { entity: Comment, fields: ["parent", "text"], path: ["parent@Book", "author"] }, + expect(reverse(Author, Author, { books: { comments: "text" } })).toEqual([ + { entity: "Author", fields: [], path: [] }, + { entity: "Book", fields: ["author"], path: ["author"] }, + { entity: "Comment", fields: ["parent", "text"], path: ["parent@Book", "author"] }, ]); }); - it("skips read-only m2o parents", () => { - expect(reverseReactiveHint(Book, Book, { author_ro: "firstName:ro" })).toEqual([ - { entity: Book, fields: [], path: [] }, + it("can do via subtype-only poly relation", () => { + // User.favoritePublisher is a poly, so filter on LargePublisher to avoid smallPublisher.critics + expect(reverse(Critic, Critic, { favoriteLargePublisher: "users" })).toEqual([ + { entity: "Critic", fields: ["favoriteLargePublisher"], path: [] }, + { entity: "User", fields: ["favoritePublisher"], path: ["favoritePublisher@LargePublisher", "critics"] }, + ]); + }); + + it("can do via subtype-only m2o relation", () => { + // Image.publisher points to any publishers, so filter on LargePublisher to avoid smallPublisher.critics + expect(reverse(Critic, Critic, { favoriteLargePublisher: "images" })).toEqual([ + { entity: "Critic", fields: ["favoriteLargePublisher"], path: [] }, + { entity: "Image", fields: ["publisher"], path: ["publisher@LargePublisher", "critics"] }, ]); }); + it("can do via subtype-only o2m relation", () => { + // PublisherGroup.publishers points to any publishers, so filter on LargePublisher to avoid smallPublisher.critics + expect(reverse(Critic, Critic, { favoriteLargePublisher: { group: "publishers" } })).toEqual([ + { entity: "Critic", fields: ["favoriteLargePublisher"], path: [] }, + { entity: "LargePublisher", fields: ["group"], path: ["critics"] }, + { entity: "Publisher", fields: ["group"], path: ["group", "publishers@LargePublisher", "critics"] }, + ]); + }); + + it("can do via subtype-only m2m relation", () => { + // Tag.publishers points to any publishers, so filter on LargePublisher to avoid smallPublisher.critics + expect(reverse(Critic, Critic, { favoriteLargePublisher: { tags: "name" } })).toEqual([ + { entity: "Critic", fields: ["favoriteLargePublisher"], path: [] }, + { entity: "LargePublisher", fields: ["tags"], path: ["critics"] }, + { entity: "Tag", fields: ["name"], path: ["publishers@LargePublisher", "critics"] }, + ]); + }); + + it("skips read-only m2o parents", () => { + expect(reverse(Book, Book, { author_ro: "firstName:ro" })).toEqual([{ entity: "Book", fields: [], path: [] }]); + }); + it("skips read-only o2m children and grand-children", () => { - expect(reverseReactiveHint(Author, Author, { books_ro: "reviews:ro", firstName_ro: {} })).toEqual([ - { entity: Author, fields: [], path: [] }, + expect(reverse(Author, Author, { books_ro: "reviews:ro", firstName_ro: {} })).toEqual([ + { entity: "Author", fields: [], path: [] }, ]); }); it("can do read-only string hint", () => { // expect(reverseHint(Author, "books:ro")).toEqual([{ entity: Book, fields: ["author"], path: ["author"] }]); - expect(reverseReactiveHint(Author, Author, "publisher:ro")).toEqual([{ entity: Author, fields: [], path: [] }]); + expect(reverse(Author, Author, "publisher:ro")).toEqual([{ entity: "Author", fields: [], path: [] }]); }); it("can do array of read-only string hints", () => { - expect(reverseReactiveHint(Author, Author, ["firstName:ro", "publisher:ro"])).toEqual([ - { entity: Author, fields: [], path: [] }, + expect(reverse(Author, Author, ["firstName:ro", "publisher:ro"])).toEqual([ + { entity: "Author", fields: [], path: [] }, ]); }); it("can do hash of read-only hints", () => { // TODO Enforce that `name` must be `name:ro` - expect(reverseReactiveHint(Author, Author, { publisher_ro: "name:ro" })).toEqual([ - { entity: Author, fields: [], path: [] }, - ]); - expect(reverseReactiveHint(BookReview, BookReview, { book: "author:ro" })).toEqual([ - { entity: BookReview, fields: [], path: [] }, + expect(reverse(Author, Author, { publisher_ro: "name:ro" })).toEqual([{ entity: "Author", fields: [], path: [] }]); + expect(reverse(BookReview, BookReview, { book: "author:ro" })).toEqual([ + { entity: "BookReview", fields: [], path: [] }, ]); }); @@ -262,3 +297,17 @@ describe("reactiveHints", () => { } }); }); + +/** Calls reverseReactiveHint but swaps the entity with a string name to placate Jest. */ +function reverse( + rootType: MaybeAbstractEntityConstructor, + entityType: MaybeAbstractEntityConstructor, + hint: ReactiveHint, + reactForOtherSide?: string | boolean, + isFirst: boolean = true, +): ReactiveTarget[] { + return reverseReactiveHint(rootType, entityType, hint, reactForOtherSide, isFirst).map((target) => { + (target as any).entity = target.entity.name; + return target; + }); +}