set metadata method should only set the fields given, not delete all the existing metadata

[irinaColgiu]

I think there was a misunderstanding as to what "set metadata does" - it should be setting the fields given as parameter (in the sense that if there is already an AVU with the existing key, its values should be changed to the values given as parameter). This operation, however, shouldn't affect other avus, but only the ones given in the IrodsMetadata as parameter. Currently the implementation is:

def set(self, path: str, metadata: IrodsMetadata):

baton does not support "set" natively, therefore this operation is not transactional

    existing_metadata = self.get_all(path)
    self.remove(path, existing_metadata)
    self.add(path, metadata)

https://github.com/wtsi-hgi/python-baton-wrapper/blob/5197522430f83799f16a488341ba81d832c359ee/baton/_baton/baton_metadata_mappers.py#L42-L46

[colin-nolan]

The set and add operations are based on those supplied by the imeta icommand. https://docs.irods.org/4.1.8/icommands/metadata/

set will overwrite all existing AVUs, in the same way imeta set does, as documented here: https://github.com/wtsi-hgi/python-baton-wrapper/blob/master/baton/mappers.py#L39

I'm not too sure what the effect of adding a different value to a pre-existing is. It's possible it does the update operation that you're looking for. If not, I can add an update operation.

I shall make it more clear what set does in the README: https://github.com/wtsi-hgi/python-baton-wrapper#metadata-avus

[irinaColgiu]

I don't see where in the definition of set according to the docs mentioned by you, it says that this commands deletes all the existing metadata. It deletes ONLY the values associated to the keys being set, not all the metadata of that data object:

" 'set' modifies an AVU if it exists, or creates one if it does not. If the AttName does not exist, or is used by multiple objects, the AVU for this object is added. If the AttName is used only by this one object, the AVU (row) is modified with the new values, reducing the database overhead (unused rows). Example: set -d file1 distance 12"

The documentation is also consistent with what imeta does in practice:

imeta ls -d /humgen/projects/serapis_staging/test-baton/test_add_metadata.txt
AVUs defined for dataObj /humgen/projects/serapis_staging/test-baton/test_add_metadata.txt:
attribute: key2
value: val2
units: 

attribute: key1
value: val1
units: 
(ENV) mercury@hgi-serapis-farm3:/nfs/users/nfs_i/ic4/Projects/python3/python-baton-wrapper$ imeta set -d /humgen/projects/serapis_staging/test-baton/test_add_metadata.txt key2 val3
(ENV) mercury@hgi-serapis-farm3:/nfs/users/nfs_i/ic4/Projects/python3/python-baton-wrapper$ imeta ls -d /humgen/projects/serapis_staging/test-baton/test_add_metadata.txt
AVUs defined for dataObj /humgen/projects/serapis_staging/test-baton/test_add_metadata.txt:
attribute: key2
value: val3
units: 

attribute: key1
value: val1
units: 

As shown, the set command modified only the key given, the rest of the AVUs remained the same.

[colin-nolan]

Aaa yes, you're right. I'm getting confused with the access control operations! Perhaps the metadata operations should be aligned with them?

[irinaColgiu]

I think it's confusing if the baton wrapper is not aligned to what the icommands are doing roughly, cause then it's hard to bare in mind what the first one is doing, what the other one is doing. Especially if one is using both (like I am).

[colin-nolan]

After looking into this further, it is apparent that the implementation for metadata set operations does not align with the documentation. Its functionality is instead similar to that of access control's set method, which is not appropriate as ACLs are rather different.

I shall fix the implementation (and tests, which are also wrong thanks to them being written by the same person that didn't implement the interface correctly!) such that this method works as expected.