arangodb / velocypack

A fast and compact format for serialization and storage
Other
420 stars 40 forks source link

Strange VPackBuilder behaviour #156

Closed olegrok closed 1 year ago

olegrok commented 1 year ago

I got following code


VPackBuilder builder;
builder.add("key1", "value1");
builder.add("key2", "value2");
builder.close();

auto slice = builder.slice();
builder = VPackCollection::remove(slice, std::vector<std::string>{"key2"});

VPackBuilder newBuilder(builder.slice());
newBuilder.openObject();

newBuilder.add("key2", "value_new2");
newBuilder.add("key3", "value3");

std::cerr << "hasKey('key1') = " << newBuilder.hasKey("key1") << std::endl; // true
std::cerr << "hasKey('key2') = " << newBuilder.hasKey("key2") << std::endl; // true
std::cerr << "hasKey('key3') = " << newBuilder.hasKey("key3") << std::endl; // true
std::cerr << "hasKey('key4') = " << newBuilder.hasKey("key4") << std::endl; // false

newBuilder.close();

std::cerr << newBuilder.toJson() << std::endl; // {"key1": "value1"}

Expected behaviour: my json contains key1, key2 and key3 (and corresponding values). Or at least I should get an error on attempt to add key2 and key3.

As result I solved my issue using newBuilder.add(VPackObjectIterator(builder.slice()));. But it wasn't obvious and took too much time. Also seems such approach produces excessive copies that I want to avoid.

jsteemann commented 1 year ago

I see the following problems here:

VPackBuilder builder;
builder.add("key1", "value1");
builder.add("key2", "value2");
builder.close();

I guess the intention here is to produce an object with 2 keys, key1 and key2. The code for this should be

VPackBuilder builder;
builder.openObject();
builder.add("key1", VPackValue("value1"));
builder.add("key2", VPackValue("value2"));
builder.close();

Otherwise you will get an need open compound value (Array or Object) error already on the first add(...) call.

jsteemann commented 1 year ago

This also looks problematic:

VPackBuilder newBuilder(builder.slice());
newBuilder.openObject();

because it will first assign an already created and closed (i.e. sealed) object to newBuilder, and then opening it again. This may be a misconception, but the openObject() will add re-open an existing object contained in the Builder. A Builder is designed to be used in an append-only fashion. So when calling openObject() here, it will start a new object behind the object that's already contained in the Builder.

If you want to start from scratch in an existing Builder object, you can always use builder.clear() to wipe all data from it.

Maybe can you elaborate a bit on the problem you are trying to solve, because I don't fully understand what the goal of the example code above is. Thanks!

olegrok commented 1 year ago

Maybe can you elaborate a bit on the problem you are trying to solve, because I don't fully understand what the goal of the example code above is. Thanks!

I have to parse json string then slightly modify it (replace one key with another) and convert back to json.

jsteemann commented 1 year ago

What about

  std::string json = "{\"foo\":\"bar\",\"baz\":\"bark\",\"qux\":\"quz\"}";
  auto builder = VPackParser::fromJson(json);

  VPackBuilder replacements;
  replacements.openObject();
  replacements.add("baz", VPackValue("replaced"));
  replacements.close();

  auto result = VPackCollection::merge(builder->slice(), replacements.slice(), false, false);
  std::cout << result.toJson() << std::endl;

An alternative would be to only build the result object once, like this:

  std::string json = "{\"foo\":\"bar\",\"baz\":\"bark\",\"qux\":\"quz\"}";
  auto builder = VPackParser::fromJson(json);

  VPackBuilder result;
  result.openObject();
  for (auto it : VPackObjectIterator(builder->slice())) {
    if (it.key.stringView() == "baz") {
      result.add(it.key.stringView(), VPackValue("replaced"));
    } else {
      result.add(it.key.stringView(), it.value);
    }
  }
  result.close();
  std::cout << result.toJson() << std::endl;
olegrok commented 1 year ago

Yes. Seems it solves my issue. I wanted to avoid copies and tried to modify the source object. But seems it's not an option.

Thanks!