[#204] Handle invalid strings by 'blanking' variable out (4-3-stable)

[MartinFlores751]

The current implementation was to make sure the solution works (which it does). Discussion can be had on if it should be implemented as is, or if it should be moved to the templated function where the error comes from.

[MartinFlores751]

Template function where the exception was thrown:

irods_rule_engine_plugin_python/include/irods/private/re/python.hpp

Lines 335 to 340 in 208ecb2

	template <>
	boost::python::object object_from_specific<const char*>(boost::any& arg)
	{
	const char* str = boost::any_cast<const char*>(arg);
	return str ? boost::python::object{std::string{str}} : boost::python::object{};
	}

Also, after the rule executes, we update the variables again:

irods_rule_engine_plugin_python/src/main.cpp

Lines 574 to 581 in 208ecb2

	int i = 0;
	for (auto& cpp_argument : rule_arguments_cpp) {
	bp::object py_argument = rule_arguments_python[i];
	update_argument(cpp_argument, py_argument);
	++i;
	}
	return to_irods_error_object(ec);

Is that an issue?

[korydraughn]

The current implementation was to make sure the solution works (which it does).

What does works mean here? Does it mean the agent doesn't crash?

With your solution, what can users/admins expect now? Does anything appear in the log file?

Template function where the exception was thrown:

Is there a benefit to moving the try-catch inside the template function?
If we did that, how would the agent react when the UTF-8 case occurs?

Is that an issue?

I'm not sure. Do a little digging to see what update_arguments does and think about how that plays into your solution.

[trel]

blanking does not send any signal to the rule author who may try to access a value that could not be string-ified.

not sure if we've agreed that we want to 'blank' the offending binary data, or set it to some well-defined/documented string value.

possible candidates...

• ''
• 'SYS_NOT_SUPPORTED'
• 'NONSTRING_NOT_AVAILABLE'
• 'NONCONVERTIBLE_BINARY_DATA'
• 'CONVERTED_TO_STRING'
• 'CONVERTED_FROM_NONSTRING'

[MartinFlores751]

The current implementation was to make sure the solution works (which it does).

What does works mean here? Does it mean the agent doesn't crash?

Works means it does not crash, and there are no obvious unintended side effects of the solution.
Funky wording because I'm not sure about update_arguments(...), but it doesn't seem to affect anything?

With your solution, what can users/admins expect now?

The affected functions should execute normally, barring the affected variable.

Some consideration may be needed if there were a real UTF-8 issue (if we even support UTF?).

Does anything appear in the log file?

No, we can log if we desire, though.

Template function where the exception was thrown:

Is there a benefit to moving the try-catch inside the template function?

Narrower scope? Less chance of catching other, possibly unrelated issues (e.g. from the <char*> template function).

If we did that, how would the agent react when the UTF-8 case occurs?

It would effectively be handled similarly, just further up the stack.

Is that an issue?

I'm not sure. Do a little digging to see what update_arguments does and think about how that plays into your solution.

Is it common to modify passed in pep arguments, and have them remain modified afterwards for rule engines? If so, then I think it would depend on how the blanked out variable is used (if at all)?

[MartinFlores751]

blanking does not send any signal to the rule author who may try to access a value that could not be string-ified.

not sure if we've agreed that we want to 'blank' the offending binary data, or set it to some well-defined/documented string value.

possible candidates...

* ''

* 'SYS_NOT_SUPPORTED'

* 'NONSTRING_NOT_AVAILABLE'

* 'NONCONVERTIBLE_BINARY_DATA'

* 'CONVERTED_TO_STRING'

* 'CONVERTED_FROM_NONSTRING'

If we have something similar in the error table, I think that might be best?

A few that might fit:

• 'TYPE_NOT_SUPPORTED'
• 'UNKNOWN_PARAM_IN_RULE_ERR'
• 'INPUT_ARG_NOT_WELL_FORMED_ERR'
• 'INVALID_OBJECT_TYPE'
• 'RE_RUNTIME_ERROR'
• 'RE_UNSUPPORTED_OP_OR_TYPE'
• 'RE_UNABLE_TO_READ_VAR'
• 'RE_TYPE_ERROR'
• 'RE_PACKING_ERROR'

[korydraughn]

I think the response is supposed to be treated as a raw byte sequence. Sadly, there doesn't appear to be a way to provide this distinction (i.e. we can't change the PEP signature).

Wait, we know the PEP, so it should be possible to take a special code path based on that.

[trel]

right, that could be a way forward. have to hold it very carefully though.