Skip to content

Commit

Permalink
Add JsonObject member sanity checking
Browse files Browse the repository at this point in the history 
Verify when we destroy a JsonObject that we actually visited all the
fields of that object.  If we didn't, this suggests an error of some
kind in the input data.

Default is to report an error to the debug log, which users won't
notice, but will cause a test failure in CI.

This means we need to nearly always pass JsonObjects by non-const
reference, so that all the member visitations get registered in a single
place.

One can opt out of this check by calling allow_omitted_members on the
JsonObject.  We need to add such calls in a bunch of places that don't
visit all the members for one reason or another.

Also added various other tweaks and workarounds to prevent false error
report.

The checking has to not be turned on when building the json tooling
executables, because it leads to linking errors (the debug log doesn't
exist for them).
  • Loading branch information
@jbytheway
jbytheway committed Oct 14, 2019
1 parent d18c9b1 commit 5f9d193
Show file tree
Hide file tree
Showing 27 changed files with 240 additions and 127 deletions.
6 changes: 4 additions & 2 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ WARNINGS += -Wimplicit-fallthrough=0
endif

CXXFLAGS += $(WARNINGS) $(DEBUG) $(DEBUGSYMS) $(PROFILE) $(OTHERS) -MMD -MP
TOOL_CXXFLAGS = -DCATA_IN_TOOL

BINDIST_EXTRAS += README.md data doc
BINDIST = $(BUILD_PREFIX)cataclysmdda-$(VERSION).tar.gz
Expand Down Expand Up @@ -801,7 +802,7 @@ localization:
lang/compile_mo.sh $(LANGUAGES)

$(CHKJSON_BIN): $(CHKJSON_SOURCES)
$(CXX) $(CXXFLAGS) -Isrc/chkjson -Isrc $(CHKJSON_SOURCES) -o $(CHKJSON_BIN)
$(CXX) $(CXXFLAGS) $(TOOL_CXXFLAGS) -Isrc/chkjson -Isrc $(CHKJSON_SOURCES) -o $(CHKJSON_BIN)

json-check: $(CHKJSON_BIN)
./$(CHKJSON_BIN)
Expand Down Expand Up @@ -1040,7 +1041,8 @@ style-all-json: json_formatter
find data -name "*.json" -print0 | xargs -0 -L 1 $(JSON_FORMATTER_BIN)

json_formatter: $(JSON_FORMATTER_SOURCES)
$(CXX) $(CXXFLAGS) -Itools/format -Isrc $(JSON_FORMATTER_SOURCES) -o $(JSON_FORMATTER_BIN)
$(CXX) $(CXXFLAGS) $(TOOL_CXXFLAGS) -Itools/format -Isrc \
$(JSON_FORMATTER_SOURCES) -o $(JSON_FORMATTER_BIN)

