feat: implement pagination behavior for grouped models

[lu-pl]

Closes #114.

[b1rger]

That seems a bit overengineered to me ;)

Why even bother with a query constructor? You could combine the get_items_query_constructor and the construct_ methods into one:

get_query(model, query, limit, offset):
   if group_by_value := getattr(model, "model_config", {}).get("group_by", None):
      return construct_grouped_pagination_query(query, group_by_value, limit, offset)
   return construct_ungrouped_pagination_query(quer, limit, offset)

[lu-pl]

That seems a bit overengineered to me ;)

Why even bother with a query constructor? You could combine the get_items_query_constructor and the construct_ methods into one:

get_query(model, query, limit, offset):
   if group_by_value := getattr(model, "model_config", {}).get("group_by", None):
      return construct_grouped_pagination_query(query, group_by_value, limit, offset)
   return construct_ungrouped_pagination_query(quer, limit, offset)

I deliberately implemented this using match case because this allows pattern matching on model_config and to trigger different query constructor code paths depending on the model config. See the commit message.

It is very likely that additional constructors will be needed in the future (e.g. for filtering and ordering etc), so I feel like this should be pluggable.

I am aware that query construction is kind of untidy at the moment, this is why I will implement #134 as soon as these changes are merged.

[b1rger]

I deliberately implemented this using match case because this allows pattern matching on model_config and to trigger different query constructor code paths depending on the model config. See the commit message.

The commit message starts with: "The get_items_query_constructor should be implemented using match/case;" - I'm not sure what this means The get_items_query_constructor is implemented using match/case. Maybe it should be rather be saying "The get_items_query_constructor is implemented using match/case, because this alllows ...."

It is very likely that additional constructors will be needed in the future (e.g. for filtering and ordering etc), so I feel like this should be pluggable.

We can easily make this pluggable once this is actually needed. We don't have to make the code overly complex so we can cater to every possible scenario that might come up in the future.

[lu-pl]

The commit message starts with: "The get_items_query_constructor should be implemented using match/case;" - I'm not sure what this means The get_items_query_constructor is implemented using match/case.

True, this is misleading, will change the commit message.

We can easily make this pluggable once this is actually needed. We don't have to make the code overly complex so we can cater to every possible scenario that might come up in the future.

I essentially agree, still I am much in favor of using the suggested match/case version because I feel like this is a rather elegant way to implement the logic and it is also an idiomatic use case for Structural Pattern Matching.

The alternative to using the partial pattern matching mechanism on the model.model_config dict is to either use KeyError exception handling or dict.get lookups - i.e. explicit checking for entry existence.

The match/case solution allows for an exact implementation of the intended behavior in a very concise way. The fact that this is easily extensible too, is more of an additional benefit.

[b1rger]

The match/case solution allows for an exact implementation of the intended behavior in a very concise way

The dict.get lookup does this too, just with less lines. But this is not only about the match/case statement. What you basically want is to modify a string. Instead of just doing that, you implement a method that returns a constructor that has to implement a protocol. Do you see how that is overengineering?
Throw away the ItemsQueryConstructor, throw away the get_items_query_constructor and instead implement what you actually need - 3 lines of code directly in SPARQLModelAdapter.query

(Another example is the construct_ungrouped_pagination_query that basically mimics what f-strings do, just instead of using f-strings it uses a template, and in this commit you're also replacing that template with a method that runs substitute on that template for you)

[lu-pl]

The match/case solution allows for an exact implementation of the intended behavior in a very concise way

The dict.get lookup does this too, just with less lines. But this is not only about the match/case statement. What you basically want is to modify a string. Instead of just doing that, you implement a method that returns a constructor that has to implement a protocol. Do you see how that is overengineering? Throw away the ItemsQueryConstructor, throw away the get_items_query_constructor and instead implement what you actually need - 3 lines of code directly in SPARQLModelAdapter.query

This might be (a mild case of) overengineering now.
But the proposed solution is the base for a functional + pluggable design I know will be needed soon.

I would agree on implementing get_items_query_constructor with dict.get (for now), but the general functional + pluggable design deems me a good way of clearly defining responsibilities in the process of query construction. Also, the ItemsQueryConstructor protocol is not only a way of communicating intent to developers (including myself) but also important for static type checking.

Generally, I see your point and you are correct. But if I completely tear this down now, I will have to re-implement everything for issue #134 again. I have a pretty clear picture now of how the query construction logic should be organized. So the current PR is more of a documented transition to the (urgently needed) #134 refactoring.

(Another example is the construct_ungrouped_pagination_query that basically mimics what f-strings do, just instead of using f-strings it uses a template, and in this commit you're also replacing that template with a method that runs substitute on that template for you)

I totally agree, this is a remnant from an earlier implementation. I still want construct_ungrouped_pagination_query to be a callable though; as a top-level entity it is easier to communicate its intent and responsibility, it can have a docstring and it can be used in the constructor factory dispatcher (get_items_query_constructor).

[lu-pl]

Ok, here is another version: I encapsulated the ItemsQueryConstructor dispatch in construct_items_query; this significantly tidies up the query retrieval in SPARQLModelAdapter:

class SPARQLModelAdapter(Generic[_TModelInstance]):
    # ...
    def query(
        self,
        *,
        page: int = 1,
        size: int = 100,
    ) -> Page[_TModelInstance]:
        """Run a query against an endpoint and return a Page model object."""
        count_query: str = construct_count_query(query=self._query, model=self._model)
        items_query: str = construct_items_query(
            query=self._query,
            model=self._model,
            limit=size,
            offset=calculate_offset(page, size),
        )
       # ...

This might still be considered overengineered or at least "eagerly modular", but as I said, I consider this PR more as a documented transition towards an rdfproxy.constructor.QueryConstructor class that will handle all query construction requirements in a concise and pluggable way.