Skip to content
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

Jump to bottom

Add validation for custom tag #452

Merged
merged 2 commits into from Dec 22, 2022
Merged

Add validation for custom tag #452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fff73b
Add validation for custom tag
JakobDev Dec 13, 2022
9362c1e
Fix typo
JakobDev Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 src/as-validator-issue-tag.h
View file
Open in desktop
@@ -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"),
Copy link
Owner

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"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> the `key` attribute

},

{ "custom-key-duplicated",
AS_ISSUE_SEVERITY_ERROR,
N_("A key can only be used once"),
},

{ "custom-value-missing",
AS_ISSUE_SEVERITY_ERROR,
Copy link
Owner

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).

N_("This custom tag is missing a value"),
},

{ "metainfo-localized-keywords-tag",
AS_ISSUE_SEVERITY_ERROR,
N_("A <keywords/> tag must not be localized in metainfo files (upstream metadata). "
45 src/as-validator.c
View file
Open in desktop
@@ -1924,6 +1924,49 @@ as_validator_check_branding (AsValidator *validator, xmlNode *node)
}
}

/**
* as_validator_check_custom:
**/
static void
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);
Copy link
Owner

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.


for (xmlNode *iter = node->children; iter != NULL; iter = iter->next) {
if (iter->type != XML_ELEMENT_NODE)
continue;

g_autofree gchar *node_content = NULL;
const gchar *node_name;
Copy link
Owner

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.


node_name = (const gchar*) iter->name;

if (g_strcmp0 (node_name, "value") != 0) {
as_validator_add_issue (validator, iter, "custom-invalid-tag", node_name);
continue;
}

gchar *key_name = NULL;
key_name = as_xml_get_prop_value (iter, "key");
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Owner

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.


if (key_name == NULL) {
as_validator_add_issue (validator, iter, "custom-key-missing", NULL);
continue;
}

if (g_hash_table_contains (known_keys, key_name))
as_validator_add_issue (validator, iter, "custom-key-duplicated", key_name);
else
g_hash_table_add (known_keys, key_name);

node_content = (gchar*) xmlNodeGetContent (iter);
Copy link
Owner

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.

Copy link
Contributor Author

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);
}

Copy link
Owner

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);

if (strlen(node_content) == 0) {
as_validator_add_issue (validator, iter, "custom-value-missing", NULL);
}
}
}

/**
* as_validator_validate_component_node:
**/
@@ -2244,7 +2287,7 @@ as_validator_validate_component_node (AsValidator *validator, AsContext *ctx, xm
as_validator_check_appear_once (validator, iter, found_tags, FALSE);
} else if (g_strcmp0 (node_name, "custom") == 0) {
as_validator_check_appear_once (validator, iter, found_tags, FALSE);
as_validator_check_children_quick (validator, iter, "value", FALSE);
as_validator_check_custom (validator, iter);
} else if ((g_strcmp0 (node_name, "metadata") == 0) || (g_strcmp0 (node_name, "kudos") == 0)) {
/* these tags are GNOME / Fedora specific extensions and are therefore quite common. They shouldn't make the validation fail,
* especially if we might standardize at leat the <kudos/> tag one day, but we should still complain about those tags to make