8000 XmlRpcValue - accessing a non-existing key throws std::out_of_range · Issue #2390 · ros/ros_comm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

XmlRpcValue - accessing a non-existing key throws std::out_of_range #2390

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

Jump to bottom
Closed
zarmomin opened this issue May 13, 2025 · 5 comments · Fixed by #2391
Closed

XmlRpcValue - accessing a non-existing key throws std::out_of_range #2390

zarmomin opened this issue May 13, 2025 · 5 comments · Fixed by #2391

Comments

@zarmomin
Copy link
zarmomin commented May 13, 2025 •
edited
Loading

One side-effect of this change is that retrieving a value of a non-existing key now throws an out of range instead of returning a default value.

#include "xmlrpcpp/XmlRpcValue.h"
#include <string>
#include <iostream>

void bla(const XmlRpc::XmlRpcValue& value) {
	const std::string& does_not_exist = value["this_does_not_exist"]; // this line now throws an exception
	std::cout << "does_not_exist:" << does_not_exist << std::endl;
}

int main() {
	XmlRpc::XmlRpcValue value;
	value["this_exists"] = "this exists";
	std::cout << "this_exists:" << value["this_exists"] << std::endl;
	bla(value);
	return 0;
}
@zarmomin zarmomin changed the title XmlRpcValue - accessing a non-existing key now throws XmlRpcValue - accessing a non-existing key throws std::out_of_range May 13, 2025
@sloretz
Copy link
Contributor
sloretz commented May 16, 2025

Thank you for the report and minimal example @zarmomin !

I'm trying to figure out if this behavior change should be treated like a regression and reverted. Would you mind explaining how you discovered this?

@sloretz sloretz mentioned this issue May 16, 2025
Merged
@mgrrx
Copy link
Contributor
mgrrx commented May 17, 2025

@sloretz this issue breaks a whole bunch of tests and packages on our side. Can you please revert the change until a proper fix has been provided?

@zarmomin
Copy link
Author

@sloretz Thanks for looking into this Shane! I noticed one of our unit-tests failing because of this - we were trying to access a non-existing value in a nested parameter. For us this actually caught a bug in the test, but it is unexpected to have this throw change from & to const& and start throwing exceptions without any outward communication about its new behavior.

@sloretz
Copy link
Contributor
sloretz commented May 19, 2025

I had to make sure I understand what's happening. I'm going to revert #2315 because it seems like a significant behavior change.

The new behavior introduced by #2315 seems better to me, but it should be a breaking change in a new ROS 1 distro, and there isn't going to be a new one of those.

The XmlRpcValue::operator[]() accesses _value, which in this case is of type ValueStruct, which is an std::map<>

ros_comm/utilities/xmlrpcpp/include/xmlrpcpp/XmlRpcValue.h

Line 43 in b6c57e7

typedef std::map<std::string, XmlRpcValue> ValueStruct;

Before #2315

It looks like ros/ros_comm at 4755096 (Just before #2315) uses the T& std::map<>::operator[]( const Key& key ); constructor, which inserts a default constructed value when the key doesn't exist.

Inserts value_type(key, T()) if the key does not exist.

After #2315

At b6c57e7 (1.17.3, current noetic-devel) it uses T& std::map<>::at( const Key& key );. This throws an exception if the key doesn't exist

std::out_of_range if the container does not have an element with the specified key.

Attempted fix #2391

With my fix the code uses T& std::map<>::operator[]( const Key& key ); again. However, XmlRpcValue::operator[]() returns type XmlRpcValue const & which in the given MRE is then cast to const std::string&(). This invokes the operator const std::string&() user-defined conversion function which raises an XMLRpcError. I think it's raising an error because a default constructed XmlRpcValue has its _type set to TypeInvalid, and the const version of assertTypeOrInvalid() always raises if the type is not set. The original code before #2315 doesn't raise XmlRpcError because the non-const version of assertTypeOrInvalid() sets _type and populates _value when _type is TypeInvalid.

sloretz added a commit that referenced this issue May 19, 2025
@sloretz
Revert "Maintain constness of accessed XmlRpcValue struct (#2315)"
ceb83e0 
This reverts commit ff77aa2.

#2390

The new behavior is reasonable, but users reported it broke their
builds.

Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
sloretz added a commit that referenced this issue May 19, 2025
@sloretz
Revert "Maintain constness of accessed XmlRpcValue struct (#2315)" (#…
767cb79 
…2391)

This reverts commit ff77aa2.

#2390

The new behavior is reasonable, but users reported it broke their
builds.

Signed-off-by: Shane Loretz <sloretz@intrinsic.ai>
@sloretz sloretz mentioned this issue May 19, 2025
Merged
@sloretz
Copy link
Contributor
sloretz commented May 19, 2025

Released as 1.17.4 ros/rosdistro#45848

@mgrrx @zarmomin Please try out the new release from the testing apt repos and report if it fixes the broken unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Revert: Maintain constness of accessed XmlRpcValue struct ros/ros_comm
3 participants
@sloretz @mgrrx @zarmomin
0