🚧 Proof of concept for arbitrary media groupings

[cparadis777]

An attemp to implement arbitrary groupings for media records. Related to #725.

Currently, allows to group by media field and mediaMetadata field.
This is a very rough PoC, everything can be changed.
I aimed to make it as transparent to other features and the existing codebase.
Example query to get media grouped by writer, then publisher.

query {
  groupedMedia(
    groupBy: {
      levels: [
        { mediaMetadata: { field: WRITERS } },
        { mediaMetadata: { field: PUBLISHER } }
      ]
    }
  ) {
    ... on SmartListGrouped {
      items {
        entity { ... on GenericGroupingValue { key } }
        subgroups { 
          entity { ... on GenericGroupingValue { key } }
          books { 
            id
            name
            pages
          }
        }
      }
    }
  }
}

[aaronleopold]

Alright I finally had a moment to take a look! I had a few comments, but I wasn't able to finish reviewing everything (I had to stop at the macro). I'll try to take another look sometime during the week 🙂

[aaronleopold]

I think I understand why we are re-using this object for this purpose, however semantically I don't love it. If I were to make a grouped query totally detached from smart lists, it feels strange to have to declare an inline fragment named for smart lists (i.e., SmartListGrouped).

We don't need total duplication, so perhaps renaming SmartListGrouped to something generic? That way it doesn't feel as awkward, maybe?

[cparadis777]

I was undecided between creating a completely new query or trying to add groupings to the existing ones in a transparent manner.
Smart lists are still a bit nebulous to me, but I understood them as a generic grouping of media that was being reused across the code base.
I'm open to anything you feel is more idiomatic and fits better in the overall app!

[aaronleopold]

Smart lists are still a bit nebulous to me, but I understood them as a generic grouping of media that was being reused across the code base

I don't think they were being reused, but if they were that should be cleaned up. Smart lists are a feature where you can configure more complex filters and save them as a "smart list" which effectively just takes that saved filter and returns the matching books (ungrouped, or grouped by library/series).

[cparadis777]

Then it makes way more sense to build something standalone. But this might be going into a more architectural direction about where and how we want to use groups. I'm trying to figure a way to not create a whole bunch of boilerplate code for each model and query. I'll have to admit I've barely worked with GraphQL, so I'm out of my depths on this side.

[aaronleopold]

But this might be going into a more architectural direction about where and how we want to use groups. I'm trying to figure a way to not create a whole bunch of boilerplate code for each model and query

Yeah that's totally fair! In my head it could be as simple as just generalizing the structs/enums (e.g., renaming to GroupedBooks or something) and using that in smart lists in addition to here.

[aaronleopold]

This one took me a second to conceptualize, but I think I understand why it was done this way. I can't think of an alternative that would definitely be better, but I'd like to consider it a bit more.

The part I'm not 100% on is that implicit bit where books will only be populated at the last level, I'd rather lean into unioned types to make it more explicit. I could be over thinking it too, though.

[cparadis777]

I guess it depends on how we want to handle books that are missing the attribute we're grouping on?
We'd either create an empty group and put them there, ensuring all books in the query are grouped on the same terminal level or we leave them higher up the grouping chain?

[aaronleopold]

Ohhh yeah great point, I missed that in my initial review. Supporting multiple levels of grouping does also complicate how to handle it. I don't have an immediate answer, unfortunately. I think as a POC this is okay as-is, I just think implicit things are harder to grasp at a glance sometimes.

A related thought that has been bubbling though since looking at this PR: when I eventually expose any kind of unfiltered, grouped functionality on the client we need pagination. Smart lists kinda get away with it (at least temporarily, eventually I have plans to "fix" it) because of the somewhat explicit requirement of filtering. But an unfiltered set of books could produce way too much, there are folks with 100k+ files in their server. Again, it is a really hard problem to solve though since we are mixing SQL and programmatic grouping

[cparadis777]

Pagination, that completely slipped my mind too. Again, I don't think it's in scope for the PR, but clearly a problem that will need solving.
It also brings up the question of performance. I'm doing everything in memory, but I wonder if trying to build DB queries and building the groups directly instead of propagation down the groupings tree would be possible. It would probably be more performant, but maybe need some prepared queries or constructing raw SQL instead of using the ORM.

[aaronleopold]

Again, I don't think it's in scope for the PR, but clearly a problem that will need solving

Yeah, I agree. I understand this is a POC

It also brings up the question of performance. I'm doing everything in memory, but I wonder if trying to build DB queries and building the groups directly instead of propagation down the groupings tree would be possible

I think it should be possible, yeah. The other bit wrt pagination of grouped items is that to have any decent client would mean also collecting the group headers and associated counts ahead of time. Perhaps for a first iteration it might be acceptable to cap the grouping to one level at first until performance is improved.

I honestly don't get much feedback in general for Stump, the recent visibility bump from the Booklore debacle excluded since I expect it to die down a bit, and so this is not something I've personally prioritized solving yet. You're welcome to continue down this route if it is an area of interest, that's what I generally encourage folks to do, but I think once this POC is more generalized away from some of the smart list bits a better immediate use of time could be improving the ordering capabilities which would fix things like this #482 (comment)

[aaronleopold]

I unfortunately won't be able to get to this one today, but I read the doc string and I think the approach outlined there so far is sound.