MAINT: Refactor 'schema'/'internal' classes #962
Conversation
mferrera
force-pushed
the
move-metadata-so-called-schema
branch
2 times, most recently
from
10aae83 to
bfe17a5
Compare
This restructures how these classes are formed and where they live. It was confusing to locate them with the model when they are really just tweaks to the model for us when exporting. This refactor tries to bring some clarity to that relationship.
mferrera
force-pushed
the
move-metadata-so-called-schema
branch
from
bfe17a5 to
209b8ff
Compare
|
Trying to do some structural clean-ups before these little schema changes starting incurring a version change. Have another one or two to follow |
| @@ -24,6 +38,38 @@ | |||
| logger: Final = null_logger(__name__) | |||
|
|
|||
|
|
|||
| class JsonSchemaMetadata(BaseModel): | |||
There was a problem hiding this comment.
Thought for a separate PR, could this be removed from here and used in the _model.root instead? 🙂
Right now we have a mismatch between the $schema defined in the main schema and the "exported". That would take us one step closer to removing the need for a separate model for the exported metadata, I look forward to the day we can align them fully!
There was a problem hiding this comment.
I think it can't actually, this is a bit confusing:
- The 'Root' json schema is validated by $schema = https://json-schema.org/draft/2020-12/schema
- Metadata we export is validated against $schema = https://main-fmu-schemas-prod.radix.equinor.com/schemas/0.8.0/fmu_results.json
It's confusing because our schema is getting validated by a 'worldwide' schema (validating that it's a valid schema), while our metadata is validated by our schema
There was a problem hiding this comment.
annoying 😩 .. and confusing yes
| # TODO: Would like to use meta.Root.model_validate() here | ||
| # but then the '$schema' field is dropped from the meta_existing | ||
| validated_metadata = schema.InternalObjectMetadata.model_validate( | ||
| meta_existing | ||
| ) | ||
| validated_metadata = ObjectMetadataExport.model_validate(meta_existing) |
There was a problem hiding this comment.
If we could add the $schema to the Root model instead, we could address the TODO here
| class ObjectMetadataExport(JsonSchemaMetadata, ObjectMetadata, populate_by_name=True): | ||
| """Wraps the schema ObjectMetadata, adjusting some values to optional for pragmatic | ||
| purposes when exporting metadata.""" | ||
|
|
||
| # These type ignores are for making the field optional | ||
| fmu: Optional[fields.FMU] # type: ignore | ||
| access: Optional[fields.SsdlAccess] # type: ignore | ||
| masterdata: Optional[fields.Masterdata] # type: ignore | ||
| # !! Keep UnsetData first in this union | ||
| data: Union[UnsetData, data.AnyData] # type: ignore | ||
| preprocessed: Optional[bool] = Field(alias="_preprocessed", default=None) |
There was a problem hiding this comment.
We are no longer creating invalid metadata since version 2.6.0 #793. ➡️ hence we can soon drop the optional fields here: 💥 🙂
There was a problem hiding this comment.
I didn't fully realize this... maybe we can have a chat about it
This restructures how these classes are formed and where they live. It was confusing to locate them with the model when they are really just tweaks to the model for us when exporting. This refactor tries to bring some clarity to that relationship.
It also changes some of the metadata classes to use their literal values as defaults which changed the schema.