thoughtbot / fishery

A library for setting up JavaScript objects as test data
MIT License
877 stars 36 forks source link

Uniqueness conflicts with auto-incrementing IDs in Postgres #56

Closed rofreg closed 3 years ago

rofreg commented 3 years ago

I'm trying to use Fishery in a project that also uses Prisma, an ORM that follows the data mapper pattern. I've defined a user factory as follows:

import { Factory } from "fishery";
import { User } from "@prisma/client";
import prisma from "../client";

export default Factory.define<User>(({ sequence, onCreate }) => {
  onCreate(user => {
    return prisma.user.create({ data: user });
  });

  return {
    id: sequence,
    ...
  };
});

The id is a required property of a User, so it must be specified here. However, id is also an auto-incrementing column in Postgres – and when you manually specify an ID when inserting a row, Postgres does NOT automatically update the auto-incrementation value. As a result, the following can happen:

Possible solutions

Ideally, we could omit id: sequence from the user factory. However, in this case the Prisma User type requires a valid value for id. (Prisma actually doesn't have the concept of an "unsaved User object", as far as I can tell – instead it uses a separate type of UserCreateInput when referring to a collection of attributes that could be inserted into the database and turned into a full-fledged User.)

One possible solution might be to update Fishery such that it allows build to return a UserCreateInput object, while allowing create to return a different type of User/Promise<User>. (Maybe this is possible already? But if so, I couldn't figure out how to do it from the documentation.) This would make Fishery compatible with ORMs that use different types for saved and unsaved objects.

Alternately, there might be a different solution that's much more appropriate! I'm definitely guessing a bit here – I'm primarily a Ruby developer, and not nearly as familiar with modern JS. If there are other suggested approaches for how to handle this issue, please let me know!

casey-chow commented 3 years ago

One stopgap solution would be to create a factory of User | UserCreateInput so that it still typechecks on creation. Alternatively (and this is the ugly solution imo) you can also shove a sentinel value (like -1) into the id field that is removed before calling Prisma in your onCreate hook.

stevehanson commented 3 years ago

Thanks for reporting this @rofreg and for your comment, @casey-chow!

I just started a Prisma project to understand the problem better and am experimenting with solutions. Would it solve your problem if we were to add a new third template argument so the return type of create is customizable? Something like:

type User = {
  name: string;
};

type SavedUser = User & {
  id: number;
  createdAt: Date;
};

// template arg 1 (currently exists) = The type that is returned from build()
// template arg 2 (currently exists) = The type of any transient params that your factory takes (defaults to 'any')
// template arg 3 (new) = The type that is returned from create()
const userFactory = Factory.define<User, any, SavedUser>(({ onCreate }) => {
  onCreate((user) => prisma.user.create(user));
  return {
    name: "Stephen",
  };
});

const user = userFactory.create();
user.id // works

I've started putting together some work on this and think it will likely be feasible.

t3hpr1m3 commented 3 years ago

I'm currently working around this problem by using the supplied sequence to derive my id's, and in some cases, ignoring the derived id altogether on save. Basically, I have a helper method:

const TEST_ID_OFFSET = 10000;
const testId = (id: number): number => (
  id + TEST_ID_OFFSET
);

This gives me an id that leaves room for more than enough auto-incrementing records to be added to the database before collision. I plan on working on a PR that would allow "seeding" the initial sequence value.

Ignoring the sequence id on save is fairly straightforward:

const userFactory = Factory.define<User>(({ sequence, onCreate }) => {
  onCreate((user): Promise<User> => {
    const { id, ...userProps } = user;
    return userRepo.save(userProps);
  });

  return {
    id: testId(sequence),
    email: `user${sequence}@example.com`
  };
});

It isn't elegant, but we're able to avoid collisions.

casey-chow commented 3 years ago

I've started putting together some work on this and think it will likely be feasible.

@stevehanson I see there's an in-progress branch, did you encounter any blockers working on this? I'm migrating my codebase to Prisma at the moment, starting with the factories, so I think I could justify putting some time into this feature.

stevehanson commented 3 years ago

@rofreg @casey-chow @t3hpr1m3 : I've finally opened a PR, #74, with the functionality I proposed (a different return type for create). Take a look if you get a chance!

casey-chow commented 3 years ago

@rofreg @casey-chow @t3hpr1m3 : I've finally opened a PR, #74, with the functionality I proposed (a different return type for create). Take a look if you get a chance!

Looks great! I've been using the old branch (merged with the current tip for main) and it's been working just great for me.