Bug 496719

This is a common behavior across multiple APIs that take as input a structure of name / value pairs.  Generally, required parameters are checked, but invalid or unknown parameters are ignored.  If we are to address this, we should do so across all of the APIs for consistency.

Do we really want to do this in Sat 5.3.0?

The behavior described by the bug is actually a consistent behavior across APIs that take a struct/map as input.  If the user provides an entry that is not used/defined, it is just ignored and the logic continues to process the request.

In order to address this bug, I'd prefer to address this generically; however, it will mean a regression of all APIs that take a struct/map as input.  The generic solution being, check the input struct/map and if any 'uknown' values are received throw an exception listing the errors.

From a development point of view, this is doable, but want to confirm that this is really 'ok' before proceeding.

Approved.  As generally all API testing is completely automated, I have no problem with us becoming more restrictive and doing extra error checking.  And ideally this will give us more opportunities for adding new tests.  We still have a reasonable amount of time to deal with any potential fall-out, and generally it is the *right* thing to do.

master git commit: 618a327382fba83905047265ac9c1925570b97fb
vader git commit: 344868173dae8b27a59e2e051a9466640bb57767
 
The usage of map (i.e. struct in docs) wasn't as wide-spread as I had expected; however, in order to ensure that the behavior is handled consistently, the following APIs were updated:

activationkey.addPackages
activationkey.removePackages
activationkey.setDetails
channel.software.clone
configchannel.createOrUpdatePath
errata.create
errata.setDetails
system.setDetails
system.config.createOrUpdatePath
system.provisioning.snapshot.deleteSnapshots (2 APIs)
system.provisioning.snapshot.listSnapshots
user.setDetails

With these changes, if the user provides an invalid key (e.g. 'Description' from the initial bug description), an exception (InvalidArgsException : id=2801) will be thrown.

mass move to ON_QA

verified and all tests are updated for extra checking.

Verified on staged (Satellite-5.3.0-RHEL5-re20090724.0) with updates from Aug 20, 2009

Verified that all the APIs listed in Comment #4 have a corresponding JUnit test that verifies an exception (redstone.xmlrpc.XmlRpcFault: Invalid argument(s):) is presented when an invalid argument is passed to the API.

For example, the activationkey.addPackages rpcapi JUnit test class contains:
/**
 * Simple activationkey add_packages with an invalid map argument key.
 *
 * @throws IOException ...
 * @throws XmlRpcException ...
 */
@SuppressWarnings("unchecked")
public void testBadHashmapKeyInArguments() throws Exception {
    	ActivationKey key = new ActivationKey();
    	Map hashMap = new HashMap();
    	List<Map> list = new ArrayList<Map> ();
    	hashMap.put("Description", "description");
    	list.add(hashMap);
    	badItemTest("activationkey.addPackages", "redstone.xmlrpc.XmlRpcFault: Invalid argument(s): Description", securityToken, key.getKey(), list);
    	key.delete();
}



$ ant -Dtestcase=addPackages 
...
    [junit] Running com.redhat.rhn.rpc.api.activationkey.addPackages
    [junit] Tests run: 2, Failures: 0, Errors: 0, Time elapsed: 1.127 sec


$ ant -Dtestcase=removePackages 
...
    [junit] Running com.redhat.rhn.rpc.api.activationkey.removePackages
    [junit] Tests run: 2, Failures: 0, Errors: 0, Time elapsed: 1.346 sec


$ant -Dtestcase=setDetails
...
    [junit] Running com.redhat.rhn.rpc.api.activationkey.setDetails
    [junit] Tests run: 7, Failures: 0, Errors: 0, Time elapsed: 2.284 sec
    [junit] Running com.redhat.rhn.rpc.api.errata.setDetails
    [junit] Tests run: 3, Failures: 0, Errors: 0, Time elapsed: 0.707 sec
    [junit] Running com.redhat.rhn.rpc.api.system.setDetails
    [junit] Tests run: 5, Failures: 0, Errors: 0, Time elapsed: 8.965 sec
    [junit] Running com.redhat.rhn.rpc.api.user.setDetails
    [junit] Tests run: 8, Failures: 0, Errors: 0, Time elapsed: 2.765 sec


$ ant -Dtestcase=clone
...
    [junit] Running com.redhat.rhn.rpc.api.channel.software.clone
    [junit] Tests run: 4, Failures: 0, Errors: 0, Time elapsed: 1.568 sec


$ ant -Dtestcase=createOrUpdatePath
...
    [junit] Running com.redhat.rhn.rpc.api.configchannel.createOrUpdatePath
    [junit] Tests run: 2, Failures: 0, Errors: 0, Time elapsed: 1.113 sec
    [junit] Running com.redhat.rhn.rpc.api.system.config.createOrUpdatePath
    [junit] Tests run: 2, Failures: 0, Errors: 0, Time elapsed: 8.626 sec


$ ant -Dtestcase=create
...
    [junit] Running com.redhat.rhn.rpc.api.errata.create
    [junit] Tests run: 2, Failures: 0, Errors: 0, Time elapsed: 0.63 sec


$ ant -Dtestcase=deleteSnapshots
...
    [junit] Running com.redhat.rhn.rpc.api.system.provisioning.snapshot.deleteSnapshots
    [junit] Tests run: 3, Failures: 0, Errors: 0, Time elapsed: 13.812 sec


$ ant -Dtestcase=listSnapshots
...
    [junit] Running  com.redhat.rhn.rpc.api.system.provisioning.snapshot.listSnapshots
    [junit] Tests run: 2, Failures: 0, Errors: 0, Time elapsed: 9.176 sec


moving to RELEASE_PENDING

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHEA-2009-1434.html