RFC: Use mutable dictionaries for object properties

[duckontheweb]

Related Issue(s):

• Slowness in Item.to_dict(), seemingly from links #546

Description:

This PR is intended to be a proof-of-concept for changing the JSON-serializable classes to maintain a mutable dictionary in a fields property instead of setting instance attributes. The current draft only implements this for the Asset class, but if there is interest I will attempt to implement this for the Link class next since we have seen performance issues in the to_dict method there.

Some possible advantages to this approach are:

• Improvement in serialization/deserialization performance, especially when using preserve_dict=False
• Possibly easier/clearer typing
  If we are able to implement this for all JSON serializable classes, then we may not need some of the Union types in our PropertiesExtension classes since those could utilize the fields dictionary instead. This may also ease some of our circular imports since we might be able to avoid some blocks like:

  if isinstance(obj, pystac.Item):
      ...
  elif isinstance(obj, pystac.Asset):
      ...

  that are necessitating those imports
• Methods like Item.set_collection might no longer be necessary if we can handle them within the setters

Some possible hiccups:

• Handling attributes of the Item class might be tricky since some of them are at the top level and some are in the properties.
• Possibly worse performance when accessing instance attributes (e.g. Asset.href) since we are now accessing those as properties.
• May not be able to completely preserve backwards compatibility
  The current behavior of the extra_fields attribute on some of the classes may be hard to replicate. Currently, this attribute is a mutable dictionary that only includes the fields not covered by specific attributes (e.g. Asset.href), but also allows mutatation of the object (e.g. asset.extra_fields["some_field"] = "some_value" will add the "some_field" property to the Asset when serialized).

PR Checklist:

• Code is formatted (run pre-commit run --all-files)
• Tests pass (run scripts/test)
• Documentation has been updated to reflect changes, if applicable
• This PR maintains or improves overall codebase code coverage.
• Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

cc: @TomAugspurger @gjoseph92

[codecov-commenter]

Codecov Report

Merging #617 (c0edaff) into main (5465979) will decrease coverage by 0.02%.
The diff coverage is 89.74%.

@@            Coverage Diff             @@
##             main     #617      +/-   ##
==========================================
- Coverage   94.72%   94.70%   -0.03%     
==========================================
  Files          75       76       +1     
  Lines       10834    10866      +32     
  Branches     1060     1058       -2     
==========================================
+ Hits        10263    10291      +28     
- Misses        396      399       +3     
- Partials      175      176       +1     

Impacted Files	Coverage Δ
tests/extensions/test_custom.py	54.83% <0.00%> (ø)
pystac/common_metadata.py	92.72% <62.50%> (ø)
pystac/base.py	84.61% <84.61%> (ø)
pystac/asset.py	90.12% <95.23%> (-0.20%)	⬇️
pystac/extensions/datacube.py	64.28% <100.00%> (ø)
pystac/extensions/eo.py	93.11% <100.00%> (ø)
pystac/extensions/file.py	92.06% <100.00%> (ø)
pystac/extensions/pointcloud.py	97.76% <100.00%> (ø)
pystac/extensions/projection.py	98.46% <100.00%> (ø)
pystac/extensions/raster.py	89.70% <100.00%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5465979...c0edaff. Read the comment docs.

[duckontheweb]

I've been working on this some more here and there, but I'm having trouble implementing this in a non-breaking way. I will open up a new PR with a breaking implementation that we can consider as part of a new major version release.