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

Wrong/Undescriptive error message from NetKAN on invalid $kref #1621

Closed
602p opened this issue Feb 28, 2016 · 10 comments
Closed

Wrong/Undescriptive error message from NetKAN on invalid $kref #1621

602p opened this issue Feb 28, 2016 · 10 comments
Labels
Netkan Issues affecting the netkan data

Comments

@602p
Copy link
Contributor

602p commented Feb 28, 2016

I'm not sure if this is where I should be reporting an issue with NetKAN, but it seemed most logical considering it's source is in this repo now.

CKAN Version: v1.16.0-0-g7206ce5 (beta)
NetKan Version: v1.16.0-63-g254ef2b (beta)

Operating System: Windows 7 Home Premium

The issue you are experiencing: Incorrect error message on invalid $kref entry in netkan file

How to recreate this issue: Paste this into a .netkan file, run ./netkan --version , note that since the file contains a invalid $kref (spaceport instead of spacedock) it will fail. However it confused me at least that this was reported as 665 [1] FATAL CKAN.NetKAN.Program (null) - JSON deserialization error instead of a invalid kref.

CKAN error codes (if applicable): 665 [1] FATAL CKAN.NetKAN.Program (null) - JSON deserialization error

Perhaps a more useful error message would be good for someone as prone to typos as me :P

@Olympic1 Olympic1 added the Netkan Issues affecting the netkan data label Feb 28, 2016
@politas
Copy link
Member

politas commented Feb 28, 2016

Given that it doesn't work as a spacedock $kref, either, we can't be sure what the error is.
But yeah, a lot of the error messages you get out of netkan are inscrutable.

@602p
Copy link
Contributor Author

602p commented Feb 28, 2016

Whoops. Here's a better test case: http://pastebin.com/raw/8WweaaJX

I deleted the mod I was using to test this there. It's now at ID 307, and the updated (correct) netkan has the same issue

@politas
Copy link
Member

politas commented Feb 28, 2016

I think the best answer would be for us to remove the spaceport transformer. That should make it throw a more meaningful error.

@politas
Copy link
Member

politas commented Feb 28, 2016

Ok, so that's the error you get if you put any random incorrect string in that place. @techman83 , any chance we could get a test for known #krefs so we can catch this with a meaningful error message?

@techman83
Copy link
Member

@politas do you mean during inflation or jenkins? I'm guessing the end point doesn't respond with JSON when you try and request a #kfref that doesn't exist causing the JSON lib to barf out.

If it's in the netkan code it probably could check for a valid #kref, but my C# is pretty limited.

@politas
Copy link
Member

politas commented Feb 29, 2016

I was thinking it should be part of netkan's pre-validation, but if Jenkins could check for it and give an error, that would be useful, too. JSON deserialisation errors are staggeringly vague. @plague006 tells me that @dbent is the person to ping for Netkan changes, anyway, so sorry to pull you into the discussion.

@dbent
Copy link
Member

dbent commented Feb 29, 2016

Sure, all error reporting and logging is due for an overhaul in NetKAN. It'll happen Eventually™.

@602p
Copy link
Contributor Author

602p commented Feb 29, 2016

Mainly my confusion stemmed from normally thinking deserialization error ==
syntax error. A little confusing to get it from a validation (?) fail.

All things considered, its a very low-priority issue: if it's gonna require
significant rework I wouldn't worry about it.
On Feb 29, 2016 08:31, "Dwayne Bent" notifications@github.com wrote:

Sure, all error reporting and logging is due for an overhaul in NetKAN.
It'll happen Eventually™.


Reply to this email directly or view it on GitHub
#1621 (comment).

@pjf
Copy link
Member

pjf commented Mar 13, 2016

Mainly my confusion stemmed from normally thinking deserialization error == syntax error. A little confusing to get it from a validation (?) fail.

Yup, this caught me too. I was running jsonlint over the file to try and figure out what's going on.

As best I can tell (by running with --debug), the error itself is originating from the CkanModule.ctor, because nothing was able to handle the $kref (being "spaceport" rather than "spacedock"), but our pipeline doesn't realise that we've missed the most important step and then gets confused that we don't have a mod version.

An easy way to catch this would be either modify StripNetkanMetadataTransformer or add a step just before it that ensures that we've actually processed the $kref somehow, and provide a "unknown $kref..." style message if we haven't. A reliable way of doing this would be passing a state object between transformers (with a bool that can be flipped to true when a $kref is processed), or even some sort of x_kref_transformed_by in the metadata object itself (which could be stripped at the start and end of the pipeline).

TL;DR: We should have a way of spotting when we've reached a point in the pipeline when we expect the $kref to have been inflated, but haven't done so.

@HebaruSan
Copy link
Member

This is a duplicate of #374.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Netkan Issues affecting the netkan data
Projects
None yet
Development

No branches or pull requests

7 participants