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

fix sqlalchemy bulk item insertion #358

Merged

Conversation

philvarner
Copy link
Collaborator

Related Issue(s):

Description:

SQLAlchemy bulk item insertion failed because the Items iterator returns the keys for the dict it contains, rather than the Item objects themselves as it's docstring says it should (and that it would make sense to).

This fixes that problem, and documents what the format of the JSON object that is the input to the bulk insert endpoint is.

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.

@@ -12,13 +12,13 @@


class Items(BaseModel):
"""A list of STAC items."""
"""A group of STAC Item objects, in the form of a dictionary from Item.id -> Item."""

items: Dict[str, Any]
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't followed all the discussion but do we need it as a dict? Or could we just have a list of stac items?

Copy link
Collaborator

@geospatial-jeff geospatial-jeff Feb 27, 2022

Choose a reason for hiding this comment

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

a list of items would align better with ItemCollection. But I believe the intent of this PR is to fix the current implementation not change how it works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, I just wanted to fix the behavior rather than change the expected input

Copy link
Collaborator

@geospatial-jeff geospatial-jeff left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the extension, let's open another issue if we want to switch this to ItemCollection as discussed in the linked issues.

@geospatial-jeff
Copy link
Collaborator

Created an issue here: #362

@philvarner philvarner force-pushed the pv/items-iterator-and-bulk-insert branch from e6a62d5 to ebc6967 Compare February 28, 2022 13:22
@philvarner philvarner force-pushed the pv/items-iterator-and-bulk-insert branch from ebc6967 to d5f3d64 Compare March 1, 2022 01:18
@philvarner
Copy link
Collaborator Author

@geospatial-jeff looks like you'll need to merge, I don't have write perms.

@philvarner
Copy link
Collaborator Author

@geospatial-jeff I resolved the merge conflict. I don't have write permission to merge it (which I would gladly accept if you want to grant me it)

@moradology moradology merged commit bb1543f into stac-utils:master Mar 25, 2022
@philvarner philvarner deleted the pv/items-iterator-and-bulk-insert branch March 25, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants