-
Notifications
You must be signed in to change notification settings - Fork 15
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
[core]: Models, containers for items & events #169
Conversation
# XXX: 'version' feels like it's duplicated between .properties.version | ||
# and this, for StacItemRecord. Do we want that? Potential for drifting. | ||
version: Optional[str] |
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.
@lossyrob when you get a chance, I'm curious about the decision to elevate version
to a top-level property of this ItemRecord
model.
Actually... I guess I see it now. ItemRecord
is a base class for both StacItemRecord
and ItemUpdateRecord
, and you want to ensure that version
is present in the ItemUpdateRecord
, so that we can use it in things like get_id
. While StacItemRecord
might have this in .properties["version"]
, ItemUpdateRecord
wouldn't have anywhere else for it.
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.
Yeah, for StacItemRecord
and ItemUpdateRecord
that's the reasoning.
However, we might in the future want documents that are saved as part of the /stac_id
partition that isn't specific to a version. Potentially removing from the base and putting version in both StacItemRecord
and ItemUpdateRecord
would be a good future-proofing move.
In preparation for low-latency, we're adding / updating a handful of models, some of which are persisted to Cosmos DB. This adds the pydantic models for that.
a9e7b07
to
1e3f977
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.
Changes look good. The two updates you mentioned - moving version out of the base model class and using ::
instead of :None:
in the document IDs, should be made before merging.
# XXX: 'version' feels like it's duplicated between .properties.version | ||
# and this, for StacItemRecord. Do we want that? Potential for drifting. | ||
version: Optional[str] |
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.
Yeah, for StacItemRecord
and ItemUpdateRecord
that's the reasoning.
However, we might in the future want documents that are saved as part of the /stac_id
partition that isn't specific to a version. Potentially removing from the base and putting version in both StacItemRecord
and ItemUpdateRecord
would be a good future-proofing move.
return v | ||
|
||
def get_id(self) -> str: | ||
version = getattr(self, "version", "") |
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.
Ugly, but the best way I could think of to convince mypy that this would be OK (normally, I would use a mixin with a typed class attribute, but that conflicts with how pydantic does stuff).
In preparation for low-latency, we're adding / updating a handful of models, some of which are persisted to Cosmos DB.
This PR adds the pydantic models and terraform (to make the Cosmos DB tables) for that.