Centralized metadata constraints #339

veloman-yunkan · 2023-03-22T15:46:05Z

This PR addresses #334 (comment)

Fixes #334
Fixes #336

src/metadata.cpp

codecov · 2023-03-22T16:19:51Z

Codecov Report

Patch coverage: 70.63% and project coverage change: +1.79 🎉

Comparison is base (3dd2f59) 43.79% compared to head (d32037c) 45.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
+ Coverage   43.79%   45.58%   +1.79%     
==========================================
  Files          23       26       +3     
  Lines        2393     2481      +88     
  Branches     1326     1380      +54     
==========================================
+ Hits         1048     1131      +83     
- Misses       1329     1334       +5     
  Partials       16       16

Impacted Files	Coverage Δ
src/zimcheck/checks.h	`100.00% <ø> (ø)`
src/zimwriterfs/zimwriterfs.cpp	`0.00% <0.00%> (ø)`
src/metadata.h	`50.00% <50.00%> (ø)`
src/metadata.cpp	`98.70% <98.70%> (ø)`
src/metadata_constraints.cpp	`100.00% <100.00%> (ø)`
src/zimcheck/checks.cpp	`96.56% <100.00%> (-0.01%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

veloman-yunkan · 2023-03-24T15:11:16Z

Inviting @mgautierfr @kelson42 to review the overall approach for centralizing all metadata checks in one place. Please look at the file metadata_constraints.cpp (note that it is not compiled separately, but is rather #included in metadata.cpp.

Issues noticed so far:

regex may be a convenient tool as a lowest common denominator for validating various strings, however it falls short for some types of data if we need more rigorous checks. In our case, validating the date with a simple regex will allow entering invalid dates (e.g. 30th of February). This can be fixed by changing the last field in the metadata info table from a regexp to a special polymorphic validator object - a concrete implementation of thereof can be created from a regular expression. Do we want to do it now?
complex checks involving dependencies between different metadata entries (e.g. the long description, if present, should be longer than the short description; the text of description metadata should be in the language matching the value of the Language metadata, etc). A draft implementation for such checks is provided in the second commit.

kelson42 · 2023-03-25T15:01:17Z

regex may be a convenient tool as a lowest common denominator for validating various strings, however it falls short for some types of data if we need more rigorous checks. In our case, validating the date with a simple regex will allow entering invalid dates (e.g. 30th of February). This can be fixed by changing the last field in the metadata info table from a regexp to a special polymorphic validator object - a concrete implementation of thereof can be created from a regular expression. Do we want to do it now?

@veloman-yunkan Thank you very much for your PR which is, at a first sight, really much the kind of code I was expected.

Indeed regexs fall short in many cases. The case I have in mind is checking illustration entries (requires libpng).

I'm not in favour of extending much the validation scenarios because this can go endlessly and my goal is to have the basic ones.

But I would appreciate to have the appropriate code infrastructure so we can easily do that work later... without rearchitecturing effort.

kelson42 · 2023-03-26T10:55:33Z

On my local computer, it does not compile:

FAILED: test/metadata-test.p/.._src_metadata.cpp.o 
ccache c++ -Itest/metadata-test.p -Itest -I../test -I/usr/local/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Werror -std=c++11 -g -Werror -Wall '-DVERSION="3.1.3"' -DGTEST_HAS_PTHREAD=1 -pthread -MD -MQ test/metadata-test.p/.._src_metadata.cpp.o -MF test/metadata-test.p/.._src_metadata.cpp.o.d -o test/metadata-test.p/.._src_metadata.cpp.o -c ../src/metadata.cpp
../src/metadata.cpp: In static member function ‘static const zim::Metadata::ReservedMetadataRecord& zim::Metadata::getReservedMetadataRecord(const string&)’:
../src/metadata.cpp:100:14: error: ‘out_of_range’ is not a member of ‘std’
  100 |   throw std::out_of_range(name + " is not a reserved metadata name");
      |              ^~~~~~~~~~~~
../src/metadata.cpp:101:1: error: control reaches end of non-void function [-Werror=return-type]
  101 | }
      | ^
cc1plus: all warnings being treated as errors
[41/48] Compiling C++ object test/tools-test.p/.._src_zimwriterfs_zimcreatorfs.cpp.o
ninja: build stopped: subcommand failed.

rgaudin · 2023-03-27T11:04:41Z

I have mixed feelings about this. On one hand, this mainly highlights the shortcomings of such an approach but on the other hand, simple checks are better than none.

Couple of comments (already identified):

I don't think a language regexp makes sense. If you want to ensure validity, you can only work of a valid list IMO. Making sure that it's 3 lowercase letters (or proper list of such) would already be beneficial.
date regexp wont capture dates such as 2023-13-01. Going the extra mile (as suggested) would not guarantee a valid date with cases like Feb 29th… that would require a date dependency
min length for all those string metadata looks problematic. Max length as well as long as the spec is silent about it. We could raise a warning above a certain threshold though.
I am not a fan of the “LongDescription shouldn't be shorter than Description” because that's not in the spec.
“Description must be in the language of the ZIM file” is an interesting than deserves a discussion and decision. I am in favor of updating the spec on this to specify whatever we decide.
Illustrations are not checked despite it being the ticket's moto.

Once again we fall short on setting clear goals for our tools. zimcheck's description is “zimcheck checks the quality of a ZIM file.”. Does that mean that whenever zimcheck doesn't report an issue, the ZIM is guaranteed to be valid?

I join @kelson42 in thinking we want basic checks for now that we could extend in the future.

Simple date format regex
Simple format regexp for language
magic number check for Illustrations
len >= 1 for mandatory texts
len <= max for bounded texts

And that's it. The rest can be discussed and extended in separate tickets, raised by actual needs.

Although it serves a different purpose, scraperlib now (not being used yet) enforces correct metadata with more elaborate checks (actual language code, proper PNG with correct sizes, etc) so most of what we produce shall be valid in this regard.

mgautierfr · 2023-03-27T15:41:49Z

I have open a issue #340 to discuss what to test and how. Please continue the discussion there and keep this discussion for PR review.

kelson42 · 2023-03-27T15:44:33Z

I just want to emphasis again that we need this basic metadata feature pretty quickly. Thank you for having open side tickets to continue the interesting discussion and keep this PR "simple" without "insulting" the future.

kelson42 · 2023-03-27T19:52:10Z

@veloman-yunkan I hope you got all the feedbacks needed, let us know othwerwise.

veloman-yunkan · 2023-03-28T13:53:28Z

With illustration metadata in the mix, in what units min and max length values should be specified - bytes or (unicode) characters?

Most metadata being textual, it's better to specify the metadata size limits in characters and not impose any size constraints on illustration metadata.
1.5 Like above, but handle illustration metadata as a special case in the C++ code and interpret its size limits in bytes.
Specify size limits in bytes. Then languages with non-latin scripts will be penalized with lower human-comprehensible upper limits on the length of the description metadata.

rgaudin · 2023-03-28T14:04:14Z

With illustration metadata in the mix, in what units min and max length values should be specified - bytes or (unicode) characters?

We'd only check the text length already in the spec for the couple that have limits. Illustration have no such constraint in the spec so we'd leave them out for now.

veloman-yunkan · 2023-03-28T15:24:57Z

A preliminary version of this PR (reflecting feedback received on the draft version) is ready. Note that the PR doesn't utilize the new Metadata utility in any of the zim tools (zimcheck, zimwriterfs).

kelson42 · 2023-04-02T11:27:57Z

@veloman-yunkan Thank you, just to ensure we understand us well, this is mandatory that this is used at least in zimcheck.

kelson42 · 2023-04-05T15:03:33Z

@mgautierfr any feedback/review?

mgautierfr

Seems globally good to me.
A small comment on the static pointer thing but the global structure is good.

How do you plan to implement checkTextLanguage ?
Or it is just a sample and it have to be removed before merge ?

src/metadata.cpp

veloman-yunkan · 2023-04-19T13:37:12Z

How do you plan to implement checkTextLanguage ?
Or it is just a sample and it have to be removed before merge ?

In this PR checkTextLanguage() was introduced only to illustrate support for complex constraints. I will get rid of it before merging.

mgautierfr · 2023-04-19T13:41:15Z

In this PR checkTextLanguage() was introduced only to illustrate support for complex constraints. I will get rid of it before merging.

Ok. You can remove it right now (or comment it to not loose the illustration purpose) while you fix the issue with last pointer.

veloman-yunkan · 2023-04-19T15:41:11Z

@veloman-yunkan Thank you, just to ensure we understand us well, this is mandatory that this is used at least in zimcheck.

I added usage of this new utility in zimwriterfs. As a result a few issues (including two preexisting bugs related to the --longDescription option of zimwriterfs) were uncovered during testing. I will finalize this PR tomorrow.

veloman-yunkan · 2023-04-25T13:08:35Z

Note that metadata constraints were relaxed in order to let the zimcheck
unittests pass without updating/regenerating the unittest ZIM data.

How complex it is to regenerate the unittest data ?

The regeneration itself should be easy. The question is whether I should use the current release of zimwriterfs or the one used to create the current version of test ZIM files.

mgautierfr · 2023-04-25T15:22:23Z

The regeneration itself should be easy. The question is whether I should use the current release of zimwriterfs or the one used to create the current version of test ZIM files.

I think here this is not relevant (at least for the change we want to made).
But the question is not easy to answer for generic case. The solution is probably in creating a full test suite with zim files created/tested with different version (https://github.com/openzim/zim-testing-suite)

veloman-yunkan · 2023-04-26T08:57:07Z

The PR is final (though needs at least a rebase&fixup unless other issues are spotted).

Known issues:

Regexp constraints can duplicate length constraints. For example for Language metadata the following pair of errors is reported when the language is specified using a 2-char code:
- Language must contain at least 3 characters
- Language doesn't match regex: \w{3}(,\w{3})*
Regexp errors are not user-friendly. A solution to this issue is to provide a human comprehensible description of the constraint along the regexp. This approach smoothly extends to the proposed more general constraint framework with polymorphic validators (with regexps being only one of implementations).

kelson42 · 2023-04-26T08:59:25Z

@veloman-yunkan For now, I'm happy with this kind of regex error.

mgautierfr · 2023-04-26T13:10:56Z

Please rebase-fixup and we can merge.

This commit is intended to demonstrate how complex checks (involving more than one metadata entries) can be added if required.

When -L|--longDesciption option was added to zimwriterfs its argument remained unused (participating in constraint checks doesn't count).

-L option of zimwriterfs was not properly registered in the call to `getopt_long()`

For some reason the PNG regex check fails on real data. But at least it uncovered the issue with the error message which reads as bellow: ``` Illustration_48x48@1 doesn't match regex: ^�PNG � .+ ```

