Skip to content
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.

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

Incorrect saving metadata sequence #37

Open
savuor opened this issue Feb 8, 2016 · 1 comment
Open

Incorrect saving metadata sequence #37

savuor opened this issue Feb 8, 2016 · 1 comment
Labels

Comments

@savuor
Copy link
Collaborator

savuor commented Feb 8, 2016

The problem appears when disabling return codes in MetadataStream::save() and saveTo() methods (see PR #36).
Test case TestSaveLoadReference.OneToOneReference fails because of incorrect procedure of metadata saving.

In modules/vmdatasource/src/xmpmetadatasource.cpp:118:

xmp->SetStructField(VMF_NS, thisPropertyPath.c_str(), VMF_NS, PROPERTY_SET, nullptr, kXMP_PropValueIsArray);

That string deletes a branch from the syntax tree that XMP keeps internally.

After that the saving procedure writes metadata records, generating paths for them.
In modules/vmdatasource/src/xmpmetadatasource.cpp:137:

    auto it = idMap.find(md->getId());
    if (it == idMap.end())
    {
        xmp->AppendArrayItem(VMF_NS, thisPropertySetPath.c_str(), kXMP_PropValueIsArray, nullptr, kXMP_PropValueIsStruct);
        SXMPUtils::ComposeArrayItemPath(VMF_NS, thisPropertySetPath.c_str(), kXMP_ArrayLastItem, &pathToMetadata);
    }
    else
    {
        pathToMetadata = it->second.path;
    }

Shortly, if there is an existing metadata record then save it using old path in the tree, if there's a new metadata record then append it to the tree.

This code works only if saving old records exactly in the order they were stored in the original file and only after that adding new items.
Metadata removal should be performed separately from that procedure. If it's performed before then all paths are becoming outdated, that is why open/close or SerializeToBuffer/ParseFromBuffer routines should be performed after removal procedure.

Due to some concourse of circumstances in most cases the metadata records are saved in the described order. But in the test case TestSaveLoadReference.OneToOneReference we have a single metadata record that belongs to the description we didn't ask to load (but this record should be loaded because there's a reference pointing on it).
Here we cannot guarantee the order of records and get an exception.

Moreover, here we drop the branch containing the metadata records for the same description that were not loaded. That's why opening and saving such a file will erase them.

@savuor
Copy link
Collaborator Author

savuor commented Feb 8, 2016

TODO: write a test for the following situation:

  1. There are metadata descriptions A and B and corresponding metadata records.
  2. Metadata record 1 from A is referencing metadata record 2 from B
  3. Metadata record 2 from A is referencing metadata record 1 from B
  4. User writes this data and closes the file.
  5. User opens this file, loads description A, saves the file and closes it.

If everything is broken the way it described above then in description B metadata records will be loaded in the order [2, 1] which will produce an exception at saving.

UPD. This may work because of the following:
In modules/vmdatasource/src/xmpmetadatasource.cpp:226:

stream.sortById();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant