[BUG] rgw/sfs: SFSBucket::merge_and_store_attrs should not drop entries not defined in the new_attrs

[giubacc]

In SFSBucket::merge_and_store_attrs we should drop this block:

  for (auto& it : attrs) {
    auto it_find = new_attrs.find(it.first);
    if (it_find == new_attrs.end()) {
      // this is an old attr that is not defined in the new_attrs
      // delete it
      attrs.erase(it.first);
    }
  }

The method should only merge and not set the new attributes into the current attributes.

[jecluis]

@giubacc can you provide something that reproduces the behavior, or a set of commands with the final expected state?

[giubacc]

Example: supposing current_attrs has the following items:

{A, B, C}

and new_attrs

{A, C, D}

calling the method produces:

{A, C, D}

but, given that this should be a merge the set should be this:

{A, B, C, D}

I haven't a concrete case to report, I stumbled into that code accidentally when looking another thing.
I've talked a bit with @0xavi0 about this and I think the code above was added to make some lifecycle test passing (?)

[jecluis]

Thanks. This sounds like a potentially low-hanging fruit, so lets see if we can handle it soon.

[giubacc]

Well, I've gone in depth into this and the true reality is that the method: merge_and_store_attrs is actually used as a set_and_store_attrs.

For example, consider this fragment from RGWDeleteBucketTags::execute:

    rgw::sal::Attrs attrs = s->bucket->get_attrs();
    attrs.erase(RGW_ATTR_TAGS);
    op_ret = s->bucket->merge_and_store_attrs(this, attrs, y);

The merge as intended in my previous comment is not the case.
Given that I devoted some time into this I decided to go with a PR simplify the current unnecessary complicated logic and rewrite the method as it should be: a simple set.

[giubacc]

Invalid