Regexp is not the best tool for use with binary data where NUL characters may occur.

Note that metadata constraints were relaxed in order to let the zimcheck unittests pass without updating/regenerating the unittest ZIM data.

Test ZIM files (good.zim, poor.zim and bad_checksum.zim) under test/data/zimfiles were regenerated using zimwriterfs v3.1.3 and the updated script create_test_zimfiles. Metadata constraints that were relaxed in the previous commit have been restored.

Before this change, simple metadata checks were performed only if there were no missing metadata entries.

veloman-yunkan · 2023-04-26T14:30:47Z

Please rebase-fixup and we can merge.

Done

mgautierfr · 2023-04-26T14:41:59Z

Regexp constraints can duplicate length constraints. For example for Language metadata the following pair of errors is reported when the language is specified using a 2-char code:

Language must contain at least 3 characters

Language doesn't match regex: \w{3}(,\w{3})*

I think this is a problem of coherence between checks and it can be fixed later (and we probably have to update the checks). For now the structure is good and we can merge this long PR.

holta reviewed Mar 22, 2023

View reviewed changes

src/metadata.cpp Outdated Show resolved Hide resolved

veloman-yunkan force-pushed the metadata_table branch from a9eaa76 to 59d7830 Compare March 24, 2023 14:51

kelson42 requested review from kelson42, rgaudin and mgautierfr March 25, 2023 14:47

