-
Notifications
You must be signed in to change notification settings - Fork 4
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(ingestor-api)!: store STAC item as string in DynamoDB #26
Conversation
a3246d4
to
afc3655
Compare
afc3655
to
caee41a
Compare
4ba3bdd
to
bb864e0
Compare
bb864e0
to
ec31203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @sharkinsspatial! Thanks for adding those tests. I added a few more (once some are written, it makes it so much easier to expand).
I rewrote a bit of the code to make use of fastapi.encoders.jsonable_encoder()
which I think makes for slightly cleaner code.
Feeling really good about this change!
class Config: | ||
json_encoders = { | ||
# Custom JSON serializer to ensure that item encodes as string. | ||
# NOTE: when serializing, must call as ingestion.json(models_as_dict=False) | ||
Item: lambda item: item.json(by_alias=True), | ||
} | ||
|
||
def json(self, *args, **kwargs): | ||
# Update default to not represent models (e.g. `items` property) as a dict to | ||
# allow our `json_encoders` override to properly serialize `items` property | ||
kwargs.setdefault("models_as_dict", False) | ||
return super().json(*args, **kwargs) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sharkinsspatial My rationale here is that if we need to put all this cruft in to make special considerations for serializing our Ingestion
object for DynamoDB, it would probably make for cleaner code if we just put that cruft into the dynamodb_dict()
method.
* fix: stac item serialization in ingestion model * fix: use pydantic parsing and remove float conversion * test: add initial test for missing ingestor coverage * fix: fix updated psql function name changed in db bootstrapper * style: fix black errors * test: add utils.load_items test for new item serialization * style: black and isort style changes * Make use of jsonable_encoder, buildout tests for submitting STAC items --------- Co-authored-by: Anthony Lukach <[email protected]>
* fix: stac item serialization in ingestion model * fix: use pydantic parsing and remove float conversion * test: add initial test for missing ingestor coverage * fix: fix updated psql function name changed in db bootstrapper * style: fix black errors * test: add utils.load_items test for new item serialization * style: black and isort style changes * Make use of jsonable_encoder, buildout tests for submitting STAC items --------- Co-authored-by: Anthony Lukach <[email protected]>
## [2.6.3](v2.6.2...v2.6.3) (2023-03-28) ### Bug Fixes * **ingestor-api:** store STAC item as string in DynamoDB ([#26](#26)) ([bd7a1fa](bd7a1fa))
# What Fixes the float epsg code issue caused by dynamodb serialization Picked up the fix from developmentseed/eoapi-cdk#26 # How test Tested by running ingest from here: https://mv5tl66nca.execute-api.us-west-2.amazonaws.com/ Test item: https://vr3ofct5u8.execute-api.us-west-2.amazonaws.com/api/stac/collections/geoglam/items/CropMonitor_202205_test_2 ```python import pystac item = pystac.Item.from_file("https://vr3ofct5u8.execute-api.us-west-2.amazonaws.com/api/stac/collections/geoglam/items/CropMonitor_202205_test_2") print(item.properties["proj:epsg"]) # should give 4326 instead of 4326.0 4326 ```
This PR addresses the issue raised in NASA-IMPACT/veda-data#57. The PR updates the ingestor code to serialize the STAC
item
property of theingestion
as a string prior to storing it in DynamoDB. Additionally, it updates the code used to deserializeingestions
queried from the DynamoDB table and load them intopgstac
and adds unit test coverage for some of theingestor
code paths (which previously had no coverage).