Ckan: Netkan.exe should fail gracefully on files with an invalid $kref token

[pjf]

I thought it did, but recent reports indicate that it doesn't.

It should, however, _warn_ tell the user if it spots a file upon which it performs no expansion.

[mutability]

The current failure mode for a netkan file with no $kref is this:

Unhandled Exception:
System.ArgumentNullException: Argument cannot be null.
Parameter name: input
  at System.Text.RegularExpressions.Regex.Match (System.String input, Int32 startat) [0x00000] in <filename unknown>:0 
  at System.Text.RegularExpressions.Regex.Match (System.String input) [0x00000] in <filename unknown>:0 
  at System.Text.RegularExpressions.Regex.Match (System.String input, System.String pattern, RegexOptions options) [0x00000] in <filename unknown>:0 
  at System.Text.RegularExpressions.Regex.Match (System.String input, System.String pattern) [0x00000] in <filename unknown>:0 
  at CKAN.NetKAN.MainClass.FindRemote (Newtonsoft.Json.Linq.JObject json) [0x00000] in <filename unknown>:0 
  at CKAN.NetKAN.MainClass.Main (System.String[] args) [0x00000] in <filename unknown>:0 

[tips48]

For reference, the issue lies on these lines:
https://github.com/KSP-CKAN/CKAN/blob/master/CKAN/NetKAN/MainClass.cs#L286
https://github.com/KSP-CKAN/CKAN/blob/master/CKAN/NetKAN/MainClass.cs#L54

[Ippo343]

This issue was moved to KSP-CKAN/CKAN-netkan#4

[pjf]

re-opening after repo merge

[pjf]

This still exists, with us currently getting:

247 [1] FATAL CKAN.NetKAN.Program (null) - Object reference not set to an instance of an object

However it strikes me that if we have no $kref we can just tell the user:

There is no $kref field in {0}, so netkan expansion is not needed or possible.

Or something similar. (In short, this is just a better-error-message change.)

[plague006]

Having no $kref at all poses no issue, having a broken $kref now gives a generic JSON deserialization error

[techman83]

@plague006 would that be broken JSON? Which should be caught during testing with jsonlint I'd expect. I think this was completed during the refactor #1218 - but I'm not sure. Either way I think this can be closed now.

[plague006]

It's not broken JSON as I understand it. It validates with jsonlint without issue. Some excerpts from a test:

"$kref": "#/ckan/techman/iscool",

gives the output

348 [1] FATAL CKAN.NetKAN.Program (null) - JSON deserialization error

which is really generic and hard for users to interpret.

[ayan4m1]

My first rodeo with the NetKAN codebase. The only place I see that error message in the current codebase is https://github.com/KSP-CKAN/CKAN/blob/master/Core/Types/CkanModule.cs#L297 - having a full stack trace would be handy because that CkanModule ctor is being called somewhat frequently in my limited profiling of the current code.

[HebaruSan]

Current output with "$kref": "#/ckan/techman/iscool",:

$ ../CKAN/_build/netkan.exe NetKAN/KSP-AVC.netkan 
343 [1] ERROR CKAN.CkanModule (null) - KSP-AVC missing required field download
354 [1] FATAL CKAN.NetKAN.Program (null) - KSP-AVC missing required field download

So it doesn't stop at JSON deserialization anymore, but it waits till later in the processing to complain, when it finally notices it doesn't have any URLs to download. Ideally yes, the error would specify that the $kref is invalid.