pierrejeambrun commented on code in PR #46827:
URL: https://github.com/apache/airflow/pull/46827#discussion_r1967892745
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line.
Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed, but I'm sure we have
encountered this. (I would have to check all the model to remember, It happened
only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I
agree that this is a more accurate representation. It's just that having to
manually specify the spec for those fields that are `undefined | actual_type`
is annoying. (especially if the type is another Datamodel / not a primitive
type).
That's good for me if you think that's preferable.
##########
airflow/api_fastapi/core_api/datamodels/log.py:
##########
@@ -16,11 +16,29 @@
# under the License.
from __future__ import annotations
-from pydantic import BaseModel
+from datetime import datetime
+from typing import Annotated
+
+from pydantic import BaseModel, ConfigDict, WithJsonSchema
+
+
+class StructuredLogMessage(BaseModel):
+ """An individual log message."""
+
+ # Not every message has a timestamp.
+ timestamp: Annotated[
+ datetime | None,
+ # Schema level, say this is always a datetime if it exists
+ WithJsonSchema({"type": "string", "format": "date-time"}),
Review Comment:
> The risk of it getting out of sync is minimal -- it's on the next line.
Yes, if the code tried to send a None it would return an error.
Ok, sounds good.
> Such as?
Can't remember exactly the datamodel that needed, but I'm sure we have
encountered this. (I would have to check all the model to remember, It happened
only once or twice).
> Why did you choose not to override it?
Simply adding a null check in the front-end wasn't too expensive, but I
agree that this is a more accurate representation. It's just that having to
manually specify the spec for those fields that are `undefined | actual_type`
is annoying. (especially if the type is another Datamodel / not a primitive
type).
That's good for me if you think that's preferable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
