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

Update get_item in sqlalchemy backend #275

Merged
merged 12 commits into from
Oct 19, 2021
Merged

Update get_item in sqlalchemy backend #275

merged 12 commits into from
Oct 19, 2021

Conversation

jonhealy1
Copy link
Collaborator

Related Issue(s): #186

Description:
Update get_item in sqlalchemy backend to allow for querying for items with same ids but in different collections. Add test.

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.

@robintw
Copy link
Contributor

robintw commented Oct 11, 2021

I've also been running into this problem with stac-fastapi currently requiring Item IDs to be unique across the whole database, rather than just within a collection.

On top of this change, I think for the SQLAlchemy backend we would need to update stac_fastapi\sqlalchemy\stac_fastapi\sqlalchemy\models\database.py, as the Item table currently has the id as the primary key. We would need to change this to make the combination of the id and collection_id fields the primary key.

I've tested this with creating two items with the same ID but in different collections, using the SQLAlchemy backend - and with these additions it works fine.

I can't seem to push to this PR, so I've added the relevant code below:

Add stac_fastapi\sqlalchemy\alembic\versions\821aa04011e8_change_pri_key_for_item.py:

"""Change pri key for Item

Revision ID: 821aa04011e8
Revises: 407037cb1636
Create Date: 2021-10-11 12:10:34.148098

"""
from alembic import op
import sqlalchemy as sa
from sqlalchemy.sql import schema


# revision identifiers, used by Alembic.
revision = '821aa04011e8'
down_revision = '407037cb1636'
branch_labels = None
depends_on = None


def upgrade():
    op.drop_constraint("items_pkey", "items", schema="data")
    op.create_primary_key("items_pkey", "items", ["id", "collection_id"], schema="data")


def downgrade():
    op.drop_constraint("items_pkey", "items", schema="data")
    op.create_primary_key("items_pkey", "items", ["id"], schema="data")

stac_fastapi\sqlalchemy\stac_fastapi\sqlalchemy\models\database.py:
Alter the definition of collection_id within the Item class to:

collection_id = sa.Column(
        sa.VARCHAR(1024), sa.ForeignKey(Collection.id), nullable=False, primary_key=True
    )

@jonhealy1
Copy link
Collaborator Author

@robintw Hi. Great feedback. Can you fork my branch and push to it? I'm not a git expert or anything.

@robintw
Copy link
Contributor

robintw commented Oct 11, 2021

@jonhealy1 Yup, I'll do that later today.

@lossyrob
Copy link
Member

These changes LGTM, waiting on @jonhealy1 to merge @robintw PR into the branch of this PR, at which point all this should need is a changelog entry

@jonhealy1
Copy link
Collaborator Author

@lossyrob Ok done

@lossyrob lossyrob merged commit 607e182 into stac-utils:master Oct 19, 2021
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.

3 participants