Significantly enhance the safety of metadata manipulation #221
Conversation
benoit74
force-pushed
the
safe_metadata_revamp
branch
from
945daa8
to
c064b54
Compare
benoit74
force-pushed
the
safe_metadata_revamp
branch
2 times, most recently
from
2bccb8d
to
6033f68
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #221 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 38 38
Lines 2224 2369 +145
Branches 426 448 +22
==========================================
+ Hits 2224 2369 +145 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
benoit74
force-pushed
the
safe_metadata_revamp
branch
from
6033f68
to
e759a36
Compare
benoit74
force-pushed
the
safe_metadata_revamp
branch
from
e759a36
to
298beef
Compare
| @@ -136,3 +136,8 @@ def compute_tags( | |||
| return { | |||
| tag.strip() for tag in list(default_tags) + (user_tags or "").split(";") if tag | |||
| } | |||
|
|
|||
|
|
|||
| def unique_values(items: list) -> list: | |||
|
|
||
| import zimscraperlib.zim.metadata |
| By default, all metadata are validated for compliance with openZIM guidelines and | ||
| conventions. Set disable_metadata_checks=True to disable this validation (you can | ||
| By default, all metadata are validated for compliance with openZIM specification and | ||
| conventions. Set check_metadata_conventions=True to disable this validation (you can |
There was a problem hiding this comment.
I guess
| conventions. Set check_metadata_conventions=True to disable this validation (you can | |
| conventions. Set check_metadata_conventions=False to disable this validation (you can |
| if indexing and not is_valid_iso_639_3(language): | ||
| raise ValueError("Not a valid ISO-639-3 language code") | ||
| super().config_indexing(indexing, language) | ||
| self.__indexing_configured = True | ||
| return self | ||
|
|
||
| def _log_metadata(self): | ||
| """Log (DEBUG) all metadata set on (_metadata ~ config_metadata()) | ||
| """Log in DEBUG level all metadata key and value""" |
There was a problem hiding this comment.
Do we still need this method?
It's only limited to the metadata in self._metadata at the time of the call.
Wouldn't it make more sense to simply log calls to add_metadata instead? That would be more true in a sense
| name: str, | ||
| value: bytes | str, | ||
| value: Metadata, | ||
| mimetype: str = "text/plain;charset=UTF-8", |
There was a problem hiding this comment.
Shouldn't this be handled in the Metadata itself. Feels weird to have to specify it here
| super().__init__(name=name, value=value) | ||
|
|
||
|
|
||
| class _MandatoryTextMetadata(_TextMetadata): |
There was a problem hiding this comment.
I see no reason for this to be kept private
| return value | ||
|
|
||
|
|
||
| class _MandatoryMetadata(Metadata): |
There was a problem hiding this comment.
It's a shame both _MandatoryMetadata and _MandatoryTextMetadata are identical.
We could have used a MixIn approach instead of single inheritance ; even stacking checks for for our use case, it's good enough. Just wanted to acknowledge it.
| value = super().libzim_value | ||
| if check_metadata_conventions: | ||
| if nb_grapheme_for(value.decode()) > RECOMMENDED_MAX_TITLE_LENGTH: | ||
| raise ValueError("Title is too long.") |
There was a problem hiding this comment.
Do we want to use this opportunity to raise custom Exceptions (ValueError subclass) for metadata validation error?
| class IllustrationMetadata(_MandatoryMetadata): | ||
| """Any Illustration_**x**@* metadata""" | ||
|
|
||
| def __init__(self, name: str, value: bytes | io.BytesIO) -> None: |
There was a problem hiding this comment.
Given Illustrations have a static name pattern, maybe we should not ask for a name but sizes and scale instead. That would remove the need for the pattern check.
We can imagine having a special Default48IllustrationMetadata or something that only takes data and supers this with 48x48 at 1 ; since that's mandatory
| ) | ||
| super().__init__( | ||
| name="Language", | ||
| value=",".join(value) if isinstance(value, list | set) else value, |
There was a problem hiding this comment.
Why are we not storing original in this case?
Fix #205
This is a full rewrite of #217, so I've opened a new PR since changes since last review made no more sense from my PoV.
zim.metadata.check_metadata_conventionszim.creator.Creator.config_metadataby using these types and been more strict:StandardMetadataclass for standard metadata, including list of mandatory oneX-prefixfail_on_missing_prefixargumentadd_metadata, use same metadata typeszim.creator.Creator.startwith new types, and drop all metadata from memory after being passed to the libzimzim.creator.convert_and_check_metadata(not usefull anymore, simply use proper metadata type)MANDATORY_ZIM_METADATA_KEYSandDEFAULT_DEV_ZIM_METADATAfromconstantstozim.metadatato avoid circular dependenciesinputs.unique_valuesutility function to compute the list of uniques values from a given list, but preserving initial list order__init__ofzim.creator.Creator, renamedisable_metadata_checkstocheck_metadata_conventionsfor clarity and brevityzim.metadata.check_metadata_conventions, so if you have many creator running in parallel, they can't have different settings, last one initialized will "win"Nota:
tests/zim/test_zim_creator.pytotests/zim/test_metadata.pysince most checks are now done at metadata initialization instead of whenconfig_metadataorstartare called, but coverage is similar