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.

Sign up for GitHub

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

Jump to bottom

PB-35 Implemented external asset urls #433

Merged
merged 28 commits into from
Jul 30, 2024
Merged

PB-35 Implemented external asset urls #433

merged 28 commits into from
Jul 30, 2024

Conversation

schtibe
Copy link
Contributor

@schtibe schtibe commented Jul 16, 2024 •
edited
Loading

  • Added a flag on the collection to allow for external assets and a char field to provide a pattern
  • Use the href field on asset creation request to ingest external URLs if that's allowed from the collection
  • Disallow multipart upload creation for the external assets
  • Validate the external url for
    • General URL validity
    • If it matches the collection pattern
    • If the provided URL is reachable
  • Adjust Asset admin page so that an asset can only be external if the collection allows it

I don't know if this is everything that's needed, but nevertheless I think a review would be good.

@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch 3 times, most recently from 3e6146f to 0074cdf Compare July 18, 2024 17:12
@schtibe
PB-35 Use href field to allow for specifying the asset url explicitly
4033368 
For certain use cases we want to specify a url for the asset instead
of uploading it to our s3 bucket.
Using the existing href parameter to provide for that
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch 10 times, most recently from 9807e90 to bb4611e Compare July 19, 2024 13:23
@schtibe schtibe changed the title WIP Feat-pb-35-external-asset-url PB-35 Implemented external asset urls Jul 19, 2024
@schtibe schtibe requested review from boecklic and benschs July 19, 2024 14:22
@schtibe schtibe marked this pull request as ready for review July 19, 2024 14:22
boecklic
boecklic approved these changes Jul 22, 2024
View reviewed changes
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!

app/stac_api/migrations/0037_asset_is_external_collectionasset_is_external.py Outdated Show resolved Hide resolved
app/stac_api/migrations/0039_alter_asset_is_external_and_more.py Outdated Show resolved Hide resolved
app/stac_api/migrations/0040_collection_external_asset_pattern.py Show resolved Hide resolved
app/stac_api/models.py Outdated Show resolved Hide resolved
app/stac_api/serializers.py Outdated Show resolved Hide resolved
app/stac_api/serializers.py Outdated Show resolved Hide resolved
app/tests/tests_10/sample_data/asset_samples.py Outdated Show resolved Hide resolved
app/tests/tests_10/test_external_assets_endpoint.py Outdated Show resolved Hide resolved
@benschs
Copy link
Contributor

benschs commented Jul 22, 2024

Looks good. Maybe also merge all the db migration files into a single file?

@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch from bb4611e to 44d40c8 Compare July 22, 2024 11:56
@schtibe
Copy link
Contributor Author

schtibe commented Jul 22, 2024

Looks good. Maybe also merge all the db migration files into a single file?

Gonna do that, or at least partially. The changes to the models have separate commits, thus I'm gonna keep their migrations separate as well!

@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch 2 times, most recently from ca6adf7 to 6dda8f1 Compare July 22, 2024 12:26
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch from be99cd4 to 298b711 Compare July 23, 2024 12:21
@schtibe schtibe requested a review from boecklic July 23, 2024 12:27
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch 2 times, most recently from 8cc51c5 to 986f98b Compare July 25, 2024 15:58
benschs
benschs approved these changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@benschs benschs left a comment

Choose a reason for hiding this comment

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

Instead of needing to save the admin form to set a file or external asset, would it be possible to allow to set external or upload a file to begin with, and then return an error on save if the collection does not allow it?

app/stac_api/admin.py Outdated Show resolved Hide resolved
@schtibe
Copy link
Contributor Author

schtibe commented Jul 29, 2024

Instead of needing to save the admin form to set a file or external asset, would it be possible to allow to set external or upload a file to begin with, and then return an error on save if the collection does not allow it?

I'm not against it per se, but I would like to hear @boecklic's opinion, especially regarding as to how much effort we want to put into the admin or if that won't even be used that much.
BTW. the current workflow is the same as with the creation of Users (default Django), so it's at least a common workflow.

@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch 2 times, most recently from f46ac78 to 2544338 Compare July 29, 2024 11:10
@schtibe
PB-35 Improve URL reachability validator
5d2c34d 
Also handle
* connection errors, for instance when the domain isn't reachable
* redirects and other 300 codes are OK too
@schtibe
PB-35 Switch to list of external asset pattern
8fa6232 
Change the regex validation of external asset urls to a list of strings.
Matching those in the beginning of the URL
@schtibe
PB-35 Test the deletion of external assets
efddd0a
@schtibe
PB-35 Log the behaviour of the external asset URL validation
42e3269
@schtibe
PB-35, PB-617 Add missing documentation for the new env variables
edd751c
@schtibe
PB-35 Fix django admin with file fields
aacf949 
The form didn't allow creating assets anymore. The changes for the file
field only worked if the asset already existed.
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch 2 times, most recently from 3b934d8 to 1dbca44 Compare July 29, 2024 13:54
@schtibe
PB-35 Validate the external assets in Django Admin
bf47982 
Refactor the validators on the external asset serializer to the
validators module for that purpose. Refactored the Django admin for
Assets a bit, fixed a bug in the process.
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch from 1dbca44 to bf47982 Compare July 29, 2024 14:48
@boecklic
Copy link
Contributor

Instead of needing to save the admin form to set a file or external asset, would it be possible to allow to set external or upload a file to begin with, and then return an error on save if the collection does not allow it?

I'm not against it per se, but I would like to hear @boecklic's opinion, especially regarding as to how much effort we want to put into the admin or if that won't even be used that much. BTW. the current workflow is the same as with the creation of Users (default Django), so it's at least a common workflow.

Currently it's unlikely that external assets will be created through admin, except for testing purposes maybe. So the most effortless approach seems good to me.

boecklic
boecklic approved these changes Jul 30, 2024
View reviewed changes
app/stac_api/validators.py Outdated Show resolved Hide resolved
@schtibe
PB-35 Test the scheme of the external asset URL
df66727 
Test if the URL's scheme is in a disallowed pattern list, foremost to
prevent the usage of http.
Also rework the validators a bit so they follow the same naming scheme
and indicate that they belong together.
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch from c9c4d38 to ff10708 Compare July 30, 2024 09:27
@schtibe
PB-35 Improve logging of URL validation
6ae0cd8 
Switch to log level "WARN". Also make sure every validation logs its
error, including the collection, so we know who the user might be
@schtibe schtibe force-pushed the feat-pb-35-external-asset-url branch from ff10708 to 6ae0cd8 Compare July 30, 2024 09:40
@schtibe schtibe merged commit 2f49b55 into develop Jul 30, 2024
3 checks passed
@schtibe schtibe deleted the feat-pb-35-external-asset-url branch July 30, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boecklic boecklic boecklic approved these changes

@benschs benschs benschs approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schtibe @benschs @boecklic