tests: version $(BUILD_PREFIX)cataclysm.a
$(MAKE) -C tests
Expand Down
92 changes: 60 additions & 32 deletions src/assign.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,28 @@ bool assign( JsonObject &jo, const std::string &name, T &val, bool strict = fals

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;
err.allow_omitted_members();
JsonObject relative = jo.get_object( "relative" );
relative.allow_omitted_members();
JsonObject proportional = jo.get_object( "proportional" );
proportional.allow_omitted_members();

// Do not require strict parsing for relative and proportional values as rules
// such as +10% are well-formed independent of whether they affect base value
if( jo.get_object( "relative" ).read( name, out ) ) {
err = jo.get_object( "relative" );
if( relative.read( name, out ) ) {
err = relative;
strict = false;
out += val;

} else if( jo.get_object( "proportional" ).read( name, scalar ) ) {
err = jo.get_object( "proportional" );
} else if( proportional.read( name, scalar ) ) {
err = proportional;
if( scalar <= 0 || scalar == 1 ) {
err.throw_error( "invalid proportional scalar", name );
}
strict = false;
out = val * scalar;

} else if( !jo.read( name, out ) ) {

return false;
}

Expand All @@ -96,15 +100,12 @@ inline bool assign( JsonObject &jo, const std::string &name, bool &val, bool str
{
bool out;

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;

if( !jo.read( name, out ) ) {
return false;
}

if( strict && out == val ) {
report_strict_violation( err, "assignment does not update value", name );
report_strict_violation( jo, "assignment does not update value", name );
}

val = out;
Expand Down Expand Up @@ -171,8 +172,10 @@ template <typename T>
typename std::enable_if<std::is_constructible<T, std::string>::value, bool>::type assign(
JsonObject &jo, const std::string &name, std::set<T> &val, bool = false )
{
auto add = jo.get_object( "extend" );
auto del = jo.get_object( "delete" );
JsonObject add = jo.get_object( "extend" );
add.allow_omitted_members();
JsonObject del = jo.get_object( "delete" );
del.allow_omitted_members();

if( jo.has_string( name ) || jo.has_array( name ) ) {
val = jo.get_tags<T>( name );
Expand Down Expand Up @@ -239,21 +242,26 @@ inline bool assign( JsonObject &jo, const std::string &name, units::volume &val,

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;
err.allow_omitted_members();
JsonObject relative = jo.get_object( "relative" );
relative.allow_omitted_members();
JsonObject proportional = jo.get_object( "proportional" );
proportional.allow_omitted_members();

// Do not require strict parsing for relative and proportional values as rules
// such as +10% are well-formed independent of whether they affect base value
if( jo.get_object( "relative" ).has_member( name ) ) {
if( relative.has_member( name ) ) {
units::volume tmp;
err = jo.get_object( "relative" );
err = relative;
if( !parse( err, tmp ) ) {
err.throw_error( "invalid relative value specified", name );
}
strict = false;
out = val + tmp;

} else if( jo.get_object( "proportional" ).has_member( name ) ) {
} else if( proportional.has_member( name ) ) {
double scalar;
err = jo.get_object( "proportional" );
err = proportional;
if( !err.read( name, scalar ) || scalar <= 0 || scalar == 1 ) {
err.throw_error( "invalid proportional scalar", name );
}
Expand Down Expand Up @@ -299,21 +307,26 @@ inline bool assign( JsonObject &jo, const std::string &name, units::mass &val,

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;
err.allow_omitted_members();
JsonObject relative = jo.get_object( "relative" );
relative.allow_omitted_members();
JsonObject proportional = jo.get_object( "proportional" );
proportional.allow_omitted_members();

// Do not require strict parsing for relative and proportional values as rules
// such as +10% are well-formed independent of whether they affect base value
if( jo.get_object( "relative" ).has_member( name ) ) {
if( relative.has_member( name ) ) {
units::mass tmp;
err = jo.get_object( "relative" );
err = relative;
if( !parse( err, tmp ) ) {
err.throw_error( "invalid relative value specified", name );
}
strict = false;
out = val + tmp;

} else if( jo.get_object( "proportional" ).has_member( name ) ) {
} else if( proportional.has_member( name ) ) {
double scalar;
err = jo.get_object( "proportional" );
err = proportional;
if( !err.read( name, scalar ) || scalar <= 0 || scalar == 1 ) {
err.throw_error( "invalid proportional scalar", name );
}
Expand Down Expand Up @@ -359,21 +372,26 @@ inline bool assign( JsonObject &jo, const std::string &name, units::money &val,

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;
err.allow_omitted_members();
JsonObject relative = jo.get_object( "relative" );
relative.allow_omitted_members();
JsonObject proportional = jo.get_object( "proportional" );
proportional.allow_omitted_members();

// Do not require strict parsing for relative and proportional values as rules
// such as +10% are well-formed independent of whether they affect base value
if( jo.get_object( "relative" ).has_member( name ) ) {
if( relative.has_member( name ) ) {
units::money tmp;
err = jo.get_object( "relative" );
err = relative;
if( !parse( err, tmp ) ) {
err.throw_error( "invalid relative value specified", name );
}
strict = false;
out = val + tmp;

} else if( jo.get_object( "proportional" ).has_member( name ) ) {
} else if( proportional.has_member( name ) ) {
double scalar;
err = jo.get_object( "proportional" );
err = proportional;
if( !err.read( name, scalar ) || scalar <= 0 || scalar == 1 ) {
err.throw_error( "invalid proportional scalar", name );
}
Expand Down Expand Up @@ -424,21 +442,26 @@ inline bool assign( JsonObject &jo, const std::string &name, units::energy &val,

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;
err.allow_omitted_members();
JsonObject relative = jo.get_object( "relative" );
relative.allow_omitted_members();
JsonObject proportional = jo.get_object( "proportional" );
proportional.allow_omitted_members();

// Do not require strict parsing for relative and proportional values as rules
// such as +10% are well-formed independent of whether they affect base value
if( jo.get_object( "relative" ).has_member( name ) ) {
if( relative.has_member( name ) ) {
units::energy tmp;
err = jo.get_object( "relative" );
err = relative;
if( !parse( err, tmp ) ) {
err.throw_error( "invalid relative value specified", name );
}
strict = false;
out = val + tmp;

} else if( jo.get_object( "proportional" ).has_member( name ) ) {
} else if( proportional.has_member( name ) ) {
double scalar;
err = jo.get_object( "proportional" );
err = proportional;
if( !err.read( name, scalar ) || scalar <= 0 || scalar == 1 ) {
err.throw_error( "invalid proportional scalar", name );
}
Expand Down Expand Up @@ -484,7 +507,7 @@ class time_duration;
template<typename T>
inline typename
std::enable_if<std::is_same<typename std::decay<T>::type, time_duration>::value, bool>::type
read_with_factor( JsonObject jo, const std::string &name, T &val, const T &factor )
read_with_factor( JsonObject &jo, const std::string &name, T &val, const T &factor )
{
int tmp;
if( jo.read( name, tmp, false ) ) {
Expand Down Expand Up @@ -514,16 +537,21 @@ std::enable_if<std::is_same<typename std::decay<T>::type, time_duration>::value,

// Object via which to report errors which differs for proportional/relative values
JsonObject err = jo;
err.allow_omitted_members();
JsonObject relative = jo.get_object( "relative" );
relative.allow_omitted_members();
JsonObject proportional = jo.get_object( "proportional" );
proportional.allow_omitted_members();

// Do not require strict parsing for relative and proportional values as rules
// such as +10% are well-formed independent of whether they affect base value
if( read_with_factor( jo.get_object( "relative" ), name, out, factor ) ) {
err = jo.get_object( "relative" );
if( read_with_factor( relative, name, out, factor ) ) {
err = relative;
strict = false;
out = out + val;

} else if( jo.get_object( "proportional" ).read( name, scalar ) ) {
err = jo.get_object( "proportional" );
} else if( proportional.read( name, scalar ) ) {
err = proportional;
if( scalar <= 0 || scalar == 1 ) {
err.throw_error( "invalid proportional scalar", name );
}
Expand Down
12 changes: 7 additions & 5 deletions src/condition.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
const efftype_id effect_currently_busy( "currently_busy" );

// throws an error on failure, so no need to return
std::string get_talk_varname( JsonObject jo, const std::string &member, bool check_value )
std::string get_talk_varname( JsonObject &jo, const std::string &member, bool check_value )
{
if( !jo.has_string( "type" ) || !jo.has_string( "context" ) ||
( check_value && !jo.has_string( "value" ) ) ) {
Expand Down Expand Up @@ -61,7 +61,7 @@ void read_condition( JsonObject &jo, const std::string &member_name,
return sub_condition( d );
};
} else if( jo.has_object( member_name ) ) {
const JsonObject con_obj = jo.get_object( member_name );
JsonObject con_obj = jo.get_object( member_name );
conditional_t<T> sub_condition( con_obj );
condition = [sub_condition]( const T & d ) {
return sub_condition( d );
Expand Down Expand Up @@ -877,7 +877,7 @@ void conditional_t<T>::set_mission_has_generic_rewards()
}

template<class T>
conditional_t<T>::conditional_t( JsonObject jo )
conditional_t<T>::conditional_t( JsonObject &jo )
{
// improve the clarity of NPC setter functions
const bool is_npc = true;
Expand All @@ -890,7 +890,8 @@ conditional_t<T>::conditional_t( JsonObject jo )
conditional_t<T> type_condition( ja.next_string() );
conditionals.emplace_back( type_condition );
} else if( ja.test_object() ) {
conditional_t<T> type_condition( ja.next_object() );
JsonObject cond = ja.next_object();
conditional_t<T> type_condition( cond );
conditionals.emplace_back( type_condition );
} else {
ja.skip_value();
Expand Down Expand Up @@ -921,7 +922,8 @@ conditional_t<T>::conditional_t( JsonObject jo )
return false;
};
} else if( jo.has_object( "not" ) ) {
const conditional_t<T> sub_condition = conditional_t<T>( jo.get_object( "not" ) );
JsonObject cond = jo.get_object( "not" );
const conditional_t<T> sub_condition = conditional_t<T>( cond );
found_sub_member = true;
condition = [sub_condition]( const T & d ) {
return !sub_condition( d );
Expand Down
4 changes: 2 additions & 2 deletions src/condition.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const std::unordered_set<std::string> complex_conds = { {
};
} // namespace dialogue_data

std::string get_talk_varname( JsonObject jo, const std::string &member, bool check_value = true );
std::string get_talk_varname( JsonObject &jo, const std::string &member, bool check_value = true );

// the truly awful declaration for the conditional_t loading helper_function
template<class T>
Expand All @@ -70,7 +70,7 @@ struct conditional_t {
public:
conditional_t() = default;
conditional_t( const std::string &type );
conditional_t( JsonObject jo );
conditional_t( JsonObject &jo );

void set_has_any_trait( JsonObject &jo, const std::string &member, bool is_npc = false );
void set_has_trait( JsonObject &jo, const std::string &member, bool is_npc = false );
Expand Down
Loading

0 comments on commit 5f9d193

Please sign in to comment.