Description of problem: Method: setDetails Description: Update the details of an activation key. Parameters: * string sessionKey * string key * struct - activation key o string "description" o string "base_channel_label" o int "usage_limit" o boolean "universal_default" Returns: * int - 1 on success, exception thrown otherwise. So basically valid keys are "description", "base_channel_label", "usage_limit" and "universal_default", but even if you provide wrong key like "Description" or "foobar". call returns 1(success). >>> server.activationkey.setDetails(key, "1-ef2f8791f42d9df5c74539f387016f8d", ({"Description": "foo"})) 1 so basically description is not changed. >>> server.activationkey.setDetails(key, "1-ef2f8791f42d9df5c74539f387016f8d", ({"foobar": 1})) 1 Nothing is changed Expected result: Error message displaying wrong key.
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