Skip to content

#208 Error reading package.json. The file may be parseable JSON but may#459

Merged
mousetraps merged 1 commit intomicrosoft:masterfrom
mousetraps:i208
Oct 13, 2015
Merged

#208 Error reading package.json. The file may be parseable JSON but may#459
mousetraps merged 1 commit intomicrosoft:masterfrom
mousetraps:i208

Conversation

@mousetraps
Copy link
Copy Markdown
Contributor

contain objects with duplicate properties

  • fallback to a different parsing method if parsing fails
  • handle failures gracefully (that package won't be displayed in sln explorer, but the rest of the packages will be.)

fix #208

…SON but may

contain objects with duplicate properties
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this definitely safe to sometimes fallback to a different method? I see DeserializeObject and JObject.Parse both return different types (Object and JObject respectively).

Also, may the new JObject.Parse now throw different exceptions than those you are handling below? (i.e. do you need to add more handlers for any JObject.Parse failures).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is safe because DeserializeObject actually returns JObject under the covers when no explicit type is specified (JsonConvert.DeserializeObject(...).GetType().FullName returns "Newtonsoft.Json.Linq.JObject")

It's unclear why the two follow different codepaths, though. Perhaps they've fixed this issue in later versions of Newtonsoft.Json, but this workaround will have to suffice for now.

@hoanhtien
Copy link
Copy Markdown
Contributor

The catch (ArgumentException ae) {...} is now redundant. Should we remove it?

public ReaderPackageJsonSource(TextReader reader) {
    try {
        var text = reader.ReadToEnd();
        try {
            // JsonConvert and JObject.Parse exhibit slightly different behavior,
            // so fall back to JObject.Parse if JsonConvert does not properly deserialize
            // the object.
            Package = JsonConvert.DeserializeObject(text);
        } catch (ArgumentException) {
            Package = JObject.Parse(text);
        }
    } catch (JsonReaderException jre) {
        WrapExceptionAndRethrow(jre);
    } catch (JsonSerializationException jse) {
        WrapExceptionAndRethrow(jse);
    } catch (FormatException fe) {
        WrapExceptionAndRethrow(fe);
    } catch (ArgumentException ae) {
        throw new PackageJsonException(
            string.Format(@"Error reading package.json. The file may be parseable JSON but may contain objects with duplicate properties.

The following error occurred:

{0}", ae.Message),
            ae);
    }
}

We may also want to fix this test if we are no longer throwing PackageJsonException on duplicate properties.

[TestMethod, Priority(0)]
[ExpectedException(typeof(PackageJsonException))]
public void TestParseDuplicateProperty() {
    TestFreshPackage("duplicateproperty");
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants