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

Create valid dict without root href #896

Merged
merged 10 commits into from
Jan 10, 2023
Merged

Create valid dict without root href #896

merged 10 commits into from
Jan 10, 2023

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Oct 4, 2022

Related issues:

Description:

Catalogs and Collections always have a root href. If the root Catalog or Collection (e.g. itself) does not have a self href, the dictionary serialization included a root link with href set to null, which is invalid. This fix removes the root link if it doesn't have a valid href.

Not required for items because they don't have a root link by default.

I discovered this problem when trying to write-then-read a very simple Collection using save_object, but the problem proved to be in the dict serialization.

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.

@gadomski
Copy link
Member Author

gadomski commented Oct 4, 2022

Whoops, my markdown linter added some spaces to the CHANGELOG. If those are unwanted noise I can remove, LMK. Cool, someone else's linter already hit these changes, disregard.

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2022

Codecov Report

Base: 94.37% // Head: 94.37% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (5ca4901) compared to base (e77cd0a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #896   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files          83       83           
  Lines       12015    12026   +11     
  Branches     1136     1137    +1     
=======================================
+ Hits        11339    11350   +11     
  Misses        496      496           
  Partials      180      180           
Impacted Files Coverage Δ
pystac/catalog.py 91.58% <100.00%> (ø)
tests/test_catalog.py 99.02% <100.00%> (+<0.01%) ⬆️
tests/test_collection.py 99.61% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Catalogs and Collections always have a root href. If the root Catalog or
Collection (e.g. itself) does not have a self href, the dictionary serialization
included a root link with href set to `null`, which is invalid. This fix removes
the root link if it doesn't have a valid href.

Not required for items because they don't have a root link by default.

I discovered this problem when trying to write-then-read a very simple
Collection using `save_object`, but the problem proved to be in the dict
serialization.
@gadomski gadomski force-pushed the simple-collection-save branch from a9fe18c to 7dca7a7 Compare October 13, 2022 14:32
@gadomski gadomski added the bugfix Fixes a bug label Nov 7, 2022
@gadomski gadomski self-assigned this Dec 30, 2022
@gadomski gadomski added this to the 1.7 milestone Dec 30, 2022
@gadomski gadomski requested a review from pjhartzell January 9, 2023 20:07
Copy link
Collaborator

@pjhartzell pjhartzell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes address #183 from a practical standpoint. The creation of null href root links seems embedded in pystac (editing catalog.py such that a root link that will have a null href is not created causes multiple test failures).

@gadomski
Copy link
Member Author

editing catalog.py such that a root link that will have a null href is not created causes multiple test failures

Yup, I found the same thing -- PySTAC kind of expects a root, whether it has a href or not.

@gadomski gadomski merged commit 9d11ef5 into main Jan 10, 2023
@gadomski gadomski deleted the simple-collection-save branch January 10, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a collection without links has an unexpected side-effect
3 participants