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

PB-769: Required fields to comply with stac v1 #426

Merged
merged 7 commits into from
Jul 11, 2024

Conversation

benschs
Copy link
Contributor

@benschs benschs commented Jul 3, 2024

Following changes are done to v1 (v0.9 stays unchanged)

Landing Page

  • Add field type
  • Add field conformsTo

Collection

  • Add field type

Item

  • Rename checksum:multihash to file:checksum
  • Rename eo:gsd to gsd

These changes will remove the ConformancePage table from the database. The information is moved to the LandingPage.

@benschs benschs requested review from schtibe, boecklic and ltshb July 3, 2024 07:24
@benschs benschs force-pushed the feat-PB-769-comply-stac-v1 branch 2 times, most recently from f05f960 to 85fa313 Compare July 3, 2024 11:37
Copy link
Contributor

@boecklic boecklic left a comment

Choose a reason for hiding this comment

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

looks good. I left a comment on the test PR, there's a project that claims to be able to check STAC APIs for validity, maybe it could be useful to validate these changes and make sure we catched all changes

@benschs benschs requested a review from boecklic July 4, 2024 12:53
Pipfile Outdated
@@ -39,6 +39,7 @@ django-prometheus = "~=2.3.1"
django-admin-autocomplete-filter = "~=0.7.1"
django-pgtrigger = "~=4.11.1"
logging-utilities = "~=4.4.1"
stac-api-validator = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not this be in the dev-package ? In the package section we should have fix major, minor version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Yes that makes sense. I added to dev-deps and fixed the version to the current latest.

@benschs benschs force-pushed the feat-PB-769-comply-stac-v1 branch 7 times, most recently from 753fd3a to 5be59f9 Compare July 10, 2024 11:46
Copy link
Contributor

@boecklic boecklic left a comment

Choose a reason for hiding this comment

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

very nice! thanks.

@@ -15,14 +15,11 @@
from django.http import HttpResponseRedirect
from django.urls import reverse

from solo.admin import SingletonModelAdmin
Copy link
Contributor

Choose a reason for hiding this comment

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

was this the only we used that SingletonModelAdmin? If yes we could remove the package from the dependancy list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint, I removed the dependency. 👍

]

operations = [
migrations.RunPython(add_landing_page_version, reverse_landing_page_version),
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Comment on lines +3 to +13
from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
('stac_api', '0032_alter_asset_file'),
('stac_api', '0033_auto_20240704_1157'),
]

operations = []
Copy link
Contributor

Choose a reason for hiding this comment

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

was this migration done automatically? Merging migrations has not always worked well in the past, but this is already a while ago, so it's probably reliable now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this was created automatically because of the conflicts with the changes for the new s3 bucket. I am not aware of any migration issues, it seems to work so far.

@@ -565,11 +557,15 @@ class Meta:
def filename(self):
return os.path.basename(self.file.name)

# From v1 on the json representation of this field changed from "checksum:multihash" to
# "file:checksum". The two names may be used interchangeably for a now.
checksum_multihash = models.CharField(
Copy link
Contributor

Choose a reason for hiding this comment

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

cosmetics, but we could rename the fields to relate to the naming in v1...

benschs added 6 commits July 11, 2024 08:42
Add field type to catalog (landing page) and collection response body.
Introduce enum to support handling api version checks.
Rename asset multihash json encoding from checksum:multihash to file:multihash.
Rename asset gsd json encoding. Introduce test_v0.9 path namespace to fix test
cases that rely on v0.9 and test the old encoding.
Find and replace file:multihash to file:checksum
Add conformsTo to landing page response. Add conformsTo array to LandingPage
model and remove the ConformancePage model.
Return landing page information based on api version.
Introduce stac-api-validator to check stac core conformance manually.
Conform with stac v1 collection definitions. Add 0 bbox if empty. This is only
an edge case if the collection has no assets. Add collection conformance link.
@benschs benschs force-pushed the feat-PB-769-comply-stac-v1 branch from 5be59f9 to 03927fd Compare July 11, 2024 06:59
Remove dependency to the singleton package solo as it is no longer needed.
@benschs benschs force-pushed the feat-PB-769-comply-stac-v1 branch from 03927fd to f586994 Compare July 11, 2024 07:10
@benschs benschs merged commit e508c8b into develop Jul 11, 2024
3 checks passed
@benschs benschs deleted the feat-PB-769-comply-stac-v1 branch July 11, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants