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
Add validation for custom tag #452
Conversation
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.
Some comments.
It would actually be super nice if you could also add some basic tests for both patches (see for example test_validator_overrides in tests/test-validate.c).
For this particular part of AppStream I don't consider this to be critical, but having tests for new features would certainly be nice, to ensure they continue to work :-)
| @@ -921,6 +921,26 @@ AsValidatorIssueTag as_validator_issue_tag_list[] = { | |||
| N_("This color is not a valid HTML color code."), | |||
| }, | |||
|
|
|||
| { "custom-invalid-tag", | |||
| AS_ISSUE_SEVERITY_ERROR, | |||
| N_("The custom tags can only contains value tags"), | |||
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.
Again, any literal should be in backticks so translators don't translate it by accident.
|
|
||
| { "custom-key-missing", | ||
| AS_ISSUE_SEVERITY_ERROR, | ||
| N_("This custom tag is missing the 'key attribute"), |
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.
=> the `key` attribute
| }, | ||
|
|
||
| { "custom-value-missing", | ||
| AS_ISSUE_SEVERITY_ERROR, |
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.
Unless this causes problems, making this a WARNING or possibly even INFO might be better. That allows some flexibility for writers of custom data (e.g. to have placeholders that are ignored deliberately).
| as_validator_check_custom (AsValidator *validator, xmlNode *node) | ||
| { | ||
| g_autoptr(GHashTable) known_keys = NULL; | ||
| known_keys = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); |
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.
nit: This could be one line with the one above it.
| continue; | ||
|
|
||
| g_autofree gchar *node_content = NULL; | ||
| const gchar *node_name; |
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.
Please don't mix declarations and statements.
| } | ||
|
|
||
| gchar *key_name = NULL; | ||
| key_name = as_xml_get_prop_value (iter, "key"); |
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.
This is missing a g_autofree
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.
Using this here, causes a crash when added to the table. Probably because it is out of scope.
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.
You need to change the g_hash_table_add line to g_hash_table_add (known_keys, g_steal_pointer (&key_name)); as well for this to work without a crash.
| else | ||
| g_hash_table_add (known_keys, key_name); | ||
|
|
||
| node_content = (gchar*) xmlNodeGetContent (iter); |
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.
Please use either as_xml_get_node_value or as_xml_get_node_value_raw.
You probably want the former and then check for NULL.
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.
This unfortunately didn't work
node_content = as_xml_get_node_value (iter);
if (node_content == NULL) {
as_validator_add_issue (validator, iter, "custom-value-missing", NULL);
}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.
What do you mean it did not work?
This code should be fine:
node_content = as_xml_get_node_value (iter);
if (node_content == NULL || node_content[0] == '\0')
as_validator_add_issue (validator, iter, "custom-value-missing", NULL);|
There will be a release today/tomorrow, would be nice to get this into the last release of this year :-) |
| } | ||
|
|
||
| gchar *key_name = NULL; | ||
| key_name = as_xml_get_prop_value (iter, "key"); |
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.
You need to change the g_hash_table_add line to g_hash_table_add (known_keys, g_steal_pointer (&key_name)); as well for this to work without a crash.
| else | ||
| g_hash_table_add (known_keys, key_name); | ||
|
|
||
| node_content = (gchar*) xmlNodeGetContent (iter); |
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.
What do you mean it did not work?
This code should be fine:
node_content = as_xml_get_node_value (iter);
if (node_content == NULL || node_content[0] == '\0')
as_validator_add_issue (validator, iter, "custom-value-missing", NULL);|
Since the release is imminent, I just addressed the issues for you. Kind of an early christmas present ^^ |
|
This patch has seemingly caused applications using the |
|
Yes, yikes... The way the keys re used there simply will not work, you can't have two identical keys and assume the values are collected in a list. Maybe simply go the more modern route and set a supported minium display size: https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-relations-display_length |
No description provided.