Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable context extension by default #211

Merged
merged 8 commits into from
Dec 7, 2021

Conversation

jaysnm
Copy link
Contributor

@jaysnm jaysnm commented Aug 7, 2021

A work around issue #207

ContextExtension for pgstac need be implemented, not working at the moment.

@robintw
Copy link
Contributor

robintw commented Oct 20, 2021

I was interested in using the ContextExtension with the pgstac backend, so I tried it and it 'just worked'. So presumably someone has implemented the ContextExtension for pgstac.

Therefore, can we go ahead with adding this as a default extension? I think it would help new users - I was wondering why various operations I was trying with other tools were failing, and it was because they needed the context extension enabled.

@jaysnm jaysnm marked this pull request as ready for review October 21, 2021 11:46
@jaysnm
Copy link
Contributor Author

jaysnm commented Oct 21, 2021

Thanks @robintw for the observation. For sure version 2.2.0 has implemented ContextExtension support on pgstac backend, thanks to @lossyrob and team. I have cleaned up my fork and made this PR ready for review.Can't wait to see the extension enabled by default!

@rsmith013
Copy link

I would think that the extensions shouldn't be enabled by default. I would hope that stac-fastapi becomes a framework that provides all the tools to accomplish what you need. I would think that it is down to the implementer to enable the context extension (and any other extension) that will be required by their community of users and for clients to inspect the conformance classes to know whether a particular feature is enabled or not.

It think the pgstac and sqlalchemy backends provide an implementation of the stac-fastapi framework. Perhaps the enabling of extensions could become a configurable option?

@lossyrob
Copy link
Member

The extensions added here are only for the example apps included in stac-fastapi. From what I can tell, normal usage would include a user creating their own instance of StacApi and configuring it with whatever extensions they would like (and therefore configurable in the way @rsmith013 requests). In this way, the addition of the ContextExtension here to the app.py examples in each of the backends that support it give the reader a better way to discover that extension.

@robintw how are you using stac-fastapi? Are you importing the app from one of the backend apps directly to get a FastAPI application? If so, you would have to rely on default configuration in these app.py's to change in order to change your configuration, though it seems like constructing your own instance of StacApi with all the configuration you need for your own instance of the server would give more flexibility.

@robintw
Copy link
Contributor

robintw commented Nov 10, 2021

I originally started by importing app from one of the original configurations, but have since moved to constructing my own instance.

You're right that this would improve discoverability: I only found out about the Context Extension by finding that various other tools failed to interact with my STAC server properly. I did some Googling and found that they needed the Context Extension, and then a bit more searching showed me that it was implemented in stac-fastapi, but just not switched on by default. I agree that having it in the example apps would be useful.

(I might see if I can submit a PR with a bit of an update to the documentation emphasising that the defined app instances are just examples, and the best way to configure this for 'real-world' use is to define your own instance of StacApi with the configuration you want.)

@lossyrob
Copy link
Member

Thanks, that make sense - @jaysnm are you using the app in the same way as Robin was previous to creating an application-specific StacApi?

@jaysnm
Copy link
Contributor Author

jaysnm commented Nov 11, 2021

Thanks for this conversation. Based on comments here, it turns out I have been running STAC back-end(s) the wrong way. In my team, we discourage building custom layer on top of upstream projects unless it's justifiably necessary (keeping track of updates and breaking changes adds to maintenance overheads). In practice thus, I directly use app from back-end to serve my STAC content. I will try this new approach of extending app to write my own custom StacAPI back-end. Probably this will also close issue #223. Yes I agree with @robintw, some of these examples can be pinned somewhere in the documentation. Once again, thank you all!

@jaysnm
Copy link
Contributor Author

jaysnm commented Dec 1, 2021

The extensions added here are only for the example apps included in stac-fastapi. From what I can tell, normal usage would include a user creating their own instance of StacApi and configuring it with whatever extensions they would like

This approach worked pretty fine for us and by so doing we were able to support other functionaries that were ambiguous previously. Our implementation was pretty simple!


"""STAC FastAPI application.
main.py
"""

from stac_fastapi.api.app import StacApi
from stac_fastapi.extensions.core import (
    FieldsExtension,
    QueryExtension,
    SortExtension,
    TransactionExtension,
    ContextExtension,
)
from stac_fastapi.extensions.third_party import BulkTransactionExtension
from stac_fastapi.sqlalchemy.config import SqlalchemySettings
from stac_fastapi.sqlalchemy.core import CoreCrudClient
from stac_fastapi.sqlalchemy.session import Session
from stac_fastapi.sqlalchemy.transactions import (
    BulkTransactionsClient,
    TransactionsClient,
)
from stac_fastapi.sqlalchemy.types.search import SQLAlchemySTACSearch

settings = SqlalchemySettings()
session = Session.create_from_settings(settings)
api = StacApi(
    settings=settings,
    extensions=[
        TransactionExtension(
            client=TransactionsClient(session=session), settings=settings
        ),
        BulkTransactionExtension(client=BulkTransactionsClient(session=session)),
        FieldsExtension(),
        QueryExtension(),
        SortExtension(),
        ContextExtension(),
    ],
    client=CoreCrudClient(session=session),
    search_request_model=SQLAlchemySTACSearch,
)

app = api.app

# add other stuff into app
# run the app using your favorite asgi server

In this way, the addition of the ContextExtension here to the app.py examples in each of the backends that support it give the reader a better way to discover that extension.

Upstream repository is as good without this PR patches. Going forward, this PR can be closed. I'm happy to contribute into docs with an example app implemented from StacApi as a followup

@moradology
Copy link
Collaborator

@jaysnm This needs a quick rebase and then I'd be happy to get it merged. Let me know if you can't find the time; I should be able to take care of things if that's helpful

@jaysnm
Copy link
Contributor Author

jaysnm commented Dec 7, 2021

Hi @moradology. This is done and change-log updated. Let me know if any other change is need.

@moradology
Copy link
Collaborator

Awesome. Thanks for the contribution!

@moradology moradology merged commit 76e8d64 into stac-utils:master Dec 7, 2021
@jaysnm
Copy link
Contributor Author

jaysnm commented Dec 7, 2021

My pleasure.Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants