Browseable and Children features from 1.0.0-beta5 #336

moradology · 2022-01-25T00:11:42Z

Related Issue(s): #311 #312

Description:
This PR adds a workaround which is intended to allow children and browsable conformance classes to be enabled in stac-fastapi while deeper changes for a more robust implementation are being developed in pgstac. For a sense of what specifying browsable hierarchies looks like, pay special attention to hierarchy.json and hierarchy.py

Additionally, clients have been broken out of the hundreds-of-lines file they used to live in and organized into their own package.

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.

moradology · 2022-01-27T13:24:49Z

With the most recent commit (191b75f), each catalog endpoint is, itself, a STAC API. Reviewers ought to keep an eye out for code that can be deduplicated without losing important context (e.g. a try/catch block which issues a 404 on exception is quite likely more useful duplicated as it doesn't hide relevant information for understanding an endpoint's behavior)

geospatial-jeff · 2022-04-24T21:12:50Z

stac_fastapi/api/stac_fastapi/api/app.py

@@ -83,9 +86,15 @@ class StacApi:
    api_version: str = attr.ib(default="0.1")
    stac_version: str = attr.ib(default=STAC_VERSION)
    description: str = attr.ib(default="stac-fastapi")
+    search_get_request_base_model: Type[BaseSearchGetRequest] = attr.ib(


Whats the difference between this and search_get_request_model? Same comment for post models.

Yeah, this is a potentially ugly and maybe a point where you could provide some counsel. Basically, the need to supply a catalog URI parameter changes the constructed search model so what I'm doing here is passing in the base search_get_request_base_model because it might possibly differ from the fully constructed search_get_request_model used by standard searches. Perhaps the answer is to just construct both of these inside of core.py. Let me know what you think.

geospatial-jeff · 2022-04-24T21:42:51Z

stac_fastapi/types/stac_fastapi/types/hierarchy.py

+        if "collection_id" in child
+    ]
+    children_links = catalog_links + collection_links
+    item_links = [


Is it necessary to link to items from the GET /children endpoint? The spec says:

The /children endpoint must return the all the Catalog and Collection objects referenced by these child link relations.

And doesn't reference items at all.

Oh nevermind this is a requirement of browseable

geospatial-jeff · 2022-04-24T21:51:28Z

stac_fastapi/api/stac_fastapi/api/app.py

@@ -319,6 +479,16 @@ def register_core(self):
        self.register_get_collection()
        self.register_get_item_collection()

+        # Browseable endpoints
+        self.register_catalog_conformance_classes()


I'd like to hide these behind some sort of flag (disabled by default) for two reasons:

These two extensions are really new, and we are the first implementation of it.

They increase the surface area of the API quite a bit, and may not be required by most use cases.

geospatial-jeff · 2022-04-24T21:53:57Z

stac_fastapi/pgstac/stac_fastapi/pgstac/core.py

@@ -103,6 +109,53 @@ async def get_collection(self, collection_id: str, **kwargs) -> Collection:

        return Collection(**collection)

+    async def get_root_children(self, **kwargs) -> Children:


Just a note to self that the duplication between async/sync clients is starting to get a little much and we may want to think of a way to fix that in the future.

Agreed. In this PR it already got to the point that organization in a single file was getting absurd.

geospatial-jeff · 2022-04-24T21:54:52Z

stac_fastapi/sqlalchemy/stac_fastapi/sqlalchemy/app.py

@@ -29,14 +32,20 @@
    TokenPaginationExtension(),
    ContextExtension(),
 ]
+with open(settings.browseable_hierarchy_definition, "r") as definition_file:


Please do the hierarchy parsing behind a function so its not happening in global scope (to avoid side effects).

geospatial-jeff · 2022-04-24T21:59:19Z

stac_fastapi/testdata/joplin/hierarchy.json

@@ -0,0 +1,35 @@
+{


I'm concerned with the scaleability of encoding larger hierarchies into a JSON file. I'm also concerned that this approach requires knowing item IDs ahead of time (this JSON file is read before the API starts up) which presents some challenges for an API that is adding/deleting items through transaction endpoints. Ideally item browseability could be done at runtime.

This is a hard problem, and I don't have a better solution for it at present; so happy to go forward with this approach and iterate/improve as needed!

Your scalability concern is reasonable. I'm having a hard time imagining it being a performance bottleneck, but JSON is brittle and it is (as currently imagined) directly maintained by developers (often a recipe for trouble). I'm not 100% sure what the best approach is to avoid such a difficulty. Perhaps rules for generating trees rather than fully specified trees? I'm open to suggestions.

Runtime addition of nodes should be possible with a small set of modifications. I agree that's important to add and am also in support of adding it in future updates

geospatial-jeff · 2022-04-24T22:00:20Z

stac_fastapi/types/stac_fastapi/types/clients/async_core.py

+        request_path = kwargs["request"]["path"]
+        split_path = request_path.split("/")[2:-1]
+        remaining_hierarchy = self.hierarchy_definition.copy()
+        # PGSTAC TODO: Add optional 'catalog' parameter to search, which will determine which


are any of these PGSTAC TODO laying around the PR important to resolve before we merge?

These were initially added to provide some pointers to @bitner about what kinds of functionality could move back into pgstac, but I'm happy to get rid of them!

geospatial-jeff · 2022-04-24T22:00:55Z

stac_fastapi/types/stac_fastapi/types/clients/async_core.py

+            if "collection_id" in node
+        ]
+        search_request.collections = child_collections
+        return await self.post_search(search_request, **kwargs)


