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 to stac-pydantic 2.0.0 #181

Merged
merged 15 commits into from
Jul 23, 2021

Conversation

moradology
Copy link
Collaborator

This PR upgrades the version of stac-pydantic to 2.0.0 so that the library can use stac-spec 1.0.0. Mostly, the changes are import updates, though collections are now represented as stac-pydantic's Collections type rather than a List[Collection]

@moradology moradology force-pushed the feature/stac-pydantic-2.0.0 branch from d13108e to 295ec37 Compare July 7, 2021 15:05
@moradology
Copy link
Collaborator Author

Apologies if the failed linting is blowing up anyone's emails - I'm attempting to track down the options I'm failing to use which seem to be expected by CI's run of black

@moradology
Copy link
Collaborator Author

It looks as though stac-pydantic likes black-19.10b0 whereas stac-fastapi is a fan of black-21.6b0

@moradology moradology changed the title Update to stac-pydantic 2.0.0 Update to stac-pydantic 2.0.0 - WIP Jul 8, 2021
@@ -39,6 +39,7 @@ def create_collection(
) -> schemas.Collection:
"""Create collection."""
data = self.collection_table.from_schema(model)
data.type = "collection"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is most certainly not going to be kept. It ought to be removed before the PR is accepted as it is residue from debugging

@moradology moradology force-pushed the feature/stac-pydantic-2.0.0 branch from c5257aa to cbac663 Compare July 8, 2021 19:36
@moradology moradology marked this pull request as draft July 8, 2021 20:18
@moradology
Copy link
Collaborator Author

moradology commented Jul 13, 2021

There's one test that is failing and which I'm having a hard time making heads or tails of:

def test_item_timestamps(app_client, load_test_data):
"""Test created and updated timestamps (common metadata)"""
test_item = load_test_data("test_item.json")
start_time = datetime.utcnow()
time.sleep(2)
# Confirm `created` timestamp
resp = app_client.post(
f"/collections/{test_item['collection']}/items", json=test_item
)
item = resp.json()
created_dt = datetime.strptime(item["properties"]["created"], DATETIME_RFC339)
assert resp.status_code == 200
assert start_time < created_dt < datetime.utcnow()
time.sleep(2)
# Confirm `updated` timestamp
item["properties"]["proj:epsg"] = 4326
resp = app_client.put(f"/collections/{test_item['collection']}/items", json=item)
assert resp.status_code == 200
updated_item = resp.json()
# Created shouldn't change on update
assert item["properties"]["created"] == updated_item["properties"]["created"]
assert (
datetime.strptime(updated_item["properties"]["updated"], DATETIME_RFC339)
> created_dt
)

At nearly every step along the request/response cycle that I can surface the created timestamp, it appears to be correctly formatted (e.g.

now = datetime.utcnow().strftime(DATETIME_RFC339)
if not properties["created"]:
properties["created"] = now
). It is also the case that the appropriate pydantic serialization arguments have been provided via class Config. Nevertheless, whenever that field is inspected in test_item_timestamps, RFC 3339 formatting (which we expect) is not there.

A second or third pair of eyes might be useful at this point.

@geospatial-jeff
Copy link
Collaborator

geospatial-jeff commented Jul 19, 2021

@moradology I don't know why this is happening, but it's related to how FastAPI is applying the response_model. For some reason it is adding a time zone component even though we aren't telling it to do that, although this is still RFC3339 compliant so maybe its not all that bad. The test failure is happening because the pydantic model is using pydantic's datetime parser while the test itself is expecting a very specific type of RFC3339 string coming from stac-pydantic. We can fix the test by updating the test case to use pydantic's datetime parser.

I'm still confused why adding a response_model changes how the datetime is serialized. It seems to be specific to context, if you call model.json() or model.dict() we get a date string that ends with Z but when FastAPI applies the response_model it creates a date string with a timezone ending in +00:00. Smells very funky, but maybe its not an issue because the date included in the response is still valid RFC3339.

@geospatial-jeff
Copy link
Collaborator

Thinking through this a little more:

  1. We are using stac_pydantic.shared.DATETIME_RFC3339 in other test cases and those tests don't break. Which means this seems to be specific to updating existing items in the database. No idea why this is the case 🤷‍♂️
  2. This tells me that it's probably related to the database or sqlalchemy update syntax, the database is using timestamps but if this were really the cause of the underlying problem I think we'd see the other test cases fail.
  3. This is even more reason to move away from using pydantic for response models (ref support custom models #147).

Either way I've fixed this test in bfa203e

@geospatial-jeff
Copy link
Collaborator

@moradology I'm marking this ready for review, please take a look over the changes I made.

@geospatial-jeff geospatial-jeff marked this pull request as ready for review July 19, 2021 03:34
@moradology moradology changed the title Update to stac-pydantic 2.0.0 - WIP Update to stac-pydantic 2.0.0 Jul 19, 2021
@lossyrob
Copy link
Member

@geospatial-jeff thinking through review strategy, which I have some time set aside for today - I'd like to review this PR and get it merged, and then update #147 with these changes and integrate while reviewing. I'll make tweaks along the way but will tag you for any larger review changes if I find them. Let me know if you want to tweak that plan, otherwise I'm going to get started.

@geospatial-jeff
Copy link
Collaborator

geospatial-jeff commented Jul 23, 2021

@lossyrob That sounds good to me. Either way the merge conflicts are going to be nasty :)

Copy link
Member

@lossyrob lossyrob left a comment

Choose a reason for hiding this comment

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

LGTM. Added a couple of commits on top that I needed for testing the PR in the dockerized environment. Will merge once this passes and integrate with #147

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