This comment was marked as off-topic.

Sign in to view

mgautierfr mentioned this pull request Mar 27, 2023

What to test with zimcheck and how ? #340

Open

veloman-yunkan force-pushed the metadata_table branch 4 times, most recently from 04d2355 to ecbd62c Compare March 28, 2023 15:17

veloman-yunkan changed the title ~~Centralized metadata table~~ Centralized metadata constraints Mar 28, 2023

veloman-yunkan marked this pull request as ready for review March 28, 2023 15:19

mgautierfr requested changes Apr 18, 2023

View reviewed changes

src/metadata.cpp Show resolved Hide resolved

veloman-yunkan requested a review from mgautierfr April 26, 2023 08:57

mgautierfr approved these changes Apr 26, 2023

View reviewed changes

veloman-yunkan added 16 commits April 26, 2023 18:20

Introduced zim::Metadata

5b57e21

Metadata length is measured in characters

7e4249d

Added support for complex metadata checks

63d79e4

This commit is intended to demonstrate how complex checks (involving more than one metadata entries) can be added if required.

Removed an unimplemented demo check

7276c23

zimwriterfs sets LongDescription metadata

6705eb7

When -L|--longDesciption option was added to zimwriterfs its argument remained unused (participating in constraint checks doesn't count).

Fixed "zimwriterfs -L"

25a33ed

-L option of zimwriterfs was not properly registered in the call to `getopt_long()`

Using zim::Metadata in zimwriterfs

1441439

For some reason the PNG regex check fails on real data. But at least it uncovered the issue with the error message which reads as bellow: ``` Illustration_48x48@1 doesn't match regex: ^�PNG � .+ ```

Testing of PNG regex failure

d296cae

Fixed non-printable chars in error messages

5cc17ce

Fixed PNG regexp

012ec00

Regexp is not the best tool for use with binary data where NUL characters may occur.

Preparing zimcheck for switching to zim::Metadata

da72df7

Used zim::Metadata in zimcheck

28b17bb

Note that metadata constraints were relaxed in order to let the zimcheck unittests pass without updating/regenerating the unittest ZIM data.

Simple metadata checks are run unconditionally

e4766ca

Before this change, simple metadata checks were performed only if there were no missing metadata entries.

A fake PNG is created via a named function

e567139

Another test for complex constraints on metadata

d32037c

veloman-yunkan force-pushed the metadata_table branch from 0f79d9d to d32037c Compare April 26, 2023 14:28

mgautierfr merged commit 54ba91f into main Apr 26, 2023

mgautierfr deleted the metadata_table branch April 26, 2023 14:42

kelson42 mentioned this pull request Apr 26, 2023

Fails to build on PPA for arm64 #345

Closed

veloman-yunkan mentioned this pull request Apr 27, 2023

Consider moving zim::Metadata into libzim openzim/libzim#785

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralized metadata constraints #339

Centralized metadata constraints #339

veloman-yunkan commented Mar 22, 2023 •

edited by kelson42

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

veloman-yunkan commented Mar 24, 2023

kelson42 commented Mar 25, 2023

kelson42 commented Mar 26, 2023

rgaudin commented Mar 27, 2023

This comment was marked as off-topic.

mgautierfr commented Mar 27, 2023

kelson42 commented Mar 27, 2023 •

edited

Loading

kelson42 commented Mar 27, 2023

veloman-yunkan commented Mar 28, 2023

rgaudin commented Mar 28, 2023

veloman-yunkan commented Mar 28, 2023

kelson42 commented Apr 2, 2023

kelson42 commented Apr 5, 2023

mgautierfr left a comment

veloman-yunkan commented Apr 19, 2023

mgautierfr commented Apr 19, 2023

veloman-yunkan commented Apr 19, 2023 •

edited

Loading

veloman-yunkan commented Apr 25, 2023

mgautierfr commented Apr 25, 2023

veloman-yunkan commented Apr 26, 2023

kelson42 commented Apr 26, 2023

mgautierfr commented Apr 26, 2023

veloman-yunkan commented Apr 26, 2023 •

edited

Loading

mgautierfr commented Apr 26, 2023

Centralized metadata constraints #339

Centralized metadata constraints #339

Conversation

veloman-yunkan commented Mar 22, 2023 • edited by kelson42 Loading

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

veloman-yunkan commented Mar 24, 2023

kelson42 commented Mar 25, 2023

kelson42 commented Mar 26, 2023

rgaudin commented Mar 27, 2023

This comment was marked as off-topic.

mgautierfr commented Mar 27, 2023

kelson42 commented Mar 27, 2023 • edited Loading

kelson42 commented Mar 27, 2023

veloman-yunkan commented Mar 28, 2023

rgaudin commented Mar 28, 2023

veloman-yunkan commented Mar 28, 2023

kelson42 commented Apr 2, 2023

kelson42 commented Apr 5, 2023

mgautierfr left a comment

Choose a reason for hiding this comment

veloman-yunkan commented Apr 19, 2023

mgautierfr commented Apr 19, 2023

veloman-yunkan commented Apr 19, 2023 • edited Loading

veloman-yunkan commented Apr 25, 2023

mgautierfr commented Apr 25, 2023

veloman-yunkan commented Apr 26, 2023

kelson42 commented Apr 26, 2023

mgautierfr commented Apr 26, 2023

veloman-yunkan commented Apr 26, 2023 • edited Loading

mgautierfr commented Apr 26, 2023

veloman-yunkan commented Mar 22, 2023 •

edited by kelson42

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

kelson42 commented Mar 27, 2023 •

edited

Loading

veloman-yunkan commented Apr 19, 2023 •

edited

Loading

veloman-yunkan commented Apr 26, 2023 •

edited

Loading