Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data Model Classes #2092

Merged
merged 37 commits into from Mar 25, 2024
Merged

Data Model Classes #2092

merged 37 commits into from Mar 25, 2024

Conversation

jdorn
Copy link
Member

@jdorn jdorn commented Feb 1, 2024

Overview

New data model classes to enforce best practices and reduce boilerplate code required to create new models. For an initial test, this PR converts the FactMetricModel to use the new class.

Example

Everything starts from a single Zod schema, representing the whole object:

// File: back-end/src/validators/foo.ts
import { z } from "zod";

export fooSchema = z.object({
  id: z.string(),
  organization: z.string(),
  dateCreated: z.date(),
  dateUpdated: z.date(),
  name: z.string(),
  // Use default() for properties that are optional when creating
  projects: z.array(z.string()).default([])
}).strict();

From this, you can export types to use throughout the front-end and back-end like we do today.

// File: back-end/types/foo.d.ts
import { z } from "zod";
import type { fooSchema } from "../src/validators/foo";
import { CreateProps, UpdateProps } from "./models";

// Full interface
export type FooInterface = z.infer<fooSchema>;

// Helper types for the subset of fields that can be set when creating/updating
// For example, you can't update 'id' after it's been created
export type CreateFooProps = CreateProps<FooInterface>;
export type UpdateFooProps = UpdateProps<FooInterface>;

Create the data model class based on the schema.

// File: back-end/src/models/FooModel.ts
import { fooSchema } from "../validators/foo";

export class FooDataModel extends BaseModel<typeof fooSchema> {
  protected getConfig() {
    const config: ModelConfig<typeof fooSchema> = {
      schema: fooSchema,
      collectionName: "foos",
      idPrefix: "foo_",
      auditLog: {
        entity: "foo",
        createEvent: "foo.create",
        updateEvent: "foo.update",
        deleteEvent: "foo.delete",
      },
      // Either "single", "multiple", or "none"
      projectScoping: "multiple",
      // If true, `id` is unique.  If false (default), the `organization`/`id` combo is unique.
      globallyUniqueIds: false,
    };
    return config;
  }

  // CRUD permission checks
  protected canCreate(doc: FooInterface): boolean {
    return this.context.permissions.canCreateFoo(doc);
  }
  protected canRead(doc: FooInterface): boolean {
    return this.context.permissions.canReadFoo(doc);
  }
  protected canUpdate(existing: FooInterface, updates: UpdateProps<FooInterface>): boolean {
    return this.context.permissions.canUpdateFoo(existing, updates);
  }
  protected canDelete(doc: FooInterface): boolean {
    return this.context.permissions.canDeleteFoo(doc);
  }
}

Add it to the Request Context class:

export class ReqContextClass {
  public models!: {
    // ...
    foos: FooDataModel;
  };
  private initModels() {
    this.models = {
      // ...
      foos: new FooDataModel(this),
    };
  }
  // ...
}

And then, you can access it anywhere you have context and use the built-in methods:

const foo = await context.models.foos.getById("foo_123");
if (!foo) throw new Error("Could not find foo");

console.log("Old Name: ", foo.name);

const newFoo = await context.models.foos.update(foo, {name: "New Name"});
console.log("New Name: ", newFoo.name);

Features

Built-in Public Methods

The following public methods are created automatically:

  • getAll()
  • getAllByProject(project)
  • getById(id)
  • getByIds(ids)
  • create(data)
  • update(existing, changes)
  • updateById(id, changes)
  • delete(existing)
  • deleteById(id)

The create, update, and updateById methods accept either strongly typed arguments or unknown. Everything is validated before saving, so this is safe. This is useful when you want to pass in req.body directly without manually doing validation.

Custom Methods

You can add more tailored data fetching methods as needed by referencing the _findOne and _find methods. There are similar protected methods for write operations, although those are rarely needed.

Here's an example:

export class FooDataModel extends BaseModel<typeof fooSchema> {
  // ...
  
  public getByNames(names: string[]) {
    return this._find({name: {$in: names}});
  }
}

Note: Permission checks, migrations, etc. are all done automatically within the _find method, so you don't need to repeat any of that in your custom methods. Also, the organization field is automatically added to every query, so it will always be multi-tenant safe.

Hooks

The following hooks are available, letting you add additional validation or perform trigger-like behavior without messing with data model internals. All of these besides migrate are async. Define these in your child class and they will be called at appropriate times.

  • migrate(legacyObj): newObj
  • customValidation(obj) (called for both update/create flows)
  • beforeCreate(newObj)
  • afterCreate(newObj)
  • beforeUpdate(existing, updates, newObj)
  • afterUpdate(existing, updates, newObj)
  • beforeDelete(existing)
  • afterDelete(existing)