I'm glad this is just a wrapper on top of the existing post_search, super cool!

geospatial-jeff · 2022-04-24T22:04:02Z

stac_fastapi/types/stac_fastapi/types/hierarchy.py

+
+
+# PGSTAC TODO: function to allow ingest of hierarchy
+def parse_hierarchy(d: dict) -> BrowseableNode:


Return type should be a Union of all three nodes, but this function is only ever called on a dictionary that would return a BrowseableNode. So I'm not sure if the return type is wrong or the CollectionNode and CatalogNode returns aren't needed.

It is very possible that I'm missing something peculiar about python types or TypedDict inheritance, but I think we can avoid the Union here because of the inheritance structure - both CollectionNode and CatalogNode are subsumed under the BrowseableNode type. Let me know if I'm off base in this assertion

gadomski · 2022-12-30T17:08:27Z

@moradology is this workaround still necessary? I'm doing a little housekeeping and don't want to try and resolve these merge conflicts if its not needed.

moradology · 2023-01-10T19:18:29Z

Hey @gadomski, I don't know that the children/browseable features are 100% necessary so it is probably not absolutely necessary that this workaround be included in the project. I'll close the PR for now but keep the branch around in case people want to refer back for a future implementation

moradology force-pushed the feature/children-1.0.0-beta.5 branch 2 times, most recently from 658c75d to d9570a0 Compare January 25, 2022 00:23

moradology mentioned this pull request Jan 31, 2022

Hierarchically organized STAC APIs stac-utils/pgstac#88

Open

moradology force-pushed the feature/children-1.0.0-beta.5 branch from e263448 to 3fbbac9 Compare April 14, 2022 17:50

moradology added 16 commits April 19, 2022 08:45

Init work towards children endpoint

f939811

Implement alternative backing for browsable features

8a1339c

Update links and support /children

122ac4b

Add example of nested catalog

546b6f0

Remove printlns

ab06b8b

Simplify url construction

e986169

Add conformance classes

932dc4d

Spell browseable correctly

d943c90

Add full page for catalogs to support API functionality

f4c5620

Add support for sub-catalog behaviors

dadce7d

Break clients out for more organization and smaller files

c1f6b3c

Add todos for pgstac

316661c

Add /catalogs/{catalog}/children endpoint

391a732

Bump conformance classes to rc1

7c049f5

Fix import after rebase

dd13bff

Set mimetype on search link to geojson

8baf324

moradology force-pushed the feature/children-1.0.0-beta.5 branch from 3fbbac9 to 8baf324 Compare April 19, 2022 14:27

moradology requested review from geospatial-jeff and philvarner and removed request for geospatial-jeff April 19, 2022 14:27

moradology marked this pull request as ready for review April 19, 2022 14:27

moradology added 2 commits April 19, 2022 09:53

Remove duplicate introduced via rebase

591fc27

Bump version to rc1

b908269

geospatial-jeff requested changes Apr 24, 2022

View reviewed changes

Respond to review

242e1fe

moradology requested a review from geospatial-jeff April 25, 2022 22:27

moradology added 2 commits May 2, 2022 13:16

Avoid error when no hierarchy file is specified

34af872

Use new relations from stac-pydantic 2.0.3

42f5e28

moradology changed the title ~~Browsable and Children features from 1.0.0-beta5~~ Browseable and Children features from 1.0.0-beta5 May 5, 2022

moradology closed this Jan 10, 2023

		@@ -103,6 +109,53 @@ async def get_collection(self, collection_id: str, **kwargs) -> Collection:

		return Collection(**collection)

		async def get_root_children(self, **kwargs) -> Children:



		# PGSTAC TODO: function to allow ingest of hierarchy
		def parse_hierarchy(d: dict) -> BrowseableNode:

Browseable and Children features from 1.0.0-beta5 #336

Browseable and Children features from 1.0.0-beta5 #336

Uh oh!

Conversation

moradology commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moradology commented Jan 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gadomski commented Dec 30, 2022

Uh oh!

moradology commented Jan 10, 2023

Uh oh!

Uh oh!

moradology commented Jan 25, 2022 •

edited

Loading