Skip to content

Minor improvement: Check sources #3502

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.

Sign up for GitHub

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

Merged
chriseth merged 1 commit into from
Feb 19, 2018
Merged

Minor improvement: Check sources #3502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Minor improvement: check sources 
- returns error, if "sources" is an array, an empty object or not defined
- Added new test-cases in test/libsolidity/StandardCompiler.cpp
  • Loading branch information
@aarlt
aarlt committed Feb 16, 2018
commit 1d4547ab037db2f21fcb31219ae28a1d295ab270
2 changes: 1 addition & 1 deletion Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Features:


Bugfixes:

* Standard JSON: catch errors properly when invalid "sources" are passed

### 0.4.20 (2018-02-14)

Expand Down
6 changes: 5 additions & 1 deletion libsolidity/interface/StandardCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,11 @@ Json::Value StandardCompiler::compileInternal(Json::Value const& _input)
return formatFatalError("JSONError", "Only \"Solidity\" is supported as a language.");

Json::Value const& sources = _input["sources"];
if (!sources)

if (!sources.isObject() && !sources.isNull())
Copy link
Contributor

@axic axic Feb 15, 2018

Choose a reason for hiding this comment

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

Why do we need isNull here?

Is IsObject true when the value is null?

Copy link
Collaborator Author

@aarlt aarlt Feb 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@axic nope.. but null is also not an object.. so if there is no sources object at all, it is also not an object.. !sources.isObject() is also true - that's why I do the additional check here.

however, just in case you didn't noticed: I introduced a slightly different error message here: Sources input is not a JSON object. in contrast to Source input is not a JSON object. - from my point of view its nice nice to distinguish between sources is not a json object, and if items of sources - so individual members of the sources object - source is not a json object.

Copy link
Contributor

@axic axic Feb 15, 2018

Choose a reason for hiding this comment

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

so if there is no sources object at all, it is also not an object

Ah, yes that makes sense :)

return formatFatalError("JSONError", "\"sources\" is not a JSON object.");

if (sources.empty())
return formatFatalError("JSONError", "No input sources specified.");

Json::Value errors = Json::arrayValue;
Expand Down
36 changes: 36 additions & 0 deletions test/libsolidity/StandardCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,42 @@ BOOST_AUTO_TEST_CASE(no_sources)
BOOST_CHECK(containsError(result, "JSONError", "No input sources specified."));
}

BOOST_AUTO_TEST_CASE(no_sources_empty_object)
{
char const* input = R"(
{
"language": "Solidity",
"sources": {}
}
)";
Json::Value result = compile(input);
BOOST_CHECK(containsError(result, "JSONError", "No input sources specified."));
}

BOOST_AUTO_TEST_CASE(no_sources_empty_array)
{
char const* input = R"(
{
"language": "Solidity",
"sources": []
Copy link
Contributor

@axic axic Feb 13, 2018

Choose a reason for hiding this comment

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

This should result in an error stating it is an invalid input.

Copy link
Collaborator Author

@aarlt aarlt Feb 13, 2018

Choose a reason for hiding this comment

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

I think its nicely reflected with No input sources specified. - not sufficient?

Copy link
Contributor

@axic axic Feb 13, 2018

Choose a reason for hiding this comment

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

Looking at the other tests it seems we do not have any more granular reporting, so it should be fine for the moment. Can revisit in the future.

Copy link
Contributor

@axic axic Feb 13, 2018

Choose a reason for hiding this comment

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

Wait a second, [ "aaa", "bbb" ] will be accepted by the empty-ness check but what will sources.getMemberNames() do?

Copy link
Collaborator Author

@aarlt aarlt Feb 13, 2018

Choose a reason for hiding this comment

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

{
	"errors" : 
	[
		{
			"component" : "general",
			"formattedMessage" : "JSON logic exception: in Json::Value::getMemberNames(), value must be objectValue",
			"message" : "JSON logic exception: in Json::Value::getMemberNames(), value must be objectValue",
			"severity" : "error",
			"type" : "InternalCompilerError"
		}
	]
}

I guess we need another check for that, another PR?

Copy link
Contributor

@axic axic Feb 13, 2018

Choose a reason for hiding this comment

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

I'd just do it in this one.

Copy link
Collaborator Author

@aarlt aarlt Feb 14, 2018

Choose a reason for hiding this comment

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

Make sense.

}
)";
Json::Value result = compile(input);
BOOST_CHECK(containsError(result, "JSONError", "\"sources\" is not a JSON object."));
}

BOOST_AUTO_TEST_CASE(sources_is_array)
{
char const* input = R"(
{
"language": "Solidity",
"sources": ["aa", "bb"]
}
)";
Json::Value result = compile(input);
BOOST_CHECK(containsError(result, "JSONError", "\"sources\" is not a JSON object."));
}

BOOST_AUTO_TEST_CASE(smoke_test)
{
char const* input = R"(
Expand Down