indiehd / web-api

GNU Affero General Public License v3.0
6 stars 4 forks source link

Integrity constraint violation while seeding #180

Closed cbj4074 closed 4 years ago

cbj4074 commented 4 years ago

While seeding Order records:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '51-1' for key 'unique_track_per_album'
poppabear8883 commented 4 years ago

AlbumFactory

Line 50:

    for ($i = 1; $i < rand(2, 21); $i++) {
        $s = factory($song->class())->create([
            'album_id' => $album->id,
            'track_number' => $i
        ]);

        $s->album()->associate($album)->save();
    }

The issue is here that we are not constraining a unique track_number between the album_id ... I'm going to handle this fix.

poppabear8883 commented 4 years ago

I lied ...

OrderSeeder

Line 39:

$song = factory(Song::class)->create([
   'album_id' => $album->id,
   'track_number' => 1,
]);
cbj4074 commented 4 years ago

Upon further investigation, it seems that a more comprehensive change is needed with regard to the AlbumFactory.

In essence, the problem here stems from adding Songs to Albums using two different methods in the factory: state() and afterCreating(). The former method is called when make() is used, and the latter when create() is used, which yields some inconsistency that is tripping us up in a few places.

To refresh our collective memories, an Album MUST have at least one Song at all times, from the moment of creation, which is why we added these methods to the AlbumFactory. The problem, however, is that we (and by that, I mean I) have been inconsistent about how we call those factory methods in tests, sometimes using the withSongs state explicitly, and relying on the afterCreating() callback in others.

Ideally, we would change the afterCreating() to afterCreatingState(), which would solve the inconsistency that I describe above. Additionally, we would add afterMaking() and afterMakingState() so that the behavior is completely consistent, whether calling make() or create().

cbj4074 commented 4 years ago

@mblarsen If you're interested in trying to improve our approach here, that would be excellent.

Fundamentally, the challenge is ensuring that an Album always has at least one Song when it's created with the AlbumFactory. Currently, it's necessary to use a factory state explicitly.

On its face, it seems like a simple problem to solve, but when you realize that we create Albums in two different ways, it becomes more complex:

  1. Using the AlbumFactory's create() method, which, of course, persists the records to the DB;
  2. Using the AlbumFactory's make() method, the result of which we pass into the AlbumRepository's create() method.

The challenge is being able to use a single factory in its default condition, without the additional hassle of remembering to use a specific state (withSongs) whenever we're creating through the AlbumRepository.

Ideally, we would be able to specify the number of songs on the Album when we call the factory methods.

Does this make sense?

mblarsen commented 4 years ago

Thanks for the explanation. It does make sense. I will look at it tomorrow.