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

“isOldAttachment” check is incorrect #2022

snej opened this issue Jan 16, 2018 · 2 comments


Copy link

commented Jan 16, 2018

I just saw this change in commit 6cc6e94:

    // Check whether the dictionary is the old attachment or not:
    static bool isOldAttachment(Dict properties, DocContext *context) {
        auto sk = context->sharedKeys();
        if (properties.get(C4STR("digest"), sk) != nullptr &&
            properties.get(C4STR("length"), sk) != nullptr &&
            properties.get(C4STR("stub"), sk) != nullptr &&
            properties.get(C4STR("revpos"), sk) != nullptr &&
            properties.get(C4STR("content_type"), sk) != nullptr)
            return true;
        return false;

This isn't going to work correctly.

  1. An attachment isn't required to have a content_type property; the developer may not have set one.
  2. Attachments only appear inside the top-level _attachments property. An object elsewhere in the document, with the above properties, isn't an attachment. It seems unlikely that this would happen, but maybe if someone were archiving old or conflicting versions of a doc inside a nested property?

The second point might be nit-picky, and it looks like it would complicate the calling code to have to know where in the document the properties Dict is, so maybe we can ignore it. But I wanted to point it out.

@snej snej added this to the 2.0.0 milestone Jan 16, 2018

@snej snej added the f: Attachment label Jan 16, 2018


This comment has been minimized.

Copy link

commented Jan 16, 2018

Thanks @snej for pointing this out. CC: @hideki and @borrrden.

pasin added a commit that referenced this issue Jan 16, 2018

Fix isOldAttachment logic
The logic shouldn’t check the content-type as it’s an optional.


This comment has been minimized.

Copy link

commented Jan 16, 2018

I did the fix for the first point. I will leave the issue open to think about the second point.

@djpongh djpongh added the P1: high label Jan 18, 2018

@pasin pasin added P2: medium and removed P1: high labels Jan 19, 2018

@djpongh djpongh removed the cross-platform label Jan 27, 2018

@djpongh djpongh modified the milestones: 2.0.0, 2.1.0 Jan 27, 2018

@djpongh djpongh added icebox and removed P2: medium labels Jan 27, 2018

@djpongh djpongh modified the milestones: 2.1.0, 2.2.0 Apr 3, 2018

@pasin pasin closed this Aug 21, 2018

@pasin pasin removed the icebox label Aug 21, 2018

@djpongh djpongh modified the milestones: 2.5.0, Iridium Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
3 participants
You can’t perform that action at this time.