Bug(C++): undefined behavior in `vertices_builder.h`

[Sober7135]

Describe the bug, including details regarding any error messages, version, and platform.

1. graphar::builder::Vertex provides a default constructor that does not initialize id_. However, GetId() can still be called and may return an uninitialized value, which is undefined behavior.

incubator-graphar/cpp/src/graphar/high-level/vertices_builder.h

Lines 52 to 68 in 7bad8ec

	class Vertex {
	public:
	Vertex() : empty_(true) {}
	/**
	* @brief Initialize the vertex with a given id.
	*
	* @param id The id of the vertex.
	*/
	explicit Vertex(IdType id) : id_(id), empty_(false) {}
	/**
	* @brief Get id of the vertex.
	*
	* @return The id of the vertex.
	*/
	inline IdType GetId() const noexcept { return id_; }

2. graphar::builder::Vertex also provides SetId(IdType id), but SetId does not set empty_ to false. This seems inconsistent with the Vertex(IdType id) constructor (which sets empty_ = false) and is likely not the expected behavior.

incubator-graphar/cpp/src/graphar/high-level/vertices_builder.h

Lines 70 to 75 in 7bad8ec

	/**
	* @brief Set id of the vertex.
	*
	* @param id The id of the vertex.
	*/
	inline void SetId(IdType id) { id_ = id; }

Component(s)

C++

[Sober7135]

CC @yangxk1

I have come up with 2 solutions:

1. Remove empty_, and require users to construct a Vertex with an id (this may be an API-breaking change).
2. In GetId(), call Empty() first; if the vertex is empty, throw an exception.

Any thoughts?

[SYaoJun]

CC @yangxk1

I have come up with 2 solutions:

1. Remove empty_, and require users to construct a Vertex with an id (this may be an API-breaking change).
2. In GetId(), call Empty() first; if the vertex is empty, throw an exception.

Any thoughts?

I prefer the second solution. It avoids breaking changes to the API.

[Sober7135]

I have some new findings:
The meaning of empty_ is ambiguous. AddProperty() sets empty to false even if the id_ is still uninitialized, so empty_ does not reliably indicate whether GetId() is safe to call.

incubator-graphar/cpp/src/graphar/high-level/vertices_builder.h

Lines 91 to 118 in 7bad8ec

	inline void AddProperty(const std::string& name, const std::any& val) {
	empty_ = false;
	properties_[name] = val;
	}
	inline void AddProperty(const Cardinality cardinality,
	const std::string& name, const std::any& val) {
	if (cardinality == Cardinality::SINGLE) {
	cardinalities_[name] = Cardinality::SINGLE;
	AddProperty(name, val);
	return;
	}
	empty_ = false;
	if (cardinalities_.find(name) != cardinalities_.end()) {
	if (cardinalities_[name] != cardinality) {
	throw std::runtime_error("Cardinality mismatch for property: " + name);
	}
	auto property_value_list =
	std::any_cast<std::vector<std::any>>(properties_[name]);
	property_value_list.push_back(val);
	properties_[name] = property_value_list;
	} else {
	auto property_value_list = std::vector<std::any>();
	property_value_list.push_back(val);
	properties_[name] = property_value_list;
	}
	cardinalities_[name] = cardinality;
	}

TBH, I prefer removing the empty_ since this is the cleanest way, and I think constructing a Vertex with an id is the most straightfoward way.

For the API change, maybe we can first deprecate the default constructor and Empty() and initialize id_ to -1 ?

For deprecation in C++, see https://stackoverflow.com/questions/295120/how-can-i-mark-a-c-class-method-as-deprecated

CC @yangxk1 @SYaoJun

[SYaoJun]

@Sober7135 I favor your idea. By the way, I noticed some bad smells in the current implementation: the use of inline is redundant in header files, and class members lack default values in their declarations. Could you fix these issues in your refactored code?