Infinite recursion with `get_all_items`

[jisantuc]

When reading s3://rasterfoundry-development-data-us-east-1/berlin-catalog/catalog.json, initially, the reported links are correct:

> from pystac import Catalog
> catalog = Catalog.from_file("s3://rasterfoundry-development-data-us-east-1/berlin-catalog/catalog.json")
> catalog.links

[<Link rel=child target=./collection.json>,
 <Link rel=self target=s3://rasterfoundry-development-data-us-east-1/berlin-catalog/catalog.json>,
 <Link rel=root target=<Catalog id=berlin>>]

However, iterating over get_all_items throws a RecursionError: maximum recursion depth exceeded in comparison, and inspecting the links after the fact reveals that the catalog has picked up a child link to itself:

> catalog.links
[<Link rel=child target=<Catalog id=berlin>>,
 <Link rel=self target=s3://rasterfoundry-development-data-us-east-1/berlin-catalog/catalog.json>,
 <Link rel=parent target=<Catalog id=berlin>>,
 <Link rel=root target=<Catalog id=berlin>>]

[lossyrob]

Looks like both the catalog and collection have the same ID. The logic in PySTAC requires ID's to be unique (also required by the spec).

What happens is PySTAC goes to read the collection link, sees that it's an object with ID 'berlin', and sees that it already has an object in memory with ID berlin, so resolves the read of the collection to the previously read instance of the 'berlin' catalog. This causes recursion on traversal.

This is the same object resolution that ensures, for instance, items that link to the same Collection end up pointing to the same instance of that collection in-memory when PySTAC reads the them in from a catalog.

Not sure the best way to handle this. There could be an object identity check for any non 'self' or 'root' link added to make sure the object the link points to is not the same object. Open to suggestions.

[jisantuc]

Is the global uniqueness constraint mentioned in the spec proper somewhere? I've only found it in item IDs and best practices. That feels a little bit short of "also required by the spec," but it may just be a case where the spec has been unclear.

I'm not really sure what the library should do, but if it just let me know somehow that this was my fault, that'd help -- infinite recursion in a catalog that I "know" is fine feels like a library error, and the surprise bonus link made it feel more that way. I have largely avoided looking at how PySTAC works to this point so I don't have any concrete technical suggestions.

[lossyrob]

Is the global uniqueness constraint mentioned in the spec proper somewhere?

I was interpreting the "they just need to be sure it is globally unique" as required, but it's true it's not specifically defined as required in the spec. Perhaps that constraint needs to be relaxed in PySTAC - will take some thought on how to re-engineer the caching to accommodate that. I'll give that some thought.

Regardless, totally agree that infinite looping is not an ideal outcome here - I think putting some sort of identity check to provide a more helpful error message would be the way to go as long as unique IDs are assumed. This issue can be closed by a PR that implements that change.