Here's an example:

export class FooDataModel extends BaseModel<typeof fooSchema> {
  // ...
  
  protected async beforeDelete(existing: FooInterface) {
    const barsUsingFoo = await getAllBarsUsingFoo(this.context, existing.id);
    if (bars.length > 0) {
      throw new Error("Cannot delete. One or more bars is linking to this foo");
    }
  }
}

Built-In Best Practices

Includes the following best practices:

  • Everything is strongly typed and validated before saving
  • Mongo indexes for common access patterns (org + id) plus additional custom indexes
  • Automatically check permissions on both read and write
  • Multi-tenant safe (auto-adds organization to every query)
  • Creates audit log entries for all write actions
  • Automatically runs JIT migrations
  • Keep dateUpdated and dateCreated up-to-date
  • Registers any new tags that are being used
  • Makes sure common foreign key references are valid (e.g. projectIds exist)

Future TODOs

  • Ability to run real migrations (persists result to Mongo)
  • Easier built-in pagination options
  • Helper to generate JSON schemas from zod validators (to reduce REST API boilerplate)
  • Move validator / type exports to shared package
  • Remove mongoose completely (still being used to manage the connection)
  • More common foreign reference checks (e.g. datasource)
  • Utilities to help with foreign key cascades on delete

Testing

  • Fact metric page and actions within the app
    • List page (with project filtering)
    • Fact metric page
    • Create fact metric
    • Edit fact metric
    • Delete fact metric
    • Refresh experiment results
  • Fact metric REST endpoints
    • List
    • Get
    • Create
    • Update
    • Delete
    • Bulk import
  • No Access read filter behavior
  • Audit log entries
  • Permission checks
  • Custom Validation
  • Migrations
  • User data validation
  • Mongo Indexes
  • dateUpdated / dateCreated
  • New tags registered
  • Ensure valid project id

@bttf
Copy link
Collaborator

bttf commented Feb 1, 2024

High-level but is there a restriction or constraint that requires us to have models defined within req.context? It would be cleaner IMO to have something like req.models. It would also avoid confusion / name-clobbering in certain cases (req.context.organizations vs. req.context.organization)

@jdorn
Copy link
Member Author

jdorn commented Feb 2, 2024

High-level but is there a restriction or constraint that requires us to have models defined within req.context? It would be cleaner IMO to have something like req.models. It would also avoid confusion / name-clobbering in certain cases (req.context.organizations vs. req.context.organization)

Nice thing about it being in context is that we can use it from anywhere, not just within request handlers. I originally put them under req.context.models, but it did get pretty verbose. Open to ideas

@bttf
Copy link
Collaborator

bttf commented Feb 7, 2024

Nice thing about it being in context is that we can use it from anywhere, not just within request handlers.

Yeah, that's pretty important.

I originally put them under req.context.models, but it did get pretty verbose. Open to ideas

I actually don't think that's verbose at all. We will likely destructure the context object like so:

const someBusinessLogicFn = ({ org, models }: ReqContext, exp: ExperimentInterface) => {
  // ...
}

Or

const someBusinessLogicFn = (context: ReqContext, exp: ExperimentInterface) => {
  const { models: { factMetrics }, org } = context;
  const factMetric = await factMetrics.findById(...);
  // ...
}

@jdorn jdorn marked this pull request as ready for review March 5, 2024 02:45
@jdorn jdorn requested a review from bttf March 5, 2024 02:45
@jdorn jdorn changed the title [WIP] Data Model Classes Data Model Classes Mar 5, 2024
Copy link

github-actions bot commented Mar 5, 2024

Your preview environment pr-2092-bttf has been deployed.

Preview environment endpoints are available at:

Copy link
Collaborator

@bttf bttf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking comments / questions

export type BaseSchema = z.ZodObject<
{
id: z.ZodString;
organization: z.ZodString;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we foresee using this with the User model or Organization model?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, those won't work with the current design. Maybe we can make a common base class that does core things like managing indexes and querying, but doesn't do any validation or have any assumptions about data structure.

>;
limit?: number;
skip?: number;
} = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we not want to enforce a limit by default here?

For the queries we use today that don't enforce a limit, each could individually specify limit: undefined

packages/back-end/src/models/BaseModel.ts Outdated Show resolved Hide resolved
packages/back-end/src/services/context.ts Outdated Show resolved Hide resolved
packages/back-end/types/fact-table.d.ts Show resolved Hide resolved
@jdorn jdorn merged commit ab8a708 into main Mar 25, 2024
3 checks passed
@jdorn jdorn deleted the jd/data-model-class branch March 25, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants