STACVersionID assigned string value in migration

[keithpeck]

https://github.com/stac-utils/pystac/blob/main/pystac/serialization/identify.py#L268
I believe version_range.set_to_single(stac_version)
should be version_range.set_to_single(STACVersionID(stac_version))

This issue results in min_version and max_version being assigned strings. Later during migrations it results in bad code paths.

This only occurs when the item stac_version is < STACVersion.DEFAULT_STAC_VERSION, which is why it doesn't happen during test_older_extension_version and probably wasn't common scenario until the STAC 1.1.0 update.

    item_as_dict["stac_version"] = "1.0.0"
    migrated_item = pystac.Item.from_dict(item_as_dict, migrate=True)

Does cause the issue.

[gadomski]

Sorry, I can't reproduce the issue. This test passes:

def test_manual_version_assignment(item: Item) -> None:
    d = item.to_dict(include_self_link=False, transform_hrefs=False)
    d["stac_version"] = "1.0.0"
    item = Item.from_dict(d, migrate=True)

Can you provide a failing test case? I think I understand the problem you're describing, but I'd like to surface it as an actual error so we can add a regression test for it. Thanks!

[keithpeck]

@gadomski thanks for taking a look at this. You are right, that alone doesn't present the issue. This does - having bands declared on both the properties and assets. This was what triggered the issue for us, but I'm sure issue can present elsewhere.

If test_older_extension_version is modified with:

    item_as_dict["stac_version"] = "1.0.0"
    item_as_dict["properties"]["eo:bands"] = [{"name": "B1", "common_name": "coastal"}]

It raises "TypeError: list indices must be integers or slices, not dict"

Due to the incorrect assigning of version with a string instead of a STACVersionID object, we end up with a string comparison of "1.0.0" < "1.0.0-beta.1" in extensions/eo.py which passes as true, and causes us to drop into the 1.0.0-beta.1 migration logic even though the item is 1.0.0.

In terms of a new test case. Maybe something like this:

def test_unnecessary_migrations_not_performed(ext_item: Item) -> None:
    item_as_dict = ext_item.to_dict(include_self_link=False, transform_hrefs=False)
    item_as_dict["stac_version"] = "1.0.0"
    item_as_dict["properties"]["eo:bands"] = [{"name": "B1", "common_name": "coastal"}]
    
    item = Item.from_dict(item_as_dict)

    migrated_item = pystac.Item.from_dict(item_as_dict, migrate=True)

    assert item.properties == migrated_item.properties
    assert len(item.assets) == len(migrated_item.assets)
    for key, value in item.assets.items():
        assert value.to_dict() == migrated_item.assets[key].to_dict()

[jsignell]

Thanks for the test! I can take a crack at this.

[keithpeck]

Thanks for the PR on this. When can we expect a release with this fix?

[gadomski]

Thanks for the poke! #1529