Improve separation of concerns between core and backend libraries

[duckontheweb]

Related Issue(s):

• #432(comment)

Description:

Demonstrates one approach for creating a clearer separation of concerns between the core stac-fastapi package and the backend packages like stac-fastapi.pgstac. This draft targets the landing page and collections endpoints to demonstrate the approach; if we decide to go this route, we should apply this to all other route handlers as well.

The general approach is as follows:

• Any methods on the *BaseCoreClient classes that are used directly as route handlers have been renamed with a handle_ prefix. These will be concrete methods defined in this library responsible for parsing the request, calling the appropriate backend method(s), and constructing the response.
• New abstract methods have been created that must be implemented in the backend packages. These methods are all prefixed with fetch_ and are responsible for fetching the appropriate objects from the database or other backend. These methods have well-defined, typed inputs rather than accepting arbitrary **kwargs.

This approach should make it easier to handle all of the logic around constructing links, conformance classes, and other API-layer concerns in this package, ensuring that responses are consistent across the different backends.

Some future TODOs:

• Having more consistent type annotations and type checking in this package would make it easier to ensure that backend applications are implementing methods correctly. This seems out of scope for this PR, but I can start implementing this once this and Remove backend packages #450 have been resolved.
• We may want to go one step further and have the fetch_ and handle_ methods in separate classes or mixins. My preference for this draft was to focus on the functional changes and leave that kind of structural refactor to a later PR, but I can always experiment with changing the class structure if we decide we want that right away.

I will open a PR in the stac-fastapi-pgstac repo to demonstrate the corresponding changes in that backend.

EDIT: Note that this PR is against the remove/backend-packages branch instead of master to simplify the changes. Once #450 has been merged we can update this to use master as the base.

PR Checklist:

• Code is formatted and linted (run pre-commit run --all-files)
• Tests pass (run make test)
• Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
• Changes are added to the CHANGELOG.

[duckontheweb]

This change is again not directly related to this refactor, but helped speed up the Docker builds

[geospatial-jeff]

I like the approach of separating link injection from backend specific logic, my main concern here is breaking the client interfaces and adding extra boilerplate to the clients. I was thinking that instead of adding more methods we could handle this with a function in the API layer that is called in the FastAPI endpoint after each client method (ex. client.get_collection). The function signature could look something like:

import typing

from starlette.requests import Request
from stac_fastapi.types.stac import Collection, Collections, Item, ItemCollection

StacTypes = typing.Union[Collection, Collections, Item, ItemCollection]

def hydrate_links(request: Request, data: StacTypes) -> StacTypes:
    ...

The request object has the URL of the request which lets us infer parent/child/root links. And we could infer the type of data using something equivalent to pystac.serialization.identity.identify_stac_object_type.

The route handler(s) would then look something like this:

def _endpoint(
    request: Request,
    request_data: request_model
):
    # Request data from the backend.
    # Note that `Request` is no longer passed to client method.
    backend_response = func(request_data)

    # Hydrate links
    response_with_links = hydrate_links(request, backend_response)

    # Serialize response
    return _wrap_response(response_with_links, response_class)

A few advantages of this approach are:

• Passing the starlette Request object to he backend clients is a leaky abstraction, and I think this entirely removes the need to do so because the Request object never leaves the route handler.
• Let's us deal with resolving links in one place, for example accounting for router prefixes (https://github.com/stac-utils/stac-fastapi/blob/master/stac_fastapi/types/stac_fastapi/types/requests.py#L6-L14), handling of FastAPI's root path parameter, or running behind a proxy.
• I'm sure we could write this in a way that makes it easy for users to inject their own custom links into API responses, ref Add a way for users to inject their own links into API responses #191.

We could probably also use FastAPI dependency injection here to simplify the code and make it easier to override, ref #402. For example the dependency could be a generic function/factory which returns the correct hydration function based on which endpoint is being called (hydrate_item, hydrate_collection etc.).

[geospatial-jeff]

Circling back to this after thinking for a while, I think the most important thing for me is to remove the need to inject the starlette Request object into the backend clients as that is a clear as that violates separation of concerns. The backend clients should just take request parameters for the given endpoint and fetch data that meets those requirements, and shouldn't be concerned with things like the base url of the request.

[duckontheweb]

Thanks @geospatial-jeff, all of your points are right on. I'll update this PR to try to take more of this approach.

my main concern here is breaking the client interfaces and adding extra boilerplate to the clients.

Yeah, I agree. I think the approach you outlined above would still break the client interfaces since they rely on the request being passed as a keyword arg. However, it minimizes the changes to the overall interface, and puts the logic for handling links in once place, which I like.

[gadomski]

Closing as OBE #555.