What is the expected behaviour on duplicated M2M entries

[Drapersniper]

class Track(Table):
    track_id = BigInt(primary_key=True)

class Playlist(Table):
    id = BigInt(primary_key=True)
    tracks = M2M(LazyTableReference("TrackToPlaylist", module_path=__module__))

class Album(Table):
    id = BigInt(primary_key=True)
    tracks = M2M(LazyTableReference("TrackToAlbum", module_path=__module__))

class TrackToPlaylist(Table):
    tracks = ForeignKey(Track)
    playlists= ForeignKey(Playlist)    

class TrackToAlbum(Table):
    tracks = ForeignKey(Track)
    albums= ForeignKey(Album)

# Step 1 -  I expect this to work fine if this is the first time the tracks with id 0-9 are being added to the db
playlist = await Playlist.objects().get_or_create(Playlist.id == 1)
tracks = [Track(track_id=i) for i in range(10)]
await playlist.add_m2m(*tracks, m2m=Playlist.tracks)

# Step 2 - After step 1 do the following
playlist2 = await Playlist.objects().get_or_create(Playlist.id == 2)
tracks2 = [Track(track_id=i) for i in range(5)]
await playlist2 .add_m2m(*tracks2, m2m=Playlist.tracks)

# Step 3
playlist3 = await Playlist.objects().get_or_create(Playlist.id == 3)
tracks3 = [Track(track_id=i) for i in range(5)]
tracks3 +=  [Track(track_id=i) for i in range(5)]
await playlist3 .add_m2m(*tracks3, m2m=Playlist.tracks)

# Step 4 - Run after step 1
album = await Album.objects().get_or_create(Album.id == 1)
tracks4 = [Track(track_id=i) for i in range(5)]
await album .add_m2m(*tracks4, m2m=Album.tracks)

What is expected to happen after steps 2, 3, and 4?

[Drapersniper]

Nvm, after re-reading what I posted I realized it shouldn't be an m2m but o2m relation which is already handled by #599 whenever that is merged in

Edit:
NVM - clearly I need a lot more coffee to get my brain working - it should be M2M given that a track can be in multiple albums/playlists and an album/playlists can have multiple tracks

[Drapersniper]

No rush,

Just to clarify what happens, if I manually define the table entries and it's an entry that already exist in the db it fails due to the unique constraint.

I would expect it to just silently ignore or update (maybe a kwarg to add_m2m?)

Right now the only way to be able to reliably add multiple m2m entries is to query the db for all the tracks with the specified ids before running add_m2m which is very inefficient.

[sinisaos]

@Drapersniper The expected result should be [{'id': 1, 'tracks': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]}, {'id': 2, 'tracks': [0, 1, 2, 3, 4]}, {'id': 3, 'tracks': [0, 1, 2, 3, 4]}, {'id': 1, 'tracks': [0, 1, 2, 3, 4]}] (at least this is the case with the Django ORM that internally handles duplicate values), but unfortunately it is not. @dantownsend I think we have a problem (bug) because we don't handle duplicate values ​​at all. Currently we can only add unique values ​​through add_m2m and that is wrong.

[Drapersniper]

I thought that was the case -

To touch a little on what you said - you can try to add duplicate returns using add_m2m.

The current behaviour is the following

 def add_m2m(*rows):   # This is a simplification of the nested calls 
    unsaved = [i for i in rows if not i._exists_in_db]
    async with rows[0]._meta.db.transaction():
        if unsaved:
            await rows[0].__class__.insert(*unsaved).run()

If you retried the row using .objects().get_or_create() for example it will have the _exists_in_db - another way to get around this is manually filtering the rows and / or adding _exists_in_db = True to the rows.

But ideally, Picollo would handle that for us automatically similar to Django ORM.

The simplest solution would just to blindly slap a ON CONFLICT DO NOTHING or ON CONFLICT DO UPDATE - but I can already think of an issue or two arising for potentially discarding new data or overriding existing data with that.

[sinisaos]

Thanks for the advice. I also used get_or_create to Track and modified your test code (without touching the Piccolo source code)

# Step 1 -  I expect this to work fine if this is the first time the tracks with id 0-9 are being added to the db
playlist = await Playlist.objects().get_or_create(Playlist.id == 1)
tracks = []
for i in range(10):
    t = await Track.objects().get_or_create(Track.track_id == i)
    tracks.append(t)
await playlist.add_m2m(*tracks, m2m=Playlist.tracks)

# Step 2 - After step 1 do the following
playlist2 = await Playlist.objects().get_or_create(Playlist.id == 2)
tracks2 = []
for i in range(5):
    t = await Track.objects().get_or_create(Track.track_id == i)
    tracks2.append(t)
await playlist2.add_m2m(*tracks2, m2m=Playlist.tracks)

# Step 3
playlist3 = await Playlist.objects().get_or_create(Playlist.id == 3)
tracks3 = []
for i in range(5):
    t = await Track.objects().get_or_create(Track.track_id == i)
    tracks3.append(t)
tracks3 += tracks3
result = sorted(set(tracks3), key=lambda x: x.track_id)
await playlist3.add_m2m(*result, m2m=Playlist.tracks)

# Step 4 - Run after step 1
album = await Album.objects().get_or_create(Album.id == 1)
tracks4 = []
for i in range(5):
    t = await Track.objects().get_or_create(Track.track_id == i)
    tracks4.append(t)
await album.add_m2m(*tracks4, m2m=Album.tracks)

print(
    await Playlist.select(
        Playlist.id, Playlist.tracks(Track.track_id, as_list=True)
    ),
    await Album.select(
        Album.id, Album.tracks(Track.track_id, as_list=True)
    ),
)

and result is [{'id': 1, 'tracks': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]}, {'id': 2, 'tracks': [0, 1, 2, 3, 4]}, {'id': 3, 'tracks': [0, 1, 2, 3, 4]}] [{'id': 1, 'tracks': [0, 1, 2, 3, 4]}] which is much better.

[Drapersniper]

Thanks for the advice. I also used get_or_create to Track and modified your test code (without touching the Piccolo source code)

Yup, this is how I gotten the current issue by calling get_or_create - the issue is Playlists can have thousands of tracks, (in the example, I used five as a simple to understand example, but the real use case is hundreds/thousands) - however I often have the Track metadata beforehand (which is why I'm able to initialize the rows manually)

The issue is that this is highly inefficient (Making a selection and/or an insert) for each value.

The issue I mentioned arises from manually creating tracks i.e [Track(track_id=i) for I in range(5)] (If you already have the data here you don't need to select/create then, as add_m2m already tries to create objects that don't exist in a bulk insert which is far more efficient than individual insert beforehand) p.s manually initialized rows always have _exists_in_db set to False, only by querying it this would be set to True, which is what is used by add_m2m in determining if a value needs to be inserted.

I would expect to handle this case rather than error if a manually initialized row already exists in the DB.

The ideal behaviour here would be a kwarg to perform an upsert/On conflict do nothing rather than forcing an insert.