Migrate Perl validation checks into netkan.exe

[HebaruSan]

Motivation

The pull request validation scripts apply a bunch of validation to .netkan and .ckan files that aren't implemented anywhere else.

https://github.com/KSP-CKAN/xKAN-meta_testing/blob/7b875488601288bdc59520b5d7ff59b6f9d1c75a/NetKAN/t/metadata.t

This can come as a surprise if a mod author prepares a netkan and tests it with netkan.exe, only to find that there were several problems with it. This is especially common with licenses and spec versions.

Specific checks

Netkans (before processing):

• Check that identifier matches netkan filename
• Check that identifier matches ^[A-Za-z0-9-]+$
• any_of requires spec_version v1.26
• any_of identifiers match ^[A-Za-z0-9-]+$
• Identifiers in relationships match ^[A-Za-z0-9-]+$
• $kref mutually exclusive with download property
• Version required when download is set
• x_netkan_override is an array
• x_netkan_override[x].version is set
• x_netkan_override[x].delete or .override is set
• spec_version matches ^1$|^v\d.\d\d?$
• Licenses are in list of known licenses (if not metanetkan and x_netkan_license_ok not set)
• License WTFPL requires spec_version v1.2
• License Unlicense requires spec_version v1.18
• ksp_version_strict requires spec_version v1.16
• replaced_by requires spec_version v1.26
• install_to: GameData/something requires spec_version v1.2
• install_to: Ships/something requires spec_version v1.12
• install_to: Ships/@thumbs requires spec_version v1.16
• find requires spec_version v1.4
• find_regexp requires spec_version v1.10
• find_matches_files requires spec_version v1.16
• as requires spec_version v1.18

Ckans (after processing):

• Matches at least one known game version
• Obeys CKAN.schema

Changes

Netkan.exe already has a validation framework, but there aren't many checks.
Now all of the above are added. This will allow mod authors to find out about problems before they submit pull requests. In time, we may be able to retire the Perl checks, which would simplify the pull request scripts.
Each validator is exercised by a small suite of tests.

For the schema check:

• CKAN.schema is now an embedded resource in netkan.exe
• The schema class built into core Newtonsoft.Json only supports v3 syntax while CKAN.schema is v4, but NJsonSchema supports v4, so that reference is added (can't use the Newtonsoft.Json.Schema assembly because it would require CKAN's license to change)

[techman83]

This is awesome! The validation scripts were born before NetKAN and when CKAN-meta was routinely submitted to directly and not via NetKAN.

In theory we could use NetKAN as the validator, ie netkan.exe --validate-ckan AwesomeMod.ckan

[Olympic1]

I thought we discussed in #2027 that we didn't want to use Json.NET Schema because of the license:

Well we still need to consider if we want to do this. Json.NET Schema is not permissively licensed, but is licensed under AGPL-3.0 which is copyleft. This means the combined work (ckan.exe, netkan.exe, etc.) has to be licensed under the AGPL.

[HebaruSan]

Of course there's always a license problem if I forget to check it. 😬
Switched to NJsonSchema as we did in that PR.

[HebaruSan]

Added tests for each validator.

[DasSkelett]

What about this test (have either kref or vref or download):
https://github.com/KSP-CKAN/xKAN-meta_testing/blob/master/NetKAN/t/metadata.t#L87-L92
Can't find it in the existing netkan checks, nor in your additions.

[HebaruSan]

Good catch! I missed that one. Will add...
Also that test is wrong-ish, a $vref won't help you with generating a download property.

[DasSkelett]

a $vref won't help you with generating a download property.

Isn't there a download property available in AVC .version files? Do we parse them? Or are they just used as general links like https://github.com/KSP-CKAN/CKAN/releases and not pointing to a specific file like https://github.com/KSP-CKAN/CKAN/releases/download/v1.26.2/ckan.exe?

[HebaruSan]

See #2517; that's handled by putting the AVC URL in a $kref, not a $vref. If you have no download property, then you can't download the file in order to parse its .version file to find the DOWNLOAD property.

[DasSkelett]

Looks good now.
I did a few test runs, in which everything worked.
I didn't really review all the unit tests, but they pass, so let's hope they pass out of the right reasons.

[HebaruSan]

Well that's fun:

@techman83, if you have a chance, could you please upload the bot's ~/.mono/registry/CurrentUser/software/ckan/values.xml file somewhere so we can take a look at it? I suspect its cached copy of builds.json is out of date, and we'll need to figure out a strategy for refreshing it. (Or favoring the embedded build map, since that'll be up to date for netkan.)

[HebaruSan]

More or less confirmed, the modules that failed with that error all have a KSP version min of 1.5.1 or later, while the ones that succeeded all include 1.4.1 or earlier. So the bot has a cached copy of builds.json from approximately October 2018.

[techman83]

Ok I uploaded the Gist here. But you're likely quite right.

[HebaruSan]

Yup, it goes up to 1.4.5. So that's from between KSP-CKAN/CKAN-meta@686ef27 (Jul 28, 2018) and KSP-CKAN/CKAN-meta@9947336 (Oct 15, 2018).

[techman83]

Good detective work 🙂

[DasSkelett]

One thing I just discovered:
In HasIdentifierValidator and in KrefValidator there are log statements:

512 [1] INFO CKAN.NetKAN.Validators.HasIdentifierValidator (null) - Validating that metadata contains an identifier
513 [1] INFO CKAN.NetKAN.Validators.KrefValidator (null) - Validating that metadata contains valid or null $kref

But not in other validators.
We should keep that unified, either everywhere logging or nowhere (my favourite) with just a Validating netkan file at the beginning of netkan validations, and Validating ckan file for the ckan validations.

[DasSkelett]

The mods Graphotron, HyperEdit and Kethane are failing with Object must implement IConvertible.. Great error message. Took about 1 1/2 hours to track down the exact line (why doesn't mono support Debugger.Launch()?!?).
It's the schema validation:

CKAN/Netkan/Validators/ObeysCKANSchemaValidator.cs

Lines 21 to 31 in b3da232

	public void Validate(Metadata metadata)
	{
	var errors = schema.Validate(metadata.Json());
	if (errors.Any())
	{
	string msg = errors
	.Select(err => $"{err.Path}: {err.Kind}")
	.Aggregate((a, b) => $"{a}\r\n{b}");
	throw new Kraken($"Schema validation failed: {msg}");
	}
	}

Line 23 is failing exactly, put a log statement before and one after and there you go.
Is that a bug on NJsonSchemas side?

Edit: I see we are passing a JObject, but JsonSchema.Validate() requests a JToken.

[HebaruSan]

JToken is a base class of JObject (and JArray), so it's fine to do that:

• https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Linq_JObject.htm
• https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Linq_JArray.htm

[HebaruSan]

Here's the complete stack trace (which you can view by changing throw e to throw in Inflator.Inflate):

3470 [1] FATAL CKAN.NetKAN.Program (null) - Object must implement IConvertible.
3471 [1] FATAL CKAN.NetKAN.Program (null) -   at System.Convert.ChangeType (System.Object value, System.Type conversionType, System.IFormatProvider provider) <0x7f7b13af7110 + 0x004d1> in <6649516e5b3542319fb262b421af0adb>:0 
  at Newtonsoft.Json.Linq.Extensions.Convert[T,U] (T token) [0x000f7] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at Newtonsoft.Json.Linq.Extensions.Value[T,U] (System.Collections.Generic.IEnumerable`1[T] value) [0x0000b] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at Newtonsoft.Json.Linq.Extensions.Value[U] (System.Collections.Generic.IEnumerable`1[T] value) [0x00000] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.Validation.JsonSchemaValidator.ValidateString (Newtonsoft.Json.Linq.JToken token, NJsonSchema.JsonSchema schema, NJsonSchema.JsonObjectType type, System.String propertyName, System.String propertyPath, System.Collections.Generic.List`1[T] errors) [0x00043] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.Validation.JsonSchemaValidator.ValidateType (Newtonsoft.Json.Linq.JToken token, NJsonSchema.JsonSchema schema, System.String propertyName, System.String propertyPath, System.Collections.Generic.List`1[T] errors) [0x001b4] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.Validation.JsonSchemaValidator.Validate (Newtonsoft.Json.Linq.JToken token, NJsonSchema.JsonSchema schema, System.String propertyName, System.String propertyPath) [0x00036] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.Validation.JsonSchemaValidator.ValidateProperties (Newtonsoft.Json.Linq.JToken token, NJsonSchema.JsonSchema schema, System.String propertyName, System.String propertyPath, System.Collections.Generic.List`1[T] errors) [0x00097] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.Validation.JsonSchemaValidator.Validate (Newtonsoft.Json.Linq.JToken token, NJsonSchema.JsonSchema schema, System.String propertyName, System.String propertyPath) [0x0004e] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.Validation.JsonSchemaValidator.Validate (Newtonsoft.Json.Linq.JToken token, NJsonSchema.JsonSchema schema) [0x0000f] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at NJsonSchema.JsonSchema.Validate (Newtonsoft.Json.Linq.JToken token) [0x0000c] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at CKAN.NetKAN.Validators.ObeysCKANSchemaValidator.Validate (CKAN.NetKAN.Model.Metadata metadata) [0x0001e] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at CKAN.NetKAN.Validators.CkanValidator.Validate (CKAN.NetKAN.Model.Metadata metadata) [0x00019] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at CKAN.NetKAN.Validators.CkanValidator.ValidateCkan (CKAN.NetKAN.Model.Metadata metadata, CKAN.NetKAN.Model.Metadata netkan) [0x00001] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at CKAN.NetKAN.Processors.Inflator.Inflate (System.String filename, CKAN.NetKAN.Model.Metadata netkan, System.Nullable`1[T] releases) [0x000bf] in <840a99bb54b8423b94f0716f18cfa664>:0 
  at CKAN.NetKAN.Program.Main (System.String[] args) [0x00150] in <840a99bb54b8423b94f0716f18cfa664>:0 

So yes, it's a bug in NJsonSchema (or Newtonsoft). RicoSuter/NJsonSchema#568 mentions problems with BigInts, but I don't think we have that here.

[HebaruSan]

I take that back. Looking for commonalities among HyperEdit, Graphotron, and Kethane, they are 3 of the 5 $krefs using #/ckan/http/ (SurveyTransponder and TalisarParts being the others, each last checked by the bot 21 hours ago). HttpTransformer is assigning a Uri object into the JToken structure:

CKAN/Netkan/Transformers/HttpTransformer.cs

Lines 28 to 34 in 1ac6261

	var resolvedUri = Net.ResolveRedirect(new Uri(metadata.Kref.Id));
	Log.InfoFormat("URL {0} resolved to {1}", metadata.Kref.Id, resolvedUri);
	if (resolvedUri != null)
	{
	json["download"] = resolvedUri;

CKAN/Core/Net/Net.cs

Line 263 in 1ac6261

public static Uri ResolveRedirect(Uri uri)

Presumably this confuses NJsonSchema. So it's our fault and easy to fix.

[DasSkelett]

Here's the complete stack trace (which you can view by changing throw e to throw in Inflator.Inflate):

Could've needed that before ;)
Also, is there a way to print the exception type too, not only the message?

I've put the json output of --debug runs irght before the error throws into this gist (concatenated as another json file, for easier handling/formatting).
Tried to see some similarities, but other than that they all start with an http kref which isn't even present any more at this point, I can't spot anything.
gist deleted, not needed anymore

[DasSkelett]

Oh common, you were like 2 seconds